Code Review on Hundreds of iOS Apps

I’ve been a freelance iOS developer since 2010, and during that time I’ve worked with 100+ clients on projects large and small, and I’ve evaluated even more for potential clients, friends, possible acquisition, etc.

As with any technology stack, there’s a wide range of technique, style, and talent in the iOS developer pool. This means that you never really know what you’re going to get 🙂 But I’ve gotten pretty comfortable learning how to dig into a new project and figure out what shape it’s in.

In this article, I’m going to walk you through some of the steps I use to evaluate the quality of an iOS app codebase. I won’t go through how to best refactor and clean up the code (that’s for a later article), but just to get a sense of what you’re dealing with (and maybe whether it’s worth dealing with at all).

To start, here’s a truth I’ve learned the hard way: many developers are just plain lazy when it comes to learning someone else’s code. We have a tendency to spend a few minutes poking around someone else’s mediocre code, not really understand what’s going on, declare the whole thing “terrible”, and start thinking about how much better and easier things would be to just rewrite it ourselves.

I used to be this way myself, but I’ve undertaken several rewrite-from-scratch projects over the course of my development career, and I would estimate that they tend to take 3-5x longer than you think they will. You should almost never, ever rewrite a code base.

I’m not saying that things shouldn’t change; a lot of code is terrible and definitely needs to go. But that doesn’t mean you should rewrite it all from scratch. I now heavily prioritize a refactor-as-you-go approach, which I find to be much more realistic, and puts business goals and priorities ahead of developer preference, which is always the right call when they disagree.

This guide is fairly technical in nature, and if you’re not at least moderately familiar with Xcode and Objective-C or Swift, you may have trouble using these steps very effectively. If that’s the case and you don’t already have a developer you trust, you may be able to hire me to do codebase review, cleanup, and ongoing maintenance.

OK, from here on in this article, I’ll assume that you’re an iOS developer and able to follow along

Get the code

The first thing I need to evaluate the code is…the code! This might seem obvious, but you’d be surprised how often I’m given the wrong codebase (the server code, for example), or an incomplete codebase (missing shared internal libraries). So the first thing I want to do is just look over what I have and make sure I have what appears to be a native iOS project, built in either Objective-C or Swift.

By the way, if you are looking at a hybrid app (Cordova, Appcelerator, etc), then feel free to read through this guide, but just be aware that much of this may not make sense for you. I wish I could help, I’ve just focused on native iOS development so that’s not my area of expertise.

Get it under version control

If the code was shared with you via Github, Bitbucket, or another source code repository service, then you’re good to go here. However, if they sent you a zip file and there’s no sign of version control anywhere, please put this under version control first. I’d recommend git, which is as simple as running these commands in the terminal:

$ git init
$ git add .
$ git commit -m ‘initial commit’

That’s it. Now you’re under version control and you can safely poke around and modify code to your heart’s content without worrying about messing anything up. We won’t be doing refactoring or anything in this article, just poking and prodding, but I still like the feeling of knowing everything is safe in version control, and if I do have to fix a few things (to get the project to build, for example), I want those changes safely stored in git.

Take a look at the file structure

Before I even open the project, I spend a few minutes looking at the file structure.

First of all, I want to see if there’s any sign of Cocoapods, Carthage, etc. I also want to see if they’ve just dumped a bunch of libraries, frameworks, or other files that are part of the project at the same level as the .xcodeproj file, instead of within the project directory. I just find this a bit sloppy.

Next, I open up the project directory. I’m really just looking to see how well things are organized here. The organization within an Xcode project doesn’t reflect the underlying file system, so it’s not uncommon to see a project folder with every single file in the project at the same level, not organized into folders, etc. While it may not seem like a big deal, I have found this to be a pretty decent predictor of whether the project overall will be sloppy.

If the project uses Cocoapods and there’s not a Pods directory and a .xcodeworkspace file that you can open, you probably need to run “pod install” from your command line to install any dependencies.

Open the project and try to build

Now we go back to the .xcodeproj or .xcodeworkspace file and open up in Xcode.

I then typically hit “Cmd + B” to build the project. I don’t usually run the project yet, because I don’t care about signing identities and provisioning profiles at this point, which are likely to throw errors. I just want to see if the code will even compile.

In the majority of cases, the code won’t compile on the first shot.

Review any errors during compiling

If the code won’t even compile, fixing that is my first priority. It could be a thousand different things, and walking you through all of the possibilities is outside of the scope of this guide, but in general, I’m looking for the following:

  • Is there code missing? See “Get the code” section above
  • Are there frameworks not linked properly?
  • Is something messed up with the Cocoapods setup (if there is one)?
  • Is the code just badly written and broken?

