A guide to code review

Put simply by really getting to know your team mates you’ll have a much better understanding of what their strengths and weaknesses are.

This will do three things:Give you an idea of what sort of mistakes, anti-patterns or oversights they are prone to making.

Allow you to effectively understand the deeper reasons as to why they do this.

Inform you about when you need to give a more detailed response in order to help them in these areas.

I’ll use myself as an example.

Before I joined my current company, Birdie, I had never done any professional backend work.

More specifically my knowledge of SQL was pretty poor.

Because we encourage 1–1 conversations with peers, other engineers got to understand my background but also my aspiration to improve in this area.

This meant that when I submitted a PR for review that involved querying the DB they would not simply right something like “Use a join” or “This should be indexed” but rather would take the time to explain their request.

This helped me to improve faster which then meant that code reviews involving queries included less mistakes and oversights.

Don’t just learn about your team mates from their work.

Talk to them, find out where they are now and where they want to be.

It will help both of you in the long run.

Ask questionsThere can come a certain fatigue with receiving change requests.

The phrase “I just want to get this merged” is not uncommon in engineering teams.

However, the moment we see change requests as just a barrier between us and merging work we have a problem.

The problem being that we have turned the review from a discussion into a dictation.

It does not matter how senior a developer the request has been made by is, you should question everything.

Of course I don’t mean that you should literally comment questions under every request.

What I mean is that you should take nothing for granted.

Whenever a request is made ask yourself the following about it:Is this providing value to the userIs this providing value to the developerIf you either cannot answer this yourself or disagree that a request provides values for these two types of people then you need to discuss it in the PR.

If somebody has requested a refactor you need to take its context into account.

Does the refactor make the code more readable?.(Providing value to the developer).

Does the refactor make a noticeable difference to the users experience or will it help the functionality scale?.(Providing value to the user).

If any of these come into question it is imperative that you discuss it.

Not all refactors are worth the time they take to change.

As an engineering team it is vital that you value your time within the context of the business and its users.

Developer time is valuable and if it is being wasted doing work that does not provide tangible value then you have a problem.

You also need to be wary that the person making the request has less context than you about this piece of work.

Its your responsibility to sensibly challenge requests internally (and then externally if necessary).

This will prevent you from wasting time making unhelpful (or even erroneous) changes.

Take nothing for granted.

Asking questions also works the other way too.

When reviewing someone’s work don’t be afraid to ask them anything you don’t understand.

It’s also a great mechanism for sparking meaningful discussion in PRs.

Making direct requests can often beget direct changes without any discussion, particularly amongst more junior developers.

Instead ask questions about why they implemented their solutions.

This will require developers to scrutinise their own reasoning and will either lead to them having an improved understanding of what they could of done better or greater confidence in explaining the benefits of their solution.

Provide contextAs someone who is submitting changes to a codebase it is your responsibility to provide as much context as to what these changes are and why they are being made.

Pull requests, especially within the context of a large codebase, can sometimes feel like someone asking to include a random chapter into a book that you may not have entirely read.

You need to understand that not everyone is on the same page as you once you’ve submitted some work.

At Birdie, we’ve resolved this by requiring a structured description for any PR.

It includes the following questions that must be answered:What does this PR do?Please provide a screenshot / screencast of it’s functionality (Not always applicable)How can it be tested manuallyDoes it have testsThese four questions help to give the reviewer a good starting point from which they can begin to make an assessment of the changes.

As well as this, I’ve found there is nothing better than just going over to the engineer reviewing your code and explaining in person what you’ve done and why.

Just remember not to hang over their shoulder after explaining it.

People need time to review and shouldn’t feel rushed or like they’re being watched.

See the bigger pictureSomething I was definitely guilty of as a reviewer was scrutinising PR’s for minor changes.

One that rustled a few jimmies in the development team was asking for the following change in a unit test:expect(array.

length).

toBe(0)toexpect(array).

toHaveLength(0)The benefit between having one or the other is minuscule and the time spent making the change surpasses its value.

Other things not to waste your time making requests for are style changes.

For example little things like comma dangle, indentation etc.

There are libraries such as prettier for this so don’t waste time discussing it in a PR.

This goes back to what I have mentioned earlier about changes providing tangible value.

Whats more important is asking yourself how does this piece of work fit into the codebase?.You need to understand it within the context of extension, adaption and integration.

Is this code extending an existing module?.Is it adapting it?.Or perhaps is it a new module integrating with an existing one?.Does this make the module easier to extend or adapt in the future or is it more tightly coupling it with something else.

Is this new module easy to extend or integrate with.

Is there a better way of doing this?This is possibly what I have found the largest hurdle to jump when it comes to code review.

I’ve tried a few things to improve this from flow charts to hexagonal diagrams.

In all honesty though, the best way to begin to understand the context of the change and its implications is to just check out the branch and try the code out for yourself.

It will take a bit longer but the more you do it the greater your understanding of what to look for will be.

Set up some breakpoints and some logging and really get a good idea of how this change is interacting with the existing code.

Do this enough times and you’ll begin to gain more of an instinct for it.

Getting this one right is imperative to effectively reviewing code so practice it!ConclusionCode reviews are tough because they require a variety of skills that can often extend out of what we initially consider as part of being a developer.

There are emotional, interpersonal and organisational requirements as well as the expected analytical and architectural.

It is by focusing on the 5 mentioned points that I have managed to drag myself out of the mire that is a poor code review experience.

Of course there is always room for improvement but by learning to separate your ego from your work, know your colleagues more personally, question everything, provide context and see the bigger picture you will begin to witness real progress in you and your team’s code reviews.

The only other thing to say is that like any other skill you won’t improve without lots of practice.

Throw yourself at code reviews.

Try and do as many as you feel comfortable handling.

If your engineering team is too small and there is not enough to review then try getting involved in an open source project.

The more you review the quicker you’ll improve.

.

. More details

Leave a Reply