Code reviews do not have to be stressful
Do you find code reviews to be stressful?
Tom Kubit, an XP die hard, does say so too.
Don’t get me wrong, I love XP and I have had some great fun and satisfaction when doing pair programming in the past. Developers not only exchange ideas but also pick up people skills. They see what excites a fellow developer and what doesn’t. During pair programming or sit-down-together code checks, the feedback loop from traditional submit-and-wait code reviews is more or less eliminated. But pairing up to code is an expensive proposition. Especially today, when the organizations are trying to squeeze every minute of your waking time to code. Velocity baby!
But we have come along way since Fagan’s inspections. Since approaches like those are super expensive, majority of the folks in the software industry have moved on and now work off of code reviews submissions. Be it Git’s PRs, Crucible or Review-board. And those can be stressful, no doubt. I have experienced stress both as a reviewer and as an author. I have also observed it on other reviewers and authors.
So what causes that stress? Well, I have noticed a few patterns over the past few years working in different areas. Here they are and I will touch upon how tweaking a few things might help. I will try to be objective :)
Passive language of the reviewers
Let us face the ugly truth first. This bugs most engineers out there. You really need to have a thick skin because reviewers often do not come across as polite. It is the nature of the beast. Code reviews are passive aggressive in nature. I have seen this on reviews especially during the end of the day. Passive mode is far more crude and rude than sitting together and being soft-voiced yet direct.
Best to have language guidelines like the ones I have one here. It does not cost a lot to you as a reviewer to add subtle phrases such as “Best if…”, “Prefer if ..” or the good ‘ol abbreviated ‘Pls’. Even if these common pleasantries are beyond you, making suggestions in an inclusive manner like “Let's try..”, “We should…” goes a long way since you are not coming across as an “individual” here.
Code review should be about code
Reviewers often turn the review into a piñata by bashing the screens posted on the review or translating the code from a user workflow point of view. Before you know it, code review is now a UX review! What is actually being reviewed is the code as it translates to the screen. If the review becomes about UX, then something is missing. By UX, I specifically mean task workflows and interactions, not colors or spacing, etc. People are confused about missing consistent patterns and they take it out on the code reviews. Screenshots should be about matching styling in code with high fidelity mockups or presenting the drastic look and feel changes. They are not truly UI workflow patterns. Those should have been decided before the code was written.
But this does give an opportunity to do UX review and usability tests results of which should bring you back to the drawing table. In any case, trying to get perfect UX related consensus during code reviews is a poor strategy and that way you will never leave the code review. Code review is all about shippable code. If there are glaring gaps in the UX from the code perspective, then do not give it a ship-it! The beauty of being agile (bless your heart if you are not) and having short sprints is that UX can always be enhanced, even if an imperfect workflow goes in as an MVP or a feature behind a production flag.
Too many reviewers
If you send your review to an open group, you are asking for trouble unless you meant to. Reviewers with no context of the story being the code whatsoever may have a field day with the code posted and nitpick on “everything”. That creates a lot of noise.
While the above is OK for trivial fixes, best to include one reviewer for bugs and two for features or enhancements. On the other end, the reviewers who are not invited to the review should only intervene if there is something is amiss glaring. Otherwise, it is nothing but an open review.
The Alpha
Even in teams that appear to have no hierarchy whatsoever, there will be an engineer or two who will turn the review or sub-points of it into a pi$$ing contest. Since they may not like talking about it face to face, the review becomes their outlet and you will soon notice that it is much ado about nothing.
Well, the person who is most knowledgeable about the codebase and the dead bodies in it or has had the longest tenure, usually “wins” here. And if those reviewers cannot solve the issues amicably, then the best thing to do is to have a chat with the whole team. Lay it out there that this is not acceptable and if the review point is so controversial, it should be taken offline and onto a whiteboard. Deal with it objectively and not just based on opinions. Again, having a reference guide on how code review fits into your particular culture. This will let the team identify the members that are unreasonable and deviate from the practice drastically.
Open-ended questions
Usually, this happens with reviewers who ask unreasonable or open-ended questions. This is mostly because of just looking at the lines changes and not gathering a context before reviewing, or even reading the author’s change notes on the review. As I mentioned above, discussing UX questions may confuse the author. I have been both guilty and a victim of this. Reviewing the code in an interrogative fashion is a technique very few of us have mastered. And those statements often are entered in an open-ended question. That starts a back and forth cycle on the review. Now you see Pair Programmers laughing at you!
Usually what works the best with reviewers (who do not get what being direct means), is to actually go over to them and talk it through. As the author, try to understand that person’s point of view and where they are coming from. It is OK to set a rule in the team to avoid open-ended questions. As a reviewer, using code reviews as a tool to know how much an author knows about a certain feature is absolutely wrong way to approach it. Talk instead! The other guy is a person, not a fish!
Didn’t I just review that?
When you update a review, it (mostly, always) generates an email to the reviewers, which could notify a whole group of developers. When comments are put in and replied to in a quick Q & A fashion instead of a wholesome change, it can create a distraction for reviewers and make them think, you acted on the review comments when instead you were just replying to some comments while you were still making changes per some other comments. This can come across as annoying to the reviewers.
Know your code review tools! Only reply to the commons as a wholesome set of changes and publish replies once. Do annotate the change set as “changes per the last review from John and Mary”. This is a clear indication that reviews were changed. Yes, it is clerical work, but getting used to the tools is important as tools are an extension of the company’s culture.
Architectural Changes = Large reviews
Say a developer starts working on a plus size feature. In the agile development world, detailed architecture documents are frowned upon, so one is missing beforehand. But when the code is submitted for review, you find so many functional bugs that the review will end up looking like a Reddit forum by the time reviewers are done with it.
To avoid ending up with such reviews, really work on hard breaking down even the MVP (Minimum Viable Product) into what I call R VPs (Reviewable Viable Pieces). The first review could be about the build infrastructure, then backend work followed by one for UI. Yes, this adds a bit of time, but at least this puts up a fight to Parkinson’s Law of Triviality and important issues will not slip through. If such a review has been submitted, then another strategy would be to grab a room and go over the review together with subject matter experts separately and capture issues. This keeps a multitude of email notifications away and discussions are sorted out on the spot.
Soak it all in mate!
This again goes for the author. I have worked with folks who were much smarter than me, but I have never worked with engineers who were the greatest ever. No such creatures exist. We all know what we know based on our experiences. Hence, to let ego get in the way of accepting constructive criticism in reviews is a self-destructive exercise. Easier said than done as its human nature to be defensive, but we can start being mindful of starting now! A review sprinkled with small doses of all problems listed above is sometimes enough to discourage you as an author. But don’t give up! Adapt!
Hence, besides following the practice established in your own team, code reviews are an excellent opportunity to learn from others and share your experience. Have a thick skin, take reviews objectively and determine what you learned from each review. Talk about in person what is not clear enough. Code reviews are nothing but a communication channel.
Originally published at https://medium.com on November 6, 2015.
