This post was first published on CoderHood as Making babies, emotions and code reviews. CoderHood is a blog dedicated to the human dimension of software engineering.
Making a baby by typing code.
A man approaches you holding a baby. He looks at the bundle with loving eyes before turning his gaze to you. “What do you think?”, he says, “He is my baby! I bathed him, dressed him and brushed his hair. I just finished, barely in time for the deadline. Isn’t he beautiful? What do you think?”
You study the child for a few minutes, without saying a word. The man waits, standing in front of you, shifting his weight from one foot to the other, moving his gaze from you to the creature in his arms, back to you.
You finally look up and say, “I don’t know about him; he has an odd-looking head that you need to cover, his clothes don’t match, and the smell tells me he needs a diaper change. Please do something about that.”
I omitted the rest of the conversation because contains expletives.
The problem.
Code reviews can feel like that. Breaking the news that something is wrong with a developer’s labor of love can be difficult for all involved. Since developers tend to identify with their code as much as parents identify with their children, any hint of criticism, even if constructive, can lead to hurt feelings.
Nonetheless, code reviews, hard as they might be, are critical in any development team. They function as a tool to prevent obvious mistakes, they increase transparency on progress, and they promote spreading of knowledge, making everyone aware of what’s going on in parts of a product they are not working on.
Effective code reviews.
Some experts have attempted to create rules and measures to give guidance on how to conduct a code review. For example, in this article titled “11 proven practices for peer review” published on the IBM site, the following guideline is given: review less than 300-500 lines of code per hour and for no more than 200-400 lines of code at once.
I am not a believer in fixed rules and measures, as not all code was created equal. One complex line of code might take a few minutes to review, and a whole page of simple code could take a few seconds. That said, here are my recommendations in no particular order:
Understand the code.
If you are the reviewer, strive to understand the code you are reviewing truly. It takes time, and you can’t rush it. If you need to ask questions and clarifications, ask for it in the form of comments in the code itself. That way future developers will benefit from the same documentation.
Keep reviews small.
Insist on relatively small reviews; if you use Agile methodologies, a small story worth of code is as much as you should be reviewing. If the amount of code to review is too large, a proper code review might have to start with a meeting where the developer gives a guided rundown of the source and the architecture.
Avoid aesthetic wars.
As a reviewer, don’t spend anytime reporting indentation and formatting issues. Utilities exist to style code automatically, and it’s not worth the time to nitpick aesthetics. Instead, spend that time configuring an excellent beautifier to style the code as per your team’s accepted standards.
Avoid discussing trivialities.
As a reviewer, avoid raising trivialities as issues to be resolved. Developers have different ways of doing things, and small differences should be expected. Fighting every little detail will cause code reviews to become a torture that nobody wants.
Sometimes there are no issues.
It’s OK if the code looks good and does not need any change. Do not expect problems at every review, and don’t feel obligated to find something that needs corrections.
Understand the problem, architecture, and domain.
Make sure you understand the problem that the code was written to resolve and the general architecture of the system it is part of.
Surface the bad but don’t forget the good.
Praise aspects of the code you like and point out things you learned from it. Don’t underestimate this part. Pointing out positives will reinforce good practices and cheer a developer’s day.
Reviews are about the code, not the person who wrote it.
Make sure to focus only on the code and not on the individual who wrote it. You are not judging the developer; you are helping him or her refine the code to prevent issues. Avoid phrasing anything in ways that could make the developer feel judged or criticized. Pointing out matters in the code is good, criticizing the person who wrote is not.
Don’t take it personally.
When someone reviews your work, remember that you are not your code. The reviewer is helping you catch issues, and is not criticizing your ability to do your job. Be humble, and don’t take it personally. Even best-seller fiction writers have editors; similarly, even the best developers have reviewers who find issues.
Learn from it.
When you are reviewing somebody’s work, try to learn from it. You are looking for potential problems, but it won’t be rare to come across many smart solutions that, if learned, can make you a better developer.
If your code is under review, learn from the comments and suggestions that your peers give you; it will improve your skills and will make you stronger.
If you enjoyed this article, keep in touch!
- Find all my posts on CoderHood.
- Join my professional network on LinkedIn.
- Follow me on Twitter.
- Join my Facebook page.
- And finally, follow me here on dev.to!
Top comments (4)
Nice article Lorenzo! :)
I also personally add "MoSCoW" ratings to my PR's. I've found since adopting this habit I've seen less friction in code reviews. Something about adding [COULD] or [SHOULD] before a comment seems to diffuse tensions especially when making slightly more pedantic requests, "trivialities" as you call them :). I think it's better to give the small comment but allow the writer to know it's optional / not that important then they are more likely to add it themselves in future.
Yes, that is a very good suggestion! Details like that matter!!
One thing I do to avoid frictions is to speake always in plural, e.g instead of saying "You should change" I write "We should change"
That way the code is not attached to the writer only, but to the whole team instead.
That also promotes team ownership! Very good. Thank you for the suggestion.