A guide to code review
Code reviews and myself have had a troubled past. Consistently taking feedback in the form of change requests whilst also being able to analyse an alien piece of work proved to be a real challenge. My first job, rather questionably, didn’t require them and so they came as quite a shock to me. Luckily the recent years have provided me with myriad works of code to review, as well as some very talented colleagues who’ve guided me along the way. In this article I would like to pass on what I’ve learnt about both giving and receiving code reviews.
Its not personal
Letting others review your code can often be a contentious part of being a programmer. There is an inherent drive in most of us to believe that our work is good (or at least not bad). However, when a submitted piece of work goes up for review and is immediately Tommy gunned with change requests it can be hard not to take a hit to the ego. This is just a reflexive echo of our social heritage as human beings. Criticism is often in direct violation of our natural compulsion to be a cohesive and collaborative species. This applies for both giving and receiving criticism. We are initially deterred from criticising for the same peace keeping reasons that we find receiving criticism disharmonious to our daily interactions with others.
The great irony of this whole debacle, however, is that by trying to avoid tearing the group’s social fabric we are in fact disabling the team. We are halting its progress. Members of a group that cannot be honest with one another as well as positively onboard sed honesty represent a satisfaction with the status quo. Improvement requires challenge and it is our responsibility as a group / team to consistently and persistently challenge ourselves and each other.
Learning to separate your ego from your work is the absolute first lesson you need to learn in reviewing code. Failing to do this will only prevent you from understanding one of the core concepts of being in an engineering team, there is no “my” code or “your” code. All code is the responsibility of the team. Only the team can claim credit and only the team can accept fault. Once you have learned this you can begin to make real progress with code reviews.
Get to know your team
In order for our feedback to reach people we have to demonstrate that one of the reasons we are giving it is because we care about their progress. We can only do this by understanding where they want to progress to. Who is this person? Where have they come from? And where would they like to go? These are all questions we need to be able to answer before we can effectively challenge someone.
But how does this relate to code reviews? 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 questions
There 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 user
- Is this providing value to the developer
If 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 context
As 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 manually
- Does it have tests
These 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 picture
Something 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)to
expect(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!
Conclusion
Code 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.
We’re hiring for a variety of roles within engineering. Come check them out here:
