Code reviews - many a new programmer will shudder at the thought of being given the task of reviewing a peer’s pull request (PR). Others will leap at the chance to improve their understanding of a problem. Some just enjoy the challenge of being able to find bugs or improvements, i.e. sort of another tool for learning, really…

At first, my thoughts, as well as the thoughts of other colleagues I had spoken to about this, were that everyone did their own thing when reviewing code. In actual fact, there is a distinct method (which I’ll share with you later) in all the madness that rings true for about 95% of the people I have worked with. Hence illuminating the fact that most of the preconceived ideas held by myself and my fellow engineers as to how others review code were totally wrong!

Luckily here at SharkTank we promote pair programming quite extensively and it’s definitely become part of the development culture. Two people, one mindset, switched on, working on a common problem or customer need. Think of it as a first class access pass into the inner workings of your pair’s analytical mind and the processes/paths they choose to use/follow when solving a problem or completing a task.

In effect, this gave me the opportunity to implicitly ask questions about how my “pair” reviews code when looking at pull requests. With each person having varying levels of skill as well as a different attitude towards reviewing code - this was going to be interesting! The idea was not to arouse suspicion. I wanted the truth, nothing but the truth. Whatever I heard or saw no matter how ugly - I wanted it all!

The (wo)man with a plan

After literally months of taking notes while ‘undercover’, I organised it all into a sort of structured list, ready for analysis. I have found that there is clearly a common set of steps taken by the majority - an existing “method in all this madness” if you will. I’ve extracted this ‘foundation’ of steps into a “start to finish” guide on how to effectively and confidently take on the challenge of reviewing your peer’s code. This is what it looks like:

1. Examine all notes and documentation for the PR.

• Read PR description and notes.
• Run through the commit messages. These should describe the path the original programmer took at a high level.

2. Spin up the app and use it.

• Simply put, you’re looking for obvious bugs, typos and an unsensible work flow.
• This also gives you an idea of what exactly the code is trying to achieve.

3. Quickly scan through the code looking for stylistic issues.

These are usually trivial things. Basically, these are the things missed because you’re working with the same code for long periods of time. Some examples are:

• Extra/pointless white space, including empty lines.
• Typos in comments, method names, classes, etc.
• Any sort of syntax or format use that breaks our coding guidelines.

Shark Tank’s Coding Guidelines consist of a set of rules that are mostly language and format specific. For example:

• Multi-line loops in Ruby should be expanded with a do and end rather than using braces. This improves readability and makes life easier for your fellow programmer.

4. Look at the tests

Tests form the living specification for a software project. Tests in our code base are usually broken up into behavioural sections and then grouped together in a general order of: context < method < class.

This gives the test suite an easy to read structure. Hence this is usually a great place to get a sense of what the solution is and what the new objects and methods are expected to accomplish.

• Read through the tests and get a basic understanding of what is trying to be achieved.
• Take a look at the level of stubbing and/or mocking.
• Ask whether the test is robust. Is it essentially a specification or is it just testing the inner workings of something? Testing inner workings(implementation) can sometimes make the test fragile when refactoring.
• Gauge the simplicity of the tests. Usually, but not always, complicated tests indicate that the solution may not be the optimal route and that there could be improvements to be made.
• Tests are worse than useless if they take too long to run. Checking the performance and approach to testing a particular method could also be something to look at.

5. Read through the code slowly in a detailed way.

At this point most programmers will start to ‘connect the dots’ so to speak on a high level. It would be relatively expensive to try and comprehend and guess why every line of code was written the way it was. Your job as a reviewer is to simply observe, understand and then critique the current piece of code you are reviewing based on your understanding of the solution at hand.

• Look out for performance issues.
• Minimum amount of DB queries that could possibly be made.
• If applicable - is the functionality thread safe.
• Look out for possible memory/resource leaks.
• Can the functionality be replaced with a standard library method?

While these steps seem to be pretty saturated with information and detail on what you should be looking at or for, remember that a lot of these points go hand in hand. For example: Looking for places in the code where fewer queries to the DB could be made. This can help with performance issues as well. You should also concentrate, in detail, on the tests, i.e. would stubbing be as effective as creating the full stack object to run a particular test?

6. Final usability look over.

• A quick check to see that all acceptance criteria has been met.

In short, what exactly are we looking for?

There are a lot of activities during a code review, but here is a list of the main things you are looking for:

• Code smells
• Code breaking the guideline specs
• Improvements - both performance and readability of code
• Unnecessary calls to the DB
• Missing or fragile tests
• Possible memory leaks
• Does the code impress upon the structure of the business rules (directly map to the business domain as much as possible)
• re-inventing the wheel syndrome

On a final note: I would caution against paying attention to who worked on the PR. It is better to go at it with an unbiased mind. Your review is about the code and nothing but the code, so help you Git (or whatever other version control system you so choose).

The above peer code review “guide” is what I would term as the basis for the majority of most review approaches. Although obviously I had a relatively small sample group for this “experiment”. Simply having more than 4 or 5 engineers with slightly different approaches, that have all the above steps in common, is more than just a coincidence. This is clearly a tried and tested core part of the review process that delivers results in the form eradicating bugs and merging the best quality code or as close as for many an engineer.

I focused this article on giving you a broader and more structured approach to achieving the best possible results when reviewing code. Of course we all have our style and preference in the way we explore problems and possible solutions. So tweaking the above approach to your own style and preference is only natural. Even if this article has only just got you thinking about how to be more effective in the peer code review process, then it has at least served a worthy purpose.