Code review checklist
This is a check list for iOS and Mac project code reviews. A good time for going through this is when merging feature branches.
Hygiene and non-code issues
No unnecessary imports/@class decls
Sorted @class decls
.m file, put the corresponding
.h file on the first
#import line followed by an empty line and the rest of the imports sorted.
Sorted Xcode groups
Keep the contents of Xcode groups sorted.
Methods in good order
Are the methods in sensible order, with good
#pragma marks for Xcode/AppCode grouping?
No compiler warnings
Use -Werror (at least for release builds) and lots of warning flags.
No analyzer warnings
Formatted to standards
Have a consistent style for the project and follow it. If you use clang-format or Uncrustify, check it has been run.
Spelling and grammar
It doesn't have to be Shakespeare but no obvious spelling or grammar errors should be introduced in names or in comments.
No configuration changes in project file if using xcconfigs
All images are of correct size and have been run through ImageOptim or similar
Hard to check ImageOptim has been run, but at least the person adding the image should know. It can sometimes cause trouble, but you should at least try running it.
All graphics have both @1x and @2x versions
And @2x have the correct dimensions.
No broken tests
Unit tests for computation
If you are writing unit tests, does at least new non-UI, non-IO code have unit tests?
If you are writing KIF tests or similar, does new UI functionality have tests?
Documentation and licenses
All existing documentation updated to match code
All license displays updated to match dependencies
If you keep a changelog, every feature branch should include an update to it.
Follow Apple's naming conventions
Code should follow Apple's Coding Guidelines for Cocoa.
As readable coupling as possible
Cocoa coupling methods, ordered by readability:
- Direct call
- Responder chain
Prefer methods higher up on the list to ones lower down, but don't shoot yourself in the foot with too tight coupling. Consistency is good.
Queues rather than threads
In general NSThreads should only be used where the framework forces you to with a service that requires access from a single thread, such as address book. Otherwise use GCD or NSOperationQueue.
Strict notification threading policy
Use one notification center per thread. Use defaultCenter only from main thread. Make it the sender's responsibility to use the correct thread.
Sane notification registration
Are notification listeners added and removed in sane places wrt. view visibility etc? Removed in dealloc?
Separate event handling from logic
A notification handler, a target-action method or similar is rarely the best place to implement complex logic.
Never block main thread
Does the code block main thread with disk IO, networking or computation?
Provide NSStringFromEnum / NSStringFromStruct methods
If the code introduces new enums or structs, it should also provide NSStringFromEnum/Struct functions.
Every class should have a good
-description method that provides useful information for debugging.
Minimize object creation, release as early as possible
Is the code creating temporary objects inside loops? Does it have to? Can it add an autoreleasepool?
enum over static ints
Enums are usually nicer than static ints.
NS_ENUM is helpful.
Cocoa naming conventions
The code should follow Cocoa naming conventions to indicate memory management.
If a function doesn't expect
NULL, it should be marked with
If a method must be called by subclasses when overriding, it should use
If a function does not modify global state, it should use
__attribute__((pure)). If it doesn't read it either (even better), it should use
Are all user-visible strings displayed through NSLocalizedString or similar internationalization mechanism? Are corresponding strings added to strings files, if they are used?
dispatch_once for singletons
Does the code really need a singleton? If it does, does it use
frame vs. bounds
Know which one you are dealing with and use the correct property.
Overall code quality
Simple class responsibilities, high cohesion
Are class responsibilities cleanly defined? Should a class be split? Should code be collected from multiple places into one?
Does everything have a sensible name?
Every notification handler should assert that they're in the thread they think they should be. Doesn't hurt in public methods, either.
Assert that things that shouldn't be nil aren't nil.
Assert other values too.
Minimal, pluggable shared state
Is all the shared state necessary? Does it have to be mutable? Does the code share state via singletons where dependency injection would be possible?
Prefer functions to class methods to instance methods
As much of the logic as possible should be implemented using pure functions because they are easiest to reason about. Class methods can offer a middle ground by offering Objective-C syntax and class as a name space but no access to instance variables.
Don't bring the jungle with the gorilla
If a function operates on a date, it shouldn't require a huge business object as an argument.
Brief where possible, comprehensible always
Brevity is good, but comprehensibility is paramount. Decide where you draw the line and guard it.
Short but readable methods and functions
Shorter methods and functions are usually better, but again, readability and straightforwardness shouldn't be sacrificed.
Comment code as necessary
Each public class and function should have documentation telling what it does if it isn't obvious. Hairy bits of internal logic should have comments too.
No useless debug noise
Remove all those NSLogs that aren't necessary anymore. If they are necessary, make sure they're understandable.
Good error logging
Error logging should describe where the error happened and what the problem was. It should be concise, precise, and helpful.