Code Review 101

Part I — Myths

Before we get started, there are a number of myths about doing code review that need debunked.

Myth: Only senior developers can do code review

The best way to become a better developer is to write code. However, the second best way is to read other peoples code.

If you cannot read other peoples code effectively, then you miss out on learning how they solve problems and how they use the language or the framework. You also miss out on thinking critically about why they wrote the code that the wrote.

So by not letting everyone do code review, an organisation is preventing less experienced developers from improving. Which is counter productive to say the least.

However, even if you accept this point, you might say: “well less experienced devs won’t be able to understand my code, so they won’t be able to review it”.

If a professional programmer with more than a couple of months of commercial experience cannot understand the code that you have written, then you have probably written bad code.

Why? Because unless you are writing in assembler, then you are writing code whose purpose is to be read by humans as well as just by computers.

So where did you go wrong….

1. You might have failed to write relevant comments explaining what the code does.

Comments should not explain the obvious. Comments should address the messages sent and received.


# BAD:
#
# This is a method that returns the sum of 2 or more numbers
def sum( *args )
args
.flatten
.select { |a| a.is_a?( Numeric ) }
.map( &:to_f )
.reduce( :+ )
end
# GOOD:
#
# This is a method that returns the sum of its arguments
# as a Float. Arguments that are not Numerics or Arrays
# of Numerics will be ignored.
#
# e.g. you can sum a number and a range as follows:
#
# sum( 2, (1..5).to_a ) # returns 17.0
def sum( *args )
args
.flatten
.select { |a| a.is_a?( Numeric ) }
.map( &:to_f )
.reduce( :+ )
end

When you write a good comment you’ll often find that you end up revising the code it describes because writing a good comment forces you to understand the code that you have written.

In the contrived example above, having written the comment, I’d likely have the method try to convert everything to an array to save the user having to think about it when they passed in a range (as per the example), and then, of course, handle errors from inputs that cannot be successfully wrangled.

2. You might have failed to write tests.

If writing a good comment helps you to understand the code that you have written, then writing tests forces you to really understand it.

If you cannot write unit and/or integration tests to confirm that the code you have written works the way you intended it to then you need to fix this (and fast).

3. You might have failed to write a meaningful Pull Request description.

Assuming you are using something like Git flow or Github flow, the you’ll be using PRs to merge your work into a mainline branch.

What you write in the PR description is as important as anything else in the PR. Link to the task in your project management tool (it probably already has an integration with your code repo tool). Highlight any relevant comments that explain why you took certain decisions. Add more detail and clarity about why X, Y, Z exist or don’t exist.

Think of PR descriptions as a first person witness statement to the inception of the code contained in the PR. If you don’t write it down, you’ll forget it (even hours later).

If you write good comments, write comprehensive tests and then take the time to write a detailed PR description, then you’ll find it almost impossible to write bad code. You might still have made mistakes, but that is what reviewing pull requests is for — because we all make lots of mistakes… so relax and be happy that you’ve done your bit.

Myth: Only developers working on the same project as me can review my code.

Ok, so even the biggest sceptic might be willing to conceed that it is in the interests of the company to have everyone doing code review, and that, yes, good code can be reviewed by any professional developer.

However, a final objection might be that less experienced developers or developers not working on the project won’t understand the ‘domain’.

Again this is almost certainly false.

There is very little that goes on in the average company that someone who is smart enough to write code for a living cannot also understand.

Understanding the other projects going on in your team is why you pay attention during standups. Done badly, a standup is a grim prisoners’ roll call, where each condemned soul says a few words before bowing her or his head and praying for it to be over.

Done well, standups are interesting, because you get to find out what else is happening. You get to listen to people describe the stuff they are working on. If someone fails to explain this properly, ask them some questions.

It is another symptom of a bad standup if only the person leading the standup speaks. Good standups tend to require a ring master not an interrogator. Someone to know when to move the discussion on for the sake of time (and when to schedule follow-ups to let people continue the debate). Writing this has highlighted that I need to write a similar guide for how standups work, so I’ll park this for now.

If someone is reviewing your pull request and they don’t understand the domain, then point them in direction of the following things:

  • The README. Your project has one of these doesn’t it? The README contains information on how to build, deploy and contribute to a code base, but it also contains a couple of paragraphs explaining what the code base is for. If it doesn’t, write the damn README you’d want to read.
  • The ticket/card you’ve been working on. And since you’ve included a link to this in the PR description, you’ve already done this bit.
  • The project manager. Don’t waste your time explaining the background to someone when there is a perfectly sentient project manager to do this for you.

If you are lucky, you’ll be working alongside good Project Managers who are friendly and knowledgable and good Delivery Managers who are experienced and wise.

Project Managers and Delivery Managers are resources for you to make use of, not to take instruction from.

Yes they have other responsibilities, but as they are not writing code, so they are less important to the success of the project, and so their time is less valuable than yours. Be clear, I did not say their time was not valuable, I said it was less valuable. So don’t waste their time either, but equally don’t be afraid to use it.

So there you go. Any professional developer can review your PR (provided the company lets them) if you write good code and if any questions about the domain (which are not answered by the code, the tests or the task and/or PR descriptions) are handled by professionals who are really good at explaining this sort of thing to developers.

Myth: I can review my own code, so I can close Pull Requests myself.

