The Craftsmanship of Code Reviews

7 min readOct 22, 2020

Performing code reviews and giving feedback to fellow developer's work is among the most important things you can do. Feedback can provide improved quality to a codebase in the form of

Readability
Maintainability
Consistency
Limit risk for mistakes and errors

and much more. Not only can feedback have a positive impact on quality, but reviewing is also a great way of learning from others and sharing knowledge.

The common assumption for these benefits is that we as developers can provide meaningful reviews and feedback to each other. So, how might we do that?

Working as a developer full-time for a little bit more than one year I’ve tried breaking down my approach to reviewing and giving feedback. The tips presented below are a summary of that experience combined with other developers' tips(see reference list). You might also find that parts of the post could be used outside the context of code reviews.

This post intention is to help developers to learn or improve their craftmanship of reviewing and giving feedback. A pre-requisite for the post is to have a basic understanding of a pull request and a git diff.

Ground rules for reviewing and giving feedback

Photo by Tingey Injury Law Firm on Unsplash

Before asking for a review or taking part in reviewing I have a set of ground rules or call them assumptions if you like. Following these has been beneficial for both authors and reviewers in my experience.

Be a humble reviewer! Hostile feedback will only result in a risk of getting your point across and who likes a Besser Wisser? Be nice, it doesn’t cost a thing and fosters a better culture.
Opinions and thoughts around a pull request (PR) are great but don’t expect the author of the PR to put in place all your suggestions. Like driving, there are quite a lot of ways to reach the end destination. Discuss trade-offs instead.
Always assume good intent, this goes both ways. Misinterpretation in communication is common and even more so when the medium is text.
The reviewer and the author are one team. When suggesting changes refer to them in the terms of “we” and in the form of a question.
Communicate the importance level of the feedback you are giving. If something is a nitpick, communicate it somehow. A personal favourite is 🏄‍♀ / 🏄 emoji. I mean, who doesn’t like emojis?
If there are issues with expressing or agreeing around something in a PR, set up an audio/video call might be a preferred option.
Always give one positive feedback remark.
More senior developers than yourself also make mistakes, don’t assume they know more. Think critically and ask questions.
Always approach a PR with the mindset that you can learn.
The reviewer’s responsibility is to do due diligence before approving a PR. What is due diligence then? Happy you asked that is what the next section!

How to review

Below you can find a list of things that I try to go through for the PRs I’m reviewing. I’ve divided the list into two phases of a review that I try to go through starting from broad to narrow. I’m not telling you that all this should be done and in this order but rather how I approach it. I would also like to note that the list below is things you could look at, not must look at.

Step 1: The broad strokes

Photo by RhondaK Native Florida Folk Artist on Unsplash

Is there a description of what and why this PR exists? What is it that we are trying to solve? If not, you have every right to ask for clarification.
Do I understand what the code does? Is the intention clear by only looking at the code? If not, there are probably other people who won’t either, ask questions!
Can the changes be broken down to help with reviewing the change? No one likes reviewing hundreds of lines changing and it also increases the risk for mistakes. Does the PR contain any scope creep that is bloating the PR that could be excluded?
Do I understand the consequences of this PR good enough? If not, are there any questions that I can ask the author to understand more? If you still don’t understand enough? Great! There might be a chance to learn something by reviewing it together with someone that has a better understanding.
Is the naming clear? Methods, variables, classes, etc.
Are there todos or commented-out code that should be removed?
Do the comments added express “what”? See if we can refactor the related code to improve readability and remove the comment. “Why” comments can often help future developers in reasoning about how they might or might not change the code.
Are the methods long? Is splitting the methods up an option?
Is there any obvious duplication? Refactor!
Are there tests for the change? There might already be tests present that covers the change. If not, how can we know that it works? Short answer, you can’t. At least not according to Robert C. Martin the author of the book (among others) “Clean Code”. In his book “The Clean Coder” he phrases the rhetorical question “How can you know all your code works if you don’t test it every time you make a change?”.
Does the code adhere to the style we’ve agreed on? Does the code follow the style of what has been done in the codebase before?

Step 2: A closer look

Besides looking at the diff that is provided for a PR in webpages like Github, I often take the opportunity to review the change in the codebase itself. After all, the codebase is how you will look at it the next time you are working on it, so why not make sure it fits there? Looking at a PR-branch in on your own computer and having the diff on a second monitor can give another perspective.

Does the code do what the author states?
Is the PR taking a critical decision that we could postpone to later? The longer you wait for making a technical decision on implementation the more information you have. Robert C. Martin (“Uncle Bob”) explains this well in this blog post and his book Clean Architecture.
Is the solution building us into a corner? An example of when this might be the case is when we can’t extend on the PR addition/changes without modifying it.
Are proposed changes made in a suitable place? In the right abstraction? Does it feel shoved/forced in?
Are there any edge cases we have forgotten to consider?
Is the PR over-engineering for something that we could omit?
Is the change backwards and forward compatible?
Could there be any concurrency issues? Could there be any locking issues?
Do we have metrics to monitor the change? Maybe you are replacing an algorithm or swapping an endpoint? Do we have metrics to monitor that the change is working and performing as expected?
Is there a risk of rolling out the change? Could we minimize the impact? Feel free to ask the author how they are thinking about this topic.
Do we have appropriate logging in place? Would we be able to debug things related when it is live?
Do the dependencies we are adding make sense? Could we remove any? Perhaps it makes sense to add an interface between the dependencies and our code?
Are we imposing a security risk?
Is there a need for a walkthrough of the code? Sometimes even though the suggested addition/change is to be considered clean and can’t be broken down it could be beneficial to do a walkthrough.
Is there any less obvious duplication when looking closer? These duplications are easier to find by checking our the PR-branch on your own computer. Refactor!

Diving into reviewing and giving feedback can be difficult, it is certainly a craftmanship that takes practice and focus. Don’t be afraid to start giving feedback and looking at what other more senior engineers might give as feedback! Having a systematic approach is something that has helped me and which might also work for you. I hope this post has given you at least one thing that you can take with you for your next review!

For writing this post I’ve taken the help of other great posts on the subject that you can find in the reference section, make sure to check them out.

I would finally like to thank my awesome colleagues from which many of these tips come from. A big shout to Katarina Bergbom, Anish Chakraborty, and Simon Sundström for reviewing and giving me feedback on the post.

Thanks for reading!

Do you think I’ve missed something that we should add to the list?
Do you disagree with something?
Other feedback?
Feel free to contact me here or through LinkedIn.

References

Books that have helped me as a developer to give better reviews in one way or another

“Refactoring” by Martin Fowler.
“Clean code” by Robert C. Martin.
“The Clean Coder” by Robert C. Martin.
“Clean Architecture” by Robert C. Martin.
“Domain-Driven Design” by Eric Evans.
“The Pragmatic Programmer” by Andrew Hunt and David Thomas.