To encourage iOS developers to approach code review more throughly we've built a simple check list that should be followed when going through an iOS pull request.
Of course we do not go through it and add checkmarks on every PR but we use it to remind us from time to time what should we look for when code reviewing. This is also a good start for someone who is new in iOS development or code reviewing in general.
1. Are there any merge conflicts in the PR?
- If yes - return to assignee before looking further into code.
2. Did the static analysis check run?
- eg. for checking the Swift code style we use Swift Lint
3. Check the architecture
- eg. data should not be fetched or processed in presenters or in views
- eg. views should not be modified in view models
4. Is the code consistent with the agreed coding guidelines?
5. Are there any new Xcode warnings introduced?
6. Prefer static constants over computed properties
Prefer
static let c: Int = 1234
Over
static var d: Int { return 1234 }
7. Check single source of truth principle
- eg. don’t preserve the same information in multiple places in the code
8. Avoid singletons
- use dependency injection instead
9.Look for force unwrapping
- force unwrap should be avoided (use in < 1% of the cases)
- Using ! should always be a red flag!
- Check if early return from functions (eg. guard) is used
10.Constants
- Extract constant to a struct
11.Check for magic numbers
- Avoid harcoding, avoid magic numbers, if needed extract into a constant
- If added, magic numbers should be commented
12.Check encapsulation
- Check if public variable or function can be private or private(set)?
- If a class won’t be ever instantiated - use enum instead.
13.Check polymorphism
- Should we mark the class final?
- Should we use class or static keyword for a function/variable.
- Prefer composition with protocols over inheritance.
14.Check for single responsability
- The function should have one purpose! Split it in two if makes sense.
- The class should not have too many responsabilities - split if needed.
- Avoid massive view controller, add managers, helpers, utils instead.
15.Check for explicit use of self keyword
- Explicit (unneeded) use of self should be avoided because it makes the capturing semantics of self stand out more in closures, and prevents verbosity elsewhere
16.Performance check
- use strings concatenation with /() instead of +
- use isEmpty instead of == nil
- use ! instead of == false
- instantiate DateFormatter only once
- reuse cells
- use image caching
- use background threads where needed
- etc…
17.Error handling
- Are all the errors cases handled in code? Log the error, notify the user, retry, fail safely…
18.Check security vulnerabilities
- Are there any user passwords or other secrets stored as plain text?
- Is there any sensitive data being logged?
19.Check naming
- Is naming clear and consistent. The naming is important for writing self-documenting code!
- Boolean values should start with is, can, should, will…
- When in doubt - use longer names (adds more information) over shorter names (can bring confusion).
- Use UpperCamelCase for types and protocols, lowerCamelCase for everything else.
20.Check duplicate code
- Duplicate code is a big no no and should be extracted and reused!
21.Remove unnecessary code
- Remove commented code. You can always get it back with git.
- Remove empty and/or unused variables, functions, imports…
- No need to add explicit init for structs if using only the default initialiser.
22.Memory leaks
- Closures should use weak self
- Delegates should be weak
- Check if unowned is misused
- Check for any retain cycles
23.Check if all strings are localised
24.Is the code covered with logs?
- Every important event in the app should be logged.
- The logs should contain enough information (class, function, parameters, severity).
- Repeating information can pollute the log.
25.Is the code well documented?
- Does every public function contain a comment?
- Does the code contain a comment for every specific part of the code?
- Does every specific class (manager, service, script…) contain a comment about their purpose?
- Does every file contain a copyright?
- Should the README be updated?
As mention before, we use this checklist as a reminder and a motivation for doing better code reviews. Thus, we are adding bullets to our checklist and improving it over time.
What would you add to the list? 🙂
Top comments (1)
The point with "Prefer static constants over computed properties" doesn't look clear. As far as I remember static constants blows a memory footprint.