iOS Code Review: Loose Guidelines
03 Jul 2014(reposted from Giant Robots Smashing Into Other Giant Robots, where it originally appeared February 19, 2014)
From time to time I’ve been asked to do an independent code review, to determine the overall health of a client’s code base. Unlike a code walkthrough, where someone familiar with the code shows the main components, this is a code review where an outside expert examines the project, takes copious amounts of notes, and reports back either in written form or in a meeting with the team, depending on what the client wants. This is separate from doing testing or any other sort of QA on the applications themselves. The idea is that you might have an application that works great and passes all kinds of acceptance tests, but is built in a such a way that future maintenance and enhancements will be difficult and/or costly. If we can identify problem areas in an application’s source code, we can help set things on a better course. The sooner we can discover potential problems, the better. The experiences and guidelines described here are centered around iOS programming practices, but many of them apply to other sorts of projects as well.
The last time I did this, the client’s budget was limited, so there wasn’t time to do in-depth examination of every source code file in each of the client’s application. I had a lot of territory to cover, and not a whole lot of time. So, I decided to do it in two phases: First, I took an initial look at each project to establish a quick (if superficial) opinion of each project’s health. After that, I dove deeper into each project, paying extra attention given to the projects that set off the most warning flags during the initial phase. This procedure worked pretty well, since I was able to report back with an approximate health level for each application, plus a lot of specifics for those that seemed to be in worse shape than the others.
The set of guidelines I’m outlining here are the kind of thing that anyone can do on their own codebase as well. If you don’t understand what some of these points are all about, or why they’re worth thinking about when it comes to your own apps, this could be a good time to improve your own skills a bit by thinking about some of these topics and looking for relevant discussion and debate (for example, on the internet).
Phase One: A Quick Look
To get a feel for the overall health of each app, do the following things:
- Make sure all external resources required by the project (3rd-party code, etc) are fully contained within the app, or referenced through submodules, or (best of all) included via CocoaPods. If they’re not managed in some way (e.g. CocoaPods) see if there is any documentation describing how to obtain and update these resources.
- Open the project in Xcode and build it. Make sure the project builds cleanly (with no warnings, and hopefully errors).
- Perform a static analysis in Xcode to see if any other problems show up.
- Run oclint and see if this uncovers any other problems that Xcode’s static analysis didn’t reveal.
- Examine the project structure. Do the various source code files seem to be placed in a reasonable hierarchy? The larger the project is, the more important it becomes to impose some kind of structure, in order to help outsiders find their way around.
- Does the app have unit tests or integration tests? If so, run them and make sure they complete without any failures. Bonus points if tools are in use for measuring test coverage.
Doing all that should take several minutes for each app, regardless of the app’s size, unless you encounter major problems somewhere along the line. Finding things that are not to your liking on one or two of those points doesn’t necessarily mean that you’ve got a huge problem on your hands, but by considering your findings here you can start to get a sense of any project’s overall “smell”.
Phase Two: Diving Deep
After that, do a closer examination of each app, starting with the ones that set off the most warning flags in your head during the initial examination. You should look at every source code file (if you have the time to do so), reading through the code with all of these things in mind. You’ll probably want to take notes as you go along, when you find things that need improving.
Objective-C
- Are the latest Objective-C features from the past few years being used? This includes implicit accessor and instance variable synthesis, new syntactic shortcuts for creation of
NSNumber/NSArray/NSDictionary, new syntax for array and dictionary indexing, etc. - Are instance variables and properties used in a consistent way? Does the code use accessors where appropriate, keeping direct instance variable access limited to init and dealloc methods?
- Are properties declared with the correct storage semantics? (e.g. copy instead of strong for value types)
- Are good names used for classes, methods, and variables?
- Are there any classes that seem overly long? Maybe some functionality should be split into separate classes.
- Within each class, are there many methods that seem too long, or are things split up nicely? Objective-C code is, by necessity, longer than the corresponding code would be in a language like Ruby, but generally shorter is better. Anything longer than ten or fifteen lines might be worth refactoring, and anything longer than 30 or 40 lines is almost definitely in need of refactoring.
- Is the app compiled with ARC, MRR, or a mix? If not all ARC, why not?
Cocoa
- Does the app make good use of common Cocoa patterns, such as MVC, notifications, KVO, lazy-loading, etc? Are there any efforts underway to adopt patterns that aren’t backed by Apple, but are gaining steam in the iOS world, such as Reactive Cocoa and MVVM?
- Are there view-controllers that are overloaded with too much responsibility?
- If discrete sections of the app need to communicate with each other, how do they do so? There are multiple ways of accomplishing this (KVO, notifications, a big pile of global variables, etc), each with their own pros and cons.
Model Layer
- If the app is using Core Data, does the data model seem sufficiently normalized and sensible? Is the Core Data stack set up for the possibility of doing some work on a background thread? See Theo’s guide to core data concurrency for more on this.
- If not using Core Data, does the app store data using some other techniques, and if so, does it seem reasonable?
- At the other end of the spectrum, does the app skip model classes to a large extent, and just deal with things as dictionaries?
GUI
- Is the GUI created primarily using xibs, storyboards, or code?
- Is GUI layout done with constraints, the old springs’n’struts, or hard-coded frame layout?
- Does the running app have a reasonable look and feel?
Network Layer
- Is all networking done using asynchronous techniques, allowing the app to remain responsive?
- If a 3rd-party network framework is being used, is it a modern, supported framework, or something that’s become a dead end?
- If no 3rd-party network framework is in use, are Apple’s underlying classes being used in a good way? (There are plenty of ways to get this wrong).
- Does the app function in a fluid manner, without undesireable timeouts or obvious network lags affecting the user?
Other
- Is the app localized for multiple languages? If so, is this done using standard iOS localisation techniques?
- If there are tricky/difficult things happening in the code, are these documented?
This is a pretty hefty set of things to consider. Depending on the code base, you won’t necessarily be able to find a clear yes/no answer for all of these questions, and for certain types of apps, some of these points will be meaningless. Note that I’m not saying exactly what I think are the “right” answers for all of the questions listed here, although I certainly have my share of strong opinions about most of these (but those are topics for other blog posts). Even if you wouldn’t agree with my answers, these are probably all points that are worth thinking about and discussing with co-workers and other collaborators to figure out just what seems right for your projects.