iOS Code Review - Health check of your Swift code

I have been recently asked to review an iOS application to see how healthy was the code base, if it follows the best practices and how easy it would be to add new features to it. If I review some code on daily basis for small pull requests, analyzing one whole app at once is quite different exercise. Here is some guidelines to help doing that analysis.

Quick look

Let’s start with a quick look to get a global overview of the project. Like a funnel, it’s easier to start at the higher level and slowly go down through each layers.

From the git repository, is there any documentation or guideline how to install, setup or build the project? Is there any Xcode or Swift version requirements? What about dependencies? That can give you the first tips how accessible the project is to new iOS developers.

Is there any external dependencies? How many of them? Are they integrated from Carthage, Cocoapods, Git submodules or others? There is nothing wrong with having 3rd part libraries included, but it’s also can expose your application at risk to some extend. It’s good to check if they are up to date and if not, how far they are from latest one.

Checking the git history gives also a good hint if there is any git flow in the process. For instance, pushing any commits to master branch is against best practice. Having a flow with review can help reduce code issues, even for a solo developer.

Once you’ve got your first impression regarding git flow, dependencies and usage, it’s time to dive further in the code and see what’s behind.

Diving in the code

When it comes to the code analysis, it back be quite tricky to know where to start. I would check first the project structure, files and folders, to give hints of code structure and architectures.

But checking how healthy and clean is the code is very broad. It goe from the naming convention, coding style, memory management, user interface, to code architecture or even design pattern used. Let’s break it down between each layers.

Codebase

Is the codebase written in Swift, Objective-C or both? The goal is to verify the codebase takes advantage of latest syntax and best practices for each of them. That includes memory management, property declaration, etc.
Which iOS version is targeted? Is there any deprecated method used? Every year, Apple add some new apis and deprecate others. Keeping a clean code go through fixing any warnings Xcode raises.
How long does it take to build the application? This is very handy to know where the issues relies. It might because of over-complex expression or chained dependencies to build. Xcode 10 includes new flags to display it like -showBuildTimingSummary.
Is there any memory leak? Using Xcode Instrument is an easy way to make sure there is no flaws regarding memory management.

Data layer

Does the application use a data storage system like CoreData? Is the database encrypted? If using CoreData, let’s make sure to check about multi-threading usage which can cause issues in the app.
Is there any use of UserDefaults? If yes, make sure there is no sensitive data stored like credentials which would be security vulnerability.

Network layer

Does the app use external library for network? If yes, is it base on Apple best practice and not deprecated methods?
Do all request run async? Are they all using https://?

UI layer

Is the UI implemented with storyboards, xib files or programmatically? There is no bad answer but some can affect more the evolution of the project.
Does the design take advantage of auto-layout? What about trait variation? With more devices and resolutions, code like if IS_IPAD { } aren’t maintainable long run.
How are integrated the assets? Using scalable assets will avoid to manage different resolutions (@2x, @3x).

Design patterns

What kind of design patterns are used in the app? Delegation, Closures, KVO? Depending of each usage, it helps a lot to anticipate other flaws. For instance, let’s make sure any delegates don’t create a retain cycle.
How about other practices like protocol oriented programing or dependency injection? You can quickly identify how easy it to maintain and test an element base on those parameters.
Are the 3rd party services implemented with some abstraction? It might sound overkilled at first, but the day you want to swap between two push notification providers or tracking system, you’ll be glad to have flagged the risk way earlier.

Testability

Is there any unit tests or ui tests in the project? If yes, let’s quickly run them to make sure they are up to date.
How testable is the code? Strong dependencies between elements can make it really hard to test them. For instance if the network layer is tied up with the parsing layer, how to make sure we can just test parsing elements? It falls back to protocol oriented programming and dependency injection too.
No unit tests at all? Missing tests in the project can be flagged as critical, especially in regards on app evolution: there is no way but manual tests to know if something is broken / working.

Script Automation

Is the project setup for CI/CD? If yes, that let you know the app is built often and can be monitored (if not yet) with small effort.
Is there any other automation tools? What about fastlane? That’s another way to know how scalable is the development process at the moment. It’s also important to make sure those tools are up to date.

There are probably more elements to think of I didn’t list but I think it’s a good base to start if you are asked to do this health check of an application tomorrow. Of course, it will depend of what you’ve been asked: looking at the scalability of a project is different that looking for security vulnerabilities in it.

Keep in mind code review isn’t about what you would have done instead but if it follows best practices. For instance, if your are a big fan of VIPER and the code base is based on MVVM, it doesn’t mean all should go away. Let’s not fall into the trap of “which architecture is best”.

Finally, last thing is stay humble when going through somebody’s else code. Every decisions made are based on time and resource constraints. It’s too easy to come months or years later and be harsh with past developers.

Thanks for reading.