Unrecognized Escape Sequence

There’s a decent case – for when working on almost any software project – that you should be doing code reviews for everything that is intended to make it into your main branch. Even if you are working alone, I would recommend making a review process part of your workflow, if only to separate the mental process of writing code from reviewing it.

In larger projects with more developers, yes, automated tests, linters and continuous integration (CI) systems are essential, but I would argue equally so is the quality control gate that is a decent review system. There is no automated system in the world that can confirm that code is genuninely human readable, that the design chosen by a developer makes sense in a bigger picture, that a piece of work is moving a project in the right direction or the wrong one. A good review process not only attempts to achieve these lofty goals, but also is critical for maintaining joint understanding and ownership of a project among the team working on it.

The standard tool for performing code reviews is the Pull Request or Merge Request depending on where your code is kept (from hereon “PR”). But what makes for a good PR? There are lots of opinions on this and each team will likely have their own stuff that makes sense. Over the years I’ve found the following general rules to be useful.

Writing up the PR

Have a template, and keep it evolving

Tools like Github and Gitlab have the facility to write a template for PRs, which are invaluable. Typically these are versioned in the repository alongside the code so it’s easy for the team to update them over time. Everything that should be included in a PR can be written as reminders in the template.

Having a checklist of reminders and steps required for getting the PR reviewed/merged is hugely valuable. For example this might include:

• All required fields in the template have been filled in
• The branch has been rebased on top of your main branch
• Appropriate documentation has been added (e.g. in-code documentation, readme files)
• Changelogs have been updated
• Appropriate automated tests have been added (plus pet-peeve, automated tests aren’t filling the log with warning and error messages)
• Style/formatting guidelines have been followed (those that aren’t checked automatically by CI or other means)
• No TODOs or FIXMEs have been left in the code (they have been made into issues etc.)
• Appropriate logging has been added (so often forgotten!)
• Any new third-party software has been vetted
• Legacy code that has been changed has been refactored to meet new standards

Write a decent title

Summarise what has changed. In a large codebase, it can be useful to prefix the title with the area/section/feature that has been affected to help categorise the PR at a glance.

Spend some time writing a detailed description of what has been changed

What did you change? Why? Be aware often the reviewer will be less familiar with the motivations of the changes and the area of code being changed. Think about how long it takes to understand what change has been made by a bunch of source code changes by looking at them alone. Even one sentence of introduction can make a huge difference.

Including things like a list of steps to take in order to view or test the code changes can be very helpful. If any complicated or unusual algorithms, structures, tools, etc. have been used, or if a convention in the code has been broken (presumably for a good reason), further explanation of this might be warranted.

Give some context, and flag further work that will be required

If this PR is part of a sequence of changes, consider listing previous PRs to give context, and suggest what the subject of the next ones might be.

Add screenshots of the changes (where appropriate), or other artifacts that might be useful for review

A picture paints a thousand words, right? Well a screenshot can save…. a thousand lines of code review? Actually that doesn’t sound too far off. Isn’t it reassuring when someone demonstrates that they’ve done the due dilligence and actually tested their code changes!?

The code itself

Be your own reviewer

If you want someone else to review your code, give them the respect of reviewing it first and picking up little things. A PR is a great way to put on your reviewer hat and look at your code changes with a fresh perspective. I know I find a lot of missing details in my code by taking this approach (although I also recommend using a local diff tool such as Meld to do this for the same reason).

Also, adding comments next to lines of the code isn’t just for reviewers! If (when?) there’s something weird in the code, give your reviewers a heads up by calling it out, and explaining why.

Limit scope to one thing, and keep the scale manageable

A PR should ideally be a single, discrete piece of work. Reviewing multiple pieces of work in the same package is hard and messy, and if one part of the PR isn’t up to scratch, the whole thing should be rejected. Likewise, PRs that are too big are equally hard to review. If big code changes are required, you’ll need to find a way to break them across multiple PRs.

Often the reason why PRs get big or have multiple pieces of work in them are that developers feel pressured to do so because of bottlenecks getting work completed. If your process makes you feel like you need to cram unrelated refactorings and changes into an PR that has functional changes too, address these problems in the process!

Get your story in order

Remember that the history of your code is almost as important as the code itself. Review your commits before sending your PR for review and rebase to squash those that aren’t required and to reword your commit messages. Ideally the commits should tell a story of how the relevant work was done, with each commit representing working code (and passing CI!).

Get your code up to date

Remember that code can be broken by merging the deployment branch into it, so this should be done before review takes place. My preference is to interactively rebase my branch on top of the latest tip of the main branch as this gives the opportunity to see the other changes coming in, and to review my commits at the same time. Don’t forget to run all unit tests after merging/rebasing!