Bitbucket is a code hosting site with unlimited public and private repositories. We're also free for small teams!

Close

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

Sorted imports

In a .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.

Tests

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?

Functional 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

Changelog updated

If you keep a changelog, every feature branch should include an update to it.

Cocoa coding

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:

  1. Direct call
  2. Delegate
  3. NSNotificationCenter
  4. Responder chain
  5. KVO
  6. Bindings

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.

Good -description methods

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?

NS_ENUM over 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.

Attributes: nonnull

If a function doesn't expect NULL, it should be marked with __attribute__((nonnull)).

Attributes: objc_requires_super

If a method must be called by subclasses when overriding, it should use __attribute__((objc_requires_super)).

Attributes: pure and const

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 __attribute__((const)).

Internationalization

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 dispatch_once?

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?

Good names

Does everything have a sensible name?

Assert everything

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.

Recent activity

Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.