Code Review: Principles and Mindset

In my experience code reviews make lots of people feel uncomfortable. While this primarily concerns reviewees since their code is being judged, some reviewers (myself included) may also have hard time deciding on the optimal strategy in a particular case.

In my opinion the biggest reason for that discomfort is that reviewing forces communication on people who are either not used to communicate or think it is not that important to do it properly. I’ve met a lot of developers who believed their primary job is to write the code. This leads to a number of poor strategic decisions, but what is important for our topic — it may lead to people not communicating mindfully or even embracing toxic behavior.

Generally being judged is a stressful situation. And being a reviewer gives one power that is somewhat easy to misuse. My main idea is to try and make this process smooth, pleasing and effective for everyone so that we are able to move the product forward, have fun and build strong relationships, not the other way around.

I do know there are plenty of ‘toxic reviewing’ out there in public. For example, some well-known open-source personalities would sometimes get mad and leave public comments that are straight up offending. And my stance is: do not follow such patterns in enterprise environment. I would rather not follow them even in open source development, but it’s somewhat more ‘relaxed’: it’s more often a hobby than a job. After all, if one dislikes something in an open-source project, it’s rather easy to leave and find another one (to a degree, but still). In enterprise world I would strongly recommend everyone to build healthy relationships instead of showing off frustration.

Reviewer

Do not judge. Don’t be a jerk. Or a smart-ass. That’s the difficult part, everything else is a breeze.

Let’s start with straightforward technical stuff: what should a reviewer pay attention to? I treat this as a mental checklist when going into a pull request.

Understand the context. What issues does this code resolve? What is the task description, what are the requirements? On rare occasions I might even think that the task itself should not be implemented because it might hurt the product. It is very unfortunate at this point for sure, since it means some time has already been spent to implement a solution, but still don’t hesitate to raise your voice if something bigger than the code concerns you. Acknowledging the mistake right now would definitely be better than acknowledging it a year later when the damage would have already been done.
Understand the code. Try to read the code and make sure it is readable in the first place. There might be a lot of code in a pull request, and sometimes when looking at larger chunks of code I prefer to only analyze general structure and architecture. The main idea is to prevent mistakes that would later have broad impact. What I mean is: it is way easier to fix a local bug in the implementation now rather than fix an inefficient interface heavily used by several components in a year. In this sense if you have to choose, focus on architecture instead of implementation, not vice versa.
Check if the code fits the style guide. Are all the public names clear enough to understand what they do without looking into the code itself?
Could this code be supported in a long run? Are errors handled clearly? If the product uses logs—is everything logged correctly? If something goes wrong, would we have enough information for an investigation?
Does everything as a whole make sense? Structuring, classes and methods responsibility, file structure and so on. Are there ways to make it simpler or more efficient?

Communication principles

Remember: this is about collaboration, not judgment or dictation. Try to drive the dialog instead of simply pointing out things requiring a fix.

Back up your opinion with arguments and context. Example: ‘This function is becoming large and difficult to understand. What do you think about splitting it?’ instead of ‘Split it’ and ‘I think we could try to make this method static since it does not depend on the object state’ instead of ‘should be static’.
Do not think your way is the best way. I’ve seen many reviewers pushing the way they like to do things instead of equally good ways reviwee brings. This is ‘I used this a hundred times and thus it’s the best’ mentality. Please, be open-minded, you may learn a lot from being a reviewer.
Do not ask questions that would make others feel uncomfortable. This includes a wide range of questions from ‘Does it even compile?’ to ‘You think this is good?’
Do not try to be funny and communicate with, say, emojis when you comment on errors or other areas for improvement. If you can’t live without emojis, I encourage you to use ‘positive’ ones (for example if you see some code you really like).
Do not spam. Suppose your style guide requires every file to have a special header with, say, company name, copyrights and/or brief file description. And one day you see a pull request containing 20 new files without these headers. 20 new files are for sure stressful, but please don’t leave a comment in each of them reminding the reviewee to add a header. One general comment ‘please add standard headers to every file, the format is described in the style guide available here’ would be enough. I’ve heard of cases where such comments were not enough and people still missed several occurrences, but in my opinion a reviewer should start mentioning every error only if the problem persists after an iteration or two.
Do not ask to fix something ‘since you’re on it’. If the code you are reviewing solves the given problem and does not introduce newer issues, it is fine. If you spot something worth fixing, make another task. It is usually very frustrating to do additional job that was never planned just because somebody changed the code in a ‘bad’ file.

No code is perfect. And what is important to know — no code should try to be perfect. There’s almost always a sweet spot also known as ‘good enough’. And it is often more reasonable to merge ‘good enough’ code today than spend two more days perfecting stuff that will never be relevant.

Reviewee

Try to make smaller pull requests, split them into meaningful commits, and otherwise help reviewers to understand what is going on. It might be a good idea to put some comments into your pull request (not the code) when opening it to focus attention on details.

Respond to every comment, especially if it is a question. Code review is an act of collaboration, so be responsive even if your comment is as simple as ‘agree’ or ‘done’. These help.
Don’t start style guide discussions in a pull request. Sure, not everyone agrees with every style guide, but a merge request is not a particularly good place to discuss it. Please never knowingly open a pull request with code that violates style guide: it makes it harder to read since our brains act on patterns.

One more thing I’d like everyone to remember: sometimes it would be way more productive to move the discussion offline from the pull request. It might take a couple of minutes to discuss a thing instead of writing a ton of comments and waiting for others to reply. And if you agreed on something offline, please make a note of it in the pull request. You’ll thank yourself later when you try to remember why it was implemented this way and not another.