This is not only false, it is dangerous.

Personally I have little time for superheroes. I do not value developers who save the day. Or at least I don’t most of the time — occasionally, something so unexpected crops up that it could not have reasonably been avoided. But those are the exceptions that prove the rule.

For everything else, there was inevitably a bunch of failings that created the situation that caused the ‘superhero’ to ‘save the day’, and over the last 17 years, I found that the superhero was normally the person responsible for the failings to begin with.

Great developers work as part of great teams. Writing software is seldom a solo pursuit. As the famous open source saw goes “with enough eyeballs, all bugs are shallow”.

You have, at most, two eyeballs. So get more of them on your code. At least four. Occasionally, more.

Closing your own pull request makes an unambiguous statement that you wear your underpants over your trousers, and that is not a good look.

Part II — How to do code review

Sprints not marathons

It is hard to review code for more than 30 minutes. It is okay to review 200–300 lines of code, and not okay to review 500+ lines of code.

If code reviews for single pull requests are taking longer than 30 minutes, then your team has a problem with the size of their features.

If you find a pull request hard to code review for any reason, including length of time, then you should include this in your comments on the pull request.

Know what the goals are..

  • Code without defects.
  • Code that delivers the functionality it was written to deliver.
  • Code that adheres to any agreed style guides and/or standards that your team uses.
  • Code that does not need to be refactored to be built upon in future (but be careful not to try and predict the future, just look for sensible objects with clearly defined interfaces).

It can be useful to follow a checklist like this one (as much of this has been culled from the web over the years, I’ll try to go back and find sources to provide appropriate attributions):

General

  • Does the code work? Does it perform its intended function?
  • Is the code easily understood?
  • Does it conform to agreed coding conventions? (e.g. https://standardjs.com/rules.html & https://google.github.io/styleguide/htmlcssguide.html)
  • Is it DRY?
  • Is it as modular as possible?
  • Can any global variables be removed and replaced with something better (there is almost always something better)?
  • Is there any commented out code? (there should be none)
  • Do conditionals exist where they could be avoided (especially inside other conditionals)?
  • Are there hidden objects that could be surfaced (are there entire modules that could be surfaced, you’ll be amazed how scared some developers are when it comes to creating new text files or folders)?
  • Can any of the code be replaced with (well-tested) library functions?
  • Have you removed any logging or debugging code you wrote when developing the feature?
  • Does the PR have any unnecessary commits squashed?
  • Is the PR branch rebased against the mainline branch (e.g. it only contains ‘ahead’ commits not ‘behind’ ones)?

Security

  • Is the code suitably defensive?
  • Are all data inputs checked (for the correct type, length, format, and range) and sanitised if necessary?
  • Where third-party utilities are used, are any errors they raise being caught and then handled correctly (not caught and then thrown away)?
  • Are output values checked and encoded?
  • Are invalid parameter values handled?

Documentation

  • Do comments exist and describe the intent of the code?
  • Are all public / interface functions commented?
  • Is any unusual behaviour or edge-case handling described?
  • Is the use and function of third-party libraries documented?
  • Are data structures and units of measurement explained?
  • Is there any incomplete code? If so, should it be removed or flagged with a suitable marker like ‘TODO:’? (Note that I prefer to remove incomplete code rather than using a ‘TODO:’, but if you are using it, then make sure you following this style: https://google.github.io/styleguide/htmlcssguide.html#Action_Items)
  • Does the pull request have a well-written description that explains the changes made?
  • Does the repo have a README that explains the purpose of the code base and how to deploy it to your dev environment, run the tests and contribute changes?

Testing

  • Is the code testable? i.e. it doesn’t bury dependencies or fails to provide enough instruction or, if appropriate, actual test data to enable state to be replicated?
  • Do tests exist and are they comprehensive? i.e. 100% code coverage (as a rule of thumb, but not an actual rule) for all models, services, composable features (e.g. directives, components, etc) and, if necessary, has a set of functional tests been written (clickable/tappable functionality is normally a good candidate for functional testing)?
  • Have you done cross-browser / device testing?
  • Do unit tests actually test that the code is performing the intended functionality?
  • Do unit tests that focus on testing failure outnumber those testing success at least 4:1?
  • Could the test code be refactored? Is it too fragile or does it require manual set-up to test? Is there any test code that could be replaced with the use of an existing API or a recording from a live API?

Tools for code review

There are plenty of tools for automating some of the code review process, which are useful on larger projects (especially where Continuous Integration / Delivery / Deployment is a goal). These typically focus on code style and best practices.

The only other tools you might need are:

  • your favourite editor
  • the correct development environment.

For some code reviews you can do it in the browser just by reading through the files changed by the pull request. For others you will need to:

  • pull the code to your laptop
  • install the deps using the details provided in the README
  • run the tests
  • run the system locally.

Consider it a major red flag if someone tells you that you can’t run their PR because of <some reason>. It is normally a bad sign.

Part 3 — Conclusion

So there you have it. Doing code review is for everyone.

Indeed, for me, it is an essential part of the daily activities of a well-functioning engineering team that, more than any other activity, makes for better software.

Of course, the truth is that even someone like me who is a fanatic, finds it a tedious task to do from time to time, so it is always worth reminding ourselves of why it is so important. I hope this article helps.