Code reviewing is an art. Although most companies conduct code reviews, only a few of them fully harness the potential benefits that code reviews offer. In this article, we will look at what you and your team can do to fully leverage the potential they offer.
In This Article
- Code Review Is a Team Activity
- Why To Do Code Reviews?
- Why You Should Not Do Code Reviews
- What To Do Before a Code Review
- What To Do During a Code Review
- What To Do After a Code Review
Code Review Is a Team Activity
First rule of code reviewing, code reviewing concerns the whole team. Every person is different, developers have different opinions about code and also different expectations of a code review. Critiques are also received differently from person to person, some are more sensitive than others.
To make sure the whole team agrees to how the code reviews should be handled, you can do one of two things. Either have a meeting about it to talk about expectations, or, make it less formal and start a discussion with the team when you do your first code review. If you bring it up when you present your first code review, you can be quite strict with your comments and still appear as a caring person.
When you discuss the code reviews, try to settle what you think is important. Should the code review be about finding bugs, or should you comment with improvements and opinions? If you encourage opinions, should those be mandatory to fix or can they be ignored?
Discuss and come up with an agreement every developers agrees with, but try to avoid creating explicit checklists of what to check. Using checklists, can easily turn into developers checking off each item an then thinking they are done with the code review, without really reflecting over the solution at all.
Why To Do Code Reviews?
From a manager's perspective, code reviews are often all about finding bugs and assure that the committed code won't hit back when testers test it. For me, as a developer, the main goal of a code review isn't to stop bugs, it's to involve more persons in the solution and to initiate discussions for alternative solutions and knowledge sharing. In the long term, that's what really going to improve the product quality and make developers write less error-prune code.
If you do code reviews correctly, they can help your team in many areas. These are just some of the benefits a code review can help you with.
- Reflecting over your code before you commit
- Learn and reflect about other developer's code and opinions
- Teaching and helping other developers
- Align the teams view on the product and its code and newly added code
- Getting better knowledge of the full code base
- Stop bugs from coming through
- Improve the quality of the code
- Improve the quality of tests
- Reduced technical debt
- Enhanced documentation with cleaner commits and better comments
- Identify security issues
- Save time by fixing issues during the code review instead of two weeks after
- Building a team
The last point in the list is what many people forget, but still one of the most important points. A good team communication is essential for all kind of business and development. Code reviews are an excellent way to let the team grow together because it is likely to cause conflicts, and anyone who knows anything about team building or conflict management knows that conflicts are necessary for a team to excel.
Tea drinkers would think it's a compliment
Why You Should Not Do Code Reviews
There aren't very many reasons for not doing code reviews honestly. It does takes time, yes, but that time you will definitely get back in the long term with all the benefits it comes with.
Think about it a bit. If you founded a tech company and hired developers. Would you like anyone to push any code without a single person looking at what they pushed? Would you rely on an open source project where anyone could contribute but no one was reviewing what was committed?
If your answer to any of those questions is no, or if you want to build a better team and improve code quality, then code reviews are for you.
What To Do Before a Code Review
A code review doesn't start with a pull request, there are certain things needed to be done before that, both as the developer who commits the code and for the person who is going to review it.
As a Coder
In a divergent team, there's always that architect guy who checks whole teams code, but also several more fast-paced developers who never really look twice at code and blindly push their code using their two characters alias for "git add . && git commit -m "save" && git push".
When submitting code for a code review (or even without a code review), those hasty developers should try to be a bit more like the architect guy. Some things that always should be done before committing code are:
- Keep the commit small, only a single story/task should be implemented in the commit. In that way, it's easier to review and also easier for teammates to track why a change was made when in doubt.
- Don't include code unrelated to the task you are working on in your commit. If you have fixed stuff unrelated to your task, commit it in another commit before or after the task.
- Does your commit include temporary code? Try to fix it before committing. If not possible, mark it with a TODO comment and plan when to follow it up.
- Does your commit include a complex solution? Write comments for the parts which needs it.
- Look through your code before pushing. Even if you have followed all the previous steps above without missing anything, you might find improvements for you code or other ways to implement the solution.
- Test your code manually. And if you haven't written any test, write those.
First when all above has been done, you are ready to commit your code and ask for a code review. When that is done, it's time for another coffee!
You know, I've seen better developers. On their first cup...
As a Reviewer
The main work of a code review as a reviewer is obviously to review the code, but before doing that, make sure to do a few things.
Firstly, make sure that the developer is done with the development and that all tests for it have been written. Don't review partly finished work unless it isn't to help out with another pair of eyes. Partly finished code is likely to change, especially if tests haven't been written and bugs may be found.
Secondly, talk to the developer who developed the feature to understand what the commit is about. Preferably with the pull request and feature in front of you. Doing this will make it easier to understand the code and the developer's reasoning.
Lastly, you can think about how you would implement the feature before looking at the pull request. This is a great practice to practice problem solving, since you will both come up with a solution on your own and potentially see an alternative solution for the same problem. If you are lucky, you the developer of the pull request implemented the feature in another way, which will make the code review much more interesting.
What To Do During a Code Review
When a pull request has been made, it's time to review it. This work lies mainly on the reviewer, but even the code contributor has a few things to think about.
As a Coder
As the developer of the pull request, you can often just sit and relax while the reviewer tears its hair. To make the life easier for the reviewer, make sure to be available for questions. If the reviewer and you don't know each other very well, make sure to mention you are available for questions, so that person dares to ask.
As a Reviewer
Reviewing large pull requests takes time. As a reviewer it is important to remember it is worth the time without stressing it, take a minute to look back at the section about why to do code reviews in the beginning of this article. Then embrace the opportunity to grow as a developer.
For anything larger than one-liner commits, make sure to check out the code for the pull request in your IDE. This makes it easier to follow links in the code and to search for usages of functions and variables. It also allows you to spot changes the developer should have done but have missed out, e.g. if a function is invoked at several places but the developer only have updated it at one place.
When writing comments on the review, be clear what you mean. If it is a matter of opinion, please make sure to motivate your answer. Apart from when your office's coffee machine decides to brew a "null coffee", there are few things as annoying as an opinionated review comment without an explanation sent with a confident "I'm the architect" attitude.
A little more seems about enough
Speaking about opinions. Don't be that bossy architect guy, unless your team haven't discussed the expectations of code reviews and asked for very critical reviews. If you have opinions, my personal opinion is that they shouldn't block merging if it isn't important.
To continuously interrupting work to fix arguably unimportant stuff like a function name which "could have been better" or an if statement which "could have used an if else" is really annoying and is time spent on fairly worthless matters. Not everyone cares if the attributes are sorted alphabetically. If you have strong feelings about it, make the linter complain about it.
In the end, it's up to your team to decide where the line goes. What are opinions and when will they need to be fixed? Should they block merging or do they only have to be fixed if there are more severe changes which needs to be done?
What To Do After a Code Review
When the reviewer have reviewed the pull request and it is time to hand the comments over to the contributor, it's a good time for a talk. By going through the comments together both developers get a time together for some knowledge sharing and to sort out any unclear comments and avoid misunderstandings.
As the author of the commit, this is the best opportunity to express any objections. If you think the comments make sense, make sure to not do the same mistake next time you push a pull request.
If everything looks good an you both are happy with the solution, go for a coffee together!
Top comments (4)
Thanks for reading! Let me know if I should post more articles like this by writing a comment or reacting with some of the reactions ❤️🦄 🔥
If you want more memes, you can find those on my Instagram. Enjoy your day!
nice piece of advices @perssondennis
You lost me at the pre-review section.
One should explain in PR description about the change, and also write good structured code, docs and comments where needed. Talking with the dev upfront is counter productive, because then you know what it is about and will not notice so easily the parts that are unclear.
Also thinking upfront about how you would implement is over the top. As if you don't have enough problem solving tasks that you need additional exercises. Now if the approach is complicated, unclear or flawed in whatever way, you would do it and suggest changes.
Which reminds me of the advise to wait until everything is finished and polished before review.For non-trivial changes, it makes a lot of sense to review early. If the approach is not good better change early, before all polishing work. That's more efficient.
Good points :) I agree to some extent, but I think we are imagining the situation differently.
Starting with your last point, that it is more efficient to review code early so the developer doesn't do work in vain. That's true, but that guidance should be done by a co-developer, not the reviewer. As you first mentioned, the reviewer should not have been involved in the decision making of the solution, it would be counter productive. Letting a co-developer review definitely spoils the purpose of the review.
And since the reviewer shouldn't have been involved in the development, it's necessary for him/her to talk to the developer before the review, to understand what the feature is about. Without that knowledge, it would be extremely time consuming for the reviewer to look at the code to understand what the developer has intended to implement.
And about thinking about a solution before doing the code review. I don't mean the reviewer should design a full solution, just to quickly think about it. The purpose of that is to get the correct mindset for the review. If the reviewer never thinks about what alternative solutions could look like, it won't actually review the solution at all. It will merely look at the code to check stuff like syntax, naming and trivial bugs. In that way, complex bugs and technical debt will be introduced, and one year later the team may realize that the design of that feature doesn't scale.