It depends on the project, but I generally don’t get codebases for review that are just completely broken to the point where they won’t even build without significant work. The exception would be if they’re very old, and are no longer compatible with Apple’s frameworks.

I can’t offer a ton of specific advice here, since each project varies so much, but it’s worth an hour or so of just fighting your way through any errors found while trying to compile and attempting to fix. Like everyone else, I typically Google the most relevant-looking portion of whatever error message I’m getting from the compiler, and go from there. If you can’t get the project to build, you can still do some of the steps below, but it won’t as effective.

Review any warnings

Once you get the code to build, check to see if there are any warnings from Xcode about your code. My personal preference is to see 0 warnings, but several isn’t the end of the world. Where I start to get nervous is if I see 127 warnings in a long list on the left side of Xcode.

Yes, they’re warnings, not errors, but in my experience, a very high volume of warnings should be a warning to you that you may have inherited a codebase that wasn’t very well taken care of.

However, you do want to distinguish between warnings in your project’s code and warnings from external library code. If you’re using Cocoapods and pulling in a dozen libraries, each of which averages 2-3 warnings, that’s several dozen warnings right there.

To that end, I typically just silence all compiler warnings in my pods by adding this to the top of my Podfile:

# ignore all warnings from all pods
inhibit_all_warnings!

Then re-run “pod install” so that Xcode hides all the warnings for your pods. The warnings that are left should be truly yours 🙂

Now, there are two main reasons for warnings: sloppy code or deprecations. The latter shouldn’t be cause for concern if the codebase is old; Apple changes their SDKs with every release of iOS, and they deprecate huge swaths of their libraries in the process. The same applies for many of the other libraries that your app may rely on. That doesn’t mean that you don’t need to allocate time to fix them, but it doesn’t necessarily imply a low quality codebase.

However, if you see a lot of warnings for “code will never be executed”, or “variable not used”, etc, then you might be a little wary that things are in worse shape.

Most of the time, I find that projects have a handful of warnings and issues for relatively minor things that can be tidied up in a few hours. This doesn’t always reflect major issues with the underlying code, but it can reflect some laziness or sloppiness on the part of the developers. Worse, it’s noise that masks other issues that may pop up later. If you have 97 issues in your code, you may not notice when you introduce #98.

Run the static analyzer for Objective-C

At this point, assuming the project will build, I also like to run the static analyzer if the codebase was written in Objective-C. To do so, select menu “Product -> Analyze” and then take a look at what it finds. Apple actually has a great little guide that covers how to do this here.

It might just be me, but I’ve noticed Xcode seeming to catch more errors that I used to have to catch with the static analyzer, so you might not get as much useful info from this as you used to. I still like to include it as an item in my checklist.

Note: As of this writing, Xcode doesn’t seem to do static analysis.

Use the app

Now that we have an app that builds, let’s fire it up. I usually try to run in the simulator first, just so I don’t have to hook up my phone. Of course, some apps rely heavily on device features that won’t run in the simulator, but I usually give it a try first.

Select the desired simulator and then run the program.

These days, most apps connect to one or more servers for their functionality. It goes without saying that if those servers aren’t available, your testing is going to be pretty limited. So the first thing I’m trying to do is figure out if the app can even work as intended. I’ll usually try and do something that requires a server connection, like register or signup. If I get an error message that indicates the server couldn’t be reached, or nothing happens at all, I generally want to try and figure out why.

The easiest thing to do is search the whole project for some text that’s only on the signup or login screen, like “login” or “forgot password”, etc. This isn’t full-proof, as sometimes images have been used (blech), but it’s a good start. From there, you can trace what happens when the login button is clicked, and follow that.

Another option is to search the project for “http” and see if you can find the configuration where the server API URL is specified, and then see where it’s used. I’ll typically throw in some breakpoints here and try to trace the program execution to see what the actual problem is.

If the app is connecting to the wrong server, and you have the correct URL, fix it and try again. If the server is missing or you don’t have it, you may not be able to actually play around with the app.

Assuming I can get the app working and connected to the server, I basically just play around and try to figure out how things work, what looks good, what seems to be broken, etc. I’ll jot down some notes about anything confusing, anything that seems broken, etc. Mainly just trying to get a sense of what the app looks like from a user’s perspective. I also want to understand what all the major screens are and how they fit together. What’s the home screen, if there is one? How do I get to the other areas of the app? Side menu, tab bar, navigation controller stack, some combination of the above, etc?

Review project organization

Once I’ve played with the app a little, assuming I’m able, I want to dig into the project organization. Meaning, how do they have the files laid out. Is everything in a single folder in the project? Do they have all the controllers together? Do they have a folder for models? Etc. I mainly just want to see how things were organized, named, and try to connect those things to what I saw while using the app. Just a very high level view, nothing fancy.

Review AppDelegate

There are about a million ways to architect an iOS app, but they all have one thing in common: AppDelegate. By default, there’s a class that has responsibility for being notified by iOS when the app has been started, restarted, sent to the background, etc. I like to start here, because this is where the core of the app is initialized and kicked off, and sometimes there’s a lot more here.

I mainly just want to see how things flow in the code from the point of iOS handing control to the app. I also want to see if the developer has thrown a bunch of stuff into AppDelegate as if it’s a “global” class.

One bad habit that a lot of iOS developers have (including yours truly in years past) is stuffing a lot of miscellaneous stuff into AppDelegate. It’s basically a globally available object that you can grab access to from anywhere, and so a lot of cruft tends to build up here. Depending on how much this is either a yellow flag or a red flag. If I see a half-dozen functions here for things like managing user state or something, it’s not the end of the world. If there are 1500 lines of code to manage every aspect of the application’s state, that’s a huge red flag and it’s going to take forever to untangle.

Another useful thing to look at in AppDelegate is integration with things like Facebook, Twitter, analytics packages, and other third party SDKs. They’re often initialized on app startup in AppDelegate, so you can get a good idea of what third-party SDKs the app uses by looking here.

Review controllers

From here, I like to dive into the actual view controllers. I typically just click through each one, getting a broad overview of what it does, how the view is setup (code, xib, or storyboard), how much code there is, etc. I’m looking for how disciplined the developer is, things like:

  • Do they keep all or almost all of their methods and properties private and not publicly declared unless necessary?
  • Do they use modern Swift or Objective-C syntax and conventions?
  • Are their methods well-named and well-organized?
  • Are the view controllers as short as possible?

Look for red flags

Too much code

A big red flag for me in low-quality code is massive classes. I’m talking multiple view controllers with several thousand lines of code each. I’ve seen this multiple times, and it usually reflects some real laziness around setting up custom classes, abstracting things out into proper patterns and protocols, etc.

Hard to follow

Well-written code should usually be relatively simple and easy to follow along to see what’s going on. Variables and methods should be named to clearly indicate their function.

Yes, you can figure almost any code out, but that’s not the point. The point is to avoid having to “figure it out” in the first place.

In areas where it might not be clear why something was done a certain way, you hopefully will see some comments explaining the decision. At the same time, code where there are more comments than code is also a red flag, as it can indicate that the code itself may not be as self-evident as you hope.

God objects

I mentioned this earlier, but AppDelegate (and some other classes) can tend to start to become “God objects”, basically globals that hold state and own objects for the entire app. Terrible practice, as it makes your code hard to read, hard to refactor, hard to debug, and hard to maintain. Unfortunately, if you find that this has been done, it can be difficult to unwind. I’ve worked on several large, complex apps where we gradually refactored out the “God objects” as we did larger-scale re-architecture work over a period of months.

Huge blocks of commented out code

I really don’t like scrolling through new projects and seeing large blocks of commented code that hasn’t been removed. I get it, you’re nervous about just tossing that code that you spent all that time writing. But that’s why we have version control! Now, I’m not above commenting out a block of code here or there for testing, or other temporary reasons. I may even check that into version control. But I try not to let that stuff build up over time. It just makes the project harder to read and understand, and it’s another sign of laziness.

Dead code

Dead code is code that’s still included in the project, but is never called. It can be tough to find without the static analyzer. The good news is that it’s typically easy to remove, as you just delete the files and remove all references to them if they’re truly not being used. Ditto for any libraries, submodules, pods, etc.

That’s just a partial list of the red flags. There are others, and if you’re a developer and you’ve spent time working with other people’s code, you probably have your own list. The only final thing I would urge is that you not confuse other people’s approach with a red flag. Yes, there are ways of doing things that are just completely wrong and cause no end of headaches down the road if not corrected. But there’s a very large set of styles and patterns that people use when programming that may not be ideal but are relatively harmless.

You can usually judge a book by its cover

You can tell a surprising amount about the deeper elegance of a codebase by looking at its surface beauty. The key thing I want to see when I review a codebase is a clear methodology that is consistently applied. If your code looks like crap on the surface, and is very inconsistent and sloppy visually, the problems probably go much deeper than that. Conversely, it’s rare to find code that looks beautiful that is horribly architected under the surface. Both anomalies happen, but are not common.

Conclusion

And that’s pretty much it! Depending on the project and the purpose of my code review, I may write up some notes if needed, including jotting down any classes or objects that are confusing or need more explanation. If I’m going to go deeper with my exploration, I might try setting some breakpoints to trace the program execution through any tricky sections of code. Regardless, none of this actually addresses or fixes the codebase issues; we’ll do that in another installment.

Again, if you have an existing app and you need help with any of the above, I can help!