Peer reviews - what good looks like

These days there are any number of tools to analyse code for you, and in some cases even write it for you. If all you care about is reducing cognitive complexity, avoiding potential null reference exceptions, and ensuring you have 80% code coverage from unit tests (I’m simplifying the job of the code analyser considerably here, but my point remains) then the peer review process isn’t going to be the focus.

There are a bunch of good books about software development that you could read to start writing better code, Clean Code by Robert C. Martin (Uncle Bob) springs to mind. It was first published in 2008 but it is well written, thorough, and still current. It is missing topics like managing asynchronous code, and some languages have moved on so it isn’t relevant for every aspect of every language, but it is general enough to provide the reader with principles that could/should be followed to write good, clean code. Code should be written for humans to be able to read and understand easily, the peer review process allows a person to make a judgement on whether they think the code has met this basic requirement.

When I am reviewing someone else’s code there are several things I’m looking for. I don’t have a checklist of things that I check for each time, I have just developed a nose for identifying code smells. This could be a case of a bloated method or class, missing error handling, or violation of one of the SOLID principles. What I’m really hoping to achieve is to make sure that any code which is committed to the code base is ‘production ready’.

The key aims of a peer review are to make sure any new code meets the projects standards (if they exist), my own internal standards of writing good code or a combination of both, all in the aim of maintainable and extensible code. Here is a list of high-level rules that you should follow if reviewing someone else’s work.

1.     Does this pull request tell me what I should check for?

2.     Do I understand what the code is doing?

3.     Does the code do what it is intended to do? This should align with the user story/requirement?

Let’s break down those 3 questions:

1.     Does this pull request tell me what I should check for?

The first thing I like to see is a good description, one which describes what the code change is meant to do, confirm how the code has been tested, include a screenshot (or two) if it would help the reviewer understand the change, add unit test evidence and results, and advise where the reviewer should start. This isn’t an exhaustive list, I’ve seen lots of good examples of pull request templates, the best ones provide the relevant information to the reviewer in a consistent way.

As an aside, DevOps and Github, and probably many more PR applications support Markdown, so styling your PR with headings, checkbox lists etc. is easy and a nice way to quickly portray the information you need to a reviewer. You can even create a default template for new pull requests. In DevOps this is done with a .azuredevops folder in the root of your project with a Markdown file called pull_request_template.md inside that folder.

2.     Do I understand what the code is doing?

I’ve worked with lots of different types of developer, the best ones are those who write code for humans. Not every developer is at the same stage in their career, so I ask myself, could an average developer understand what’s going on? This opinion will change throughout the career of the developer but should always be a consideration when reviewing code. Yes, LINQ Lambda expressions are awesome and can reduce the number of lines of code you have to write, but are they always more readable or easy to understand than if they were written in a way that a person can easily read them? This is the most common aspect that I find myself commenting on; it’s very hard to “mark your own homework” so getting a fresh perspective is always good. Making sure the code makes sense is important, it gives confidence that another developer can pick up where the last developer left off.

3.     Does the code do what it is intended to do? This should align with the user story/requirement?

If you’re going to do something, you might as well do it right. Take the time to review all the code, look for something as simple as a spelling mistake in a comment (a personal bug bear of mine), as common as any opportunity to refactor something to increase readability or maintainability, as well as spotting a developer using an insecure encryption algorithm. Compare what you’ve read to the intended change, this is hopefully covered by dev testing and QA but it’s helpful to make sure that the requirement is met.

If I’m putting my approval on someone else’s work, I’d like them to have some level of confidence that they haven’t missed something obvious. This is a good reason to promote regular commits and pull requests too! Seeing a pull request with 100+ files changes is going to take a large investment from anyone reviewing code, but still, be thorough.

4.     Be kind/have some standards

Granted, this one doesn’t feature on my top 3 list but when you first start getting involved in a peer review process, it’s easy to take review comments negatively. Writing code is subjective so it’s not always a case of someone being ‘right’ but rather an opinion. To remove opinion, I always recommend witing and maintaining coding standards, either for a project or ideally the organisation. Standards are a way to make the review more formal and to lessen friction within a development team.

Try to make comments helpful and constructive, if a comment needs to be made on code, suggest a better way.

If someone finds a bug in some code, it doesn’t have to be seen negatively, it’s an opportunity to learn from a mistake and a good way to increase each developers own code writing competency.