Simon Harriyott

Code Reviews

I started my current freelance role 6 years ago. I created an intranet, which I have been expanding and maintaining mostly by myself. Recently, a new senior developer has joined the team, as a permanent member of staff. During the induction process we discussed how we should work together, and decided early on that code reviews would be useful. They would enable me to check that he's using the existing (well, legacy to him) code in the right way, and secondly to ensure the code he produces is of suitable quality.

The notion of suitable quality is somewhat subjective, so we talked at length how we could make it more objective. There are horror stories about code reviews causing tensions between coders, and we wanted to avoid starring in our own. Having objective ways to assess the code under review would help.

We also agreed that the code was under review, not the coder, so any perceived criticism need not be taken personally.

The objective criteria we came up with are:

  • The code should compile and run
  • The unit tests should all pass
  • The code should implement the specification
  • The coding standards have been adhered to
  • The code is SOLID

Being objective, this allows us to write code in our own personal style; after all, there are many ways to solve the same problem. So long as the criteria are met, the code is acceptable.

I decided that my code should be reviewed too. For years I've been working mostly alone with no review process, and it would do the codebase and me some good. This meant a bit of source control reconfiguration; I used to code on the main github repo, but now I have a fork that I develop on, and submit pull requests to the main repo for review. The github pull request and code review process is really good (with the possible exception of a feisty red cross icon against any comment).

Well, so far it's going well. As we know our code will be reviewed, we run through the checklist before submitting the pull request, so the code is already improved before the review starts. There have been no major infractions: most of the requested changes have been typos, suggested renamings, the odd tabs / spaces inconsistency. There was one small but clear bug; I think a less than that should have been a greater than or something like that; a code review is a much cheaper place to find it than on the live site.

The best moments so far though, have been when noticing small code smells. This has led to a couple of good conversations discussing what the problem might be (they weren't immediately obvious), and suggesting possible refactorings. After a bit of recoding, the resubmitted code was leaner, more SOLID, more readable, and just plainly better. If it wasn't obvious beforehand, it certainly was afterwards.

In all, I'm really pleased with how the code reviews have been going. I feel more confident about the codebase, and my coding skills. I've learnt a couple of new things from reading every line of someone else's code too, which is a side-effect I hadn't anticipated.

3 January 2017