The Checklist of my Code Review Practice

As I wrote in my previous article, I truly believe code reviews are a very valuable practice. I started learning the works of code reviews from a very talented and strict developer. It was hard in the beginning but it really helped me improve in ways I did not think possible.

Since then, I am putting a lot of effort and time in doing proper reviews. My colleagues know that I am one of the most excited person when it comes to reviews and that I bring out every issue that might be improved, no matter how small.

During my experience, the same things seem to pop up over and over again, so I put together a list of potential improvements or issues to be constantly aware of and have in the back of my mind.

As a disclaimer, I am an iOS developer and most of the things I can think of are somehow related to the specifics of the platform.

But I also think there are some aspects that can be taken into consideration regardless of the development language (I would really like to hear your opinions about these, especially if you are not working as an iOS developer)

The code examples will be in Swift or Objective-C but I hope they are clear enough to be seen as language agnostic. So, let’s review some code :)

Language independent red flags

1. Dont repeat yourself — DRY

Take as example the following code

// Let's assume the array is not empty
if array.count == 1 {
   submitFormWithValues("Only one value selected", array)
} else {
   submitFormWithValues("\(array.count) values selected", array)
}

This must seems ok at first but if you take a closer look that method is being called 2 times, even though, the difference between the 2 calls is just one parameter.

If, for example, the method would be renamed or would have a third parameter added, you will have to change the code in 2 places, because there is duplication in your code.

This could be avoided by using the following:

let string = array.count == 1 ? "Only one value selected" : "\(array.count) values selected"
// Now we can call the method only once
submitFormWithValues(string, array)

In the above sample, in the case the method changes, we would just need to adapt the code in one place only - no duplication anymore.

There would be a lot of examples for this, but basically when you see the same things more than once in a method or a file, just ask yourself how could you remove the duplicated code by pulling out the variable part as reduced as possible.

2. Returning booleans

func isEmpty(array: [String]) -> Bool {
   if array.count == 0 {
       return true
   } else {
       return false
   }
}

Whenever you are returning boolean values, take a closer look at your code, because, most of the times, it can be written in a simpler way.

func isEmpty(array: [String]) -> Bool {
   return array.count == 0
}

The same applies when calculating boolean properties on objects. For example:

func hideHeaderIfEmptyTableView() {
   if datasource.count == 0 {
       header.hidden = true
   } else {
       header.hidden = false
   }
}

This is the exact same case, you have an if case where inside you set (or return) boolean values. You can just use those values directly.

func hideHeaderIfEmptyTableView() {
   header.hidden = (datasource.count == 0)
}

3. Reduce the level of indentation and use early exits

The more levels of indentation you have in your code, the harder it gets to be read and understood.

Our brains are capable of only holding a limited amount of conditions at once, so the more loops you have, the more effort it is required for your brain to make sense of it.

This sort of goes hand in hand with the early exit rule. Instead of having all your code wrapped up in an if loop(all code is indented), you can just add the condition for which the loop is not executed first and just return if that is the case.

So, this is rather straightforward. Instead of having:

func myMethod(elements: [String]) {
   if elements.count > 0 {
      for element in elements {
        // do some processing
      }
   }
}

You could use:

func myMethod(elements: [String]) {
   if elements.isEmpty {
      return
   }
   for element in elements {
      // do some processing
   }
}

I am aware this is also a matter of taste and style, but I consider it a lot easier to follow and the control flow is a bit more linear.

4. “Shy” code

As mentioned in The Pragmatic Programmer, your code should always be “shy code”.

Write everything with the smallest scope possible and only increase the scope if you really need to — for example, start with everything private, your properties as well as your methods.

Another way this rule applies is on the level of exposion of the properties which your objects own. Let’s say you have a custom UI component which contains an image inside, which should also be set from outside your component.

Instead of exposing the image as a read/write property, you could provide a method to update the corresponding property inside your class. The image will only be exposed as read-only and you can also have more control on the values that are being set on your property.

5. Remove empty methods and any unused generated code

The best code is the one that does not exist

Make a habit of deleting any (even IDE generated) code you don’t use. Dont just leave empty methods, unused variables, imports, outdated comments hanging around your code.

If you dont need it, just delete it.

Removing code should be one of your favorite activities as a developer. For sure, it is for me.

“Perfection is achieved not when there is nothing more to add, but when there is nothing left to take away” — Antoine de Saint-Exupery

The same goes for any commented code. Don’t just leave it hanging around there. You have anyway all the history in your source control tool, so just remove it. If you will ever need it again, you can just revert your file — even though from my experience, you rarely go back to code you are uncertain about(which might be the reason you decided to comment it in the first place).

Other things to consider

You should always bear this in mind when reviewing code:

• Hard-coded values: First you need to ask yourself, isn’t there any way you could remove the static value? Strings, floats, ints whatever it might be. If the answer is yes, then go ahead and make it better :) If there isn’t any way you can get rid of it, store it in a static variable(or whatever the format is in the language you are using).

The name will provide the value with some context, like defaultImageWidth or controllerIdentifier, making it easier to understand what it was supposed to mean whenever you come back to the code after some time. Another benefit would be that, from my experience, sometimes these values show up over and over again, in more places. Without having them stored in a variable, you would need to change the code in all places when your value changes from 3 to 4 — having again duplicated code.

• Team standards and guidelines — always make sure the standards are followed by everyone and don’t let things slip through the cracks of your code review. This should include consistent ways of naming variables, code formatting, best practices for your language agreed on in your team, taking care that the names, methods and decisions mean the same for everyone.
• Log your errors — maybe it is obvious, but I think it is worth mentioning, especially since you don’t see the value of this unless your code is already in production(at which time is kind of difficult to make changes, at least for mobile applications). Make sure every time your code reaches an invalid state or some properties are wrongly configured, even though you think some things are impossible, just log the error and put as much information as possible in the message. You wont believe how many times this will come in handy.

Objetive-c / swift specific observations

1. Make sure to deregister observers

Every time you are reviewing or writing code that registers for some sort of notification or subscribes as an observer for updates, make sure you check that they also unregister/unsubscribe. You will be surprised how many times you forget this aspect and we all know how difficult “Message sent to deallocated instance” is to debug :)

I can’t stress this enough, check check check.

2. Most specific method arguments

Take for example, button callbacks. Always write the method with the most specific type you know for your parameter and also always add the calling object.

This can help in 2 cases: once it is more clear what is happening and that only a button can call that method and in case you need to inspect the properties of that object you dont need to cast it or anything. You might not need it now, but always leave space for future development.

@IBAction func didClickContinue(sender: id) { 
    // in an year maybe you wont remember what sort of object is calling this method
}

You could use something more meaningful, that will require less time to figure out what the code is actually doing

// How about using this? You'll ever know what this method is used for and what the calling object can be
@IBAction func didClickContinueButton(sender: UIButton) {
}

About selectors:

// Always include the calling object in the callback
let tapGesture = UITapGestureRecognizer(target: self, action: “didTap:”)

3. Most generic object references

When you are using an object, save it to a variable that has the most generic type possible while allowing you to use the functionality you need.

For example:

private var viewController: AlertViewController?
init() {
   viewController = AlertViewController()
   navigationController?.pushViewController(viewController,     animated: true)
}

You might notice that the push method only needs a UIViewController not a AlertViewController. Therefore, we can save it in a property of the most generic type possible UIViewController.

// We dont need any particular properties of the AlertViewController so we can save it as a UIViewController
private var viewController: UIViewController?
init() {
   viewController = AlertViewController()
   navigationController?.pushViewController(viewController, animated: true)
}

The idea here is not to give information when it is not specifically asked for. The less the objects know about each other, the less dependencies you have in your code.

4. Documentation — Pragma marks

When a class becomes too large (sometimes it happens, no matter how great developers we are :D), it is useful to add some sort of delimitation for the public/private methods, the protocol implementations, public/private variables, anything that will help anyone that will have to understand and work with the code navigate easier through the class.

Other things to keep an eye for

• Retain cycles — always make sure the delegates are weak references. It is also good practice to capture self as weak inside blocks, to prevent retain cycles.
• Forced unwrap in swift — when you see a forced unwrap, always ask yourself, isnt there a better way to do this? Maybe a guard, optional chaining. Make sure you consider everything before you accept it as part of the code.
• Unnecessary code — As mentioned before, don’t leave any code around that you dont necessarily need. For example, implicit imports, self reference (when it is not mandatory, like in closures), braces for arguments in if and switch cases, type declaration for variables or properties which type can be determined automatically, blocks parameters you don’t use

Would be when you consider all the above:

One more thing..

A very important aspect is that there is nothing too small to comment on. You might be familiar with the Broken window theory. I totally believe that this applies to code and generally to software development. It’s awesomely explained in the The Pragmatic Programmer.

So, if you see something that does not seem completely right for you, just question it. You see an extra space somewhere or a typo, point it out. It will make a big difference in the long run.

It requires time and actual effort but it will always pay off in ways you never imagined.

Wrapping up

These are just some basic things I consider during code reviews that I learned during the past years, while working with very talented and passionate people. Of course, there are always logical and implementation details to be discussed which cannot be abstracted in such a list.

I try to keep my list always open and to be continuously adding things. Always be looking for the next challenge.

As the team gets used to reviews, the quality of the code will keep on improving and stuff from the list won’t appear that often anymore. That’s when you need to look deeper, maybe there is something that has now become clear after the clutter was removed.

You should always be asking yourself during the review: Is there any other way to make this code better?

I would love to hear what else you consider for reviews in your team. What is definitely on your list?

If you would like to get personal coaching for improving your mobile development skills and get input from me on your most pressing iOS or Android issues, I am currently open to coaching other devs. If you are interested, just get in contact . More details here: https://mobiledev.consulting/1x1-expert-coaching/

My goal is to be constantly learning and I believe there is no better way to gain deeper insights than teaching and sharing with others what you know. This is why I want to share my expertise to any mobile developer interested in learning more and getting stronger in their skills.

If you are interested in code reviews, you might want to check this also:

Why I 😍 Code Reviews

I hope you enjoyed reading this and that it was able to provide a little guidance in your review process.

Please, let me know how you found it, if you think you can also apply it to the programming language you are working with :)

Do you need help sharpening your mobile development skills? I want to share my experience and knowledge with people that want to learn more and get better — as well as learning as much as possible from you.

So, if you are interested in setting up a collaboration plan for your professional development, just drop me an email at catalina.turlea@gmail.com