Bad Code Reviews Wreck Morale. We Need a Different Outlook
The right mindset makes all the difference.
Join the DZone community and get the full member experience.Join For Free
I love reviews. When it comes to new gadgets and whether they're worth my money, it’s a blessing to have so much free advice to guide your choice. Last year I bought a new digital piano online, purely based on internet reviews since retail was locked down and I couldn’t try it out anywhere. Most reviews were favorable to raving, so I found safety in numbers there. Sure, retailers reviewing their own wares are naturally biased, but then again nobody would go to the trouble of a scathing review simply because they don’t like music in general or have it in for Casio as a company. Pianos are not controversial, unlike coding styles. And movies. Cinematic opinions are as subjective and controversial as it gets. The site Rotten Tomatoes simplifies all reviews into a thumbs up/down verdict and gives you a percentage. It works great for anything in the upper 20th percentile as well as the lower. I rarely regret watching a 90% movie and I ignore anything under thirty percent. Movie reviews are pure gut feeling. There’s no checklist by which to arrive at an objective verdict. Often reviewers are just film lovers with a gift for writing. They are seldom other directors, screenwriters, or actors.
So why do we treat code reviews like movie reviews? A crude thumbs up/down rating exercise by someone who was not involved or even interested in the creative process. True, the analogy doesn’t hold when it comes to expertise since code reviewers are your programming peers, but that only means they should know better. The movie reviewer who shows off her literary style is like the code reviewer who likes to nitpick on small imperfections to make himself look clever. This is counter-productive, not to say toxic for team morale. You’d better not do reviews at all than do them in this way. Good teams usually do their best to be fair and constructive in their criticism towards each other if only because they need to get along and everybody knows they will be on the receiving end tomorrow. This sounds plausible, but it still misses what I believe is the true purpose of code review. That is firstly to share your work with the team, not to subject it to a quality test.
Reviews of a musical instrument or of Christopher Nolan’s Tenet are all variations on the same theme: the writer gives a verdict, and we hope they know their stuff and their opinion is not based on ignorance or spite. It’s basically a graded test. Reviewing code is not about testing that code. That should have been taken care of already. If every review yields new bugs for the backlog the work was not ready for review and you need to review your process instead. I mentioned that reviews should be about sharing. It is the stage during the development process where individual contributions become part of the team effort. It’s an active two-way transfer of knowledge. This is a world apart from the everyday meaning of review. There should be no hierarchy, no master/teacher relationship and the reviewer must be invested in the process. They won’t settle for a bad review, because it becomes their code too.
It's Not Your Code, It's Ours
Delivering software as a team doesn’t mean you write every line of code as a group; that’s just not practicable. Even pair-programming is not for everybody. Many developers prefer to get into the zone on their own and that’s fine. Therefore, some ceremony in knowledge transfer is indispensable if you commit to deliver as a team. Individual members should tone down their pride of ownership over their contribution. Stop thinking of it as your baby, but send it out into the world generously. The reviewer accepts this collective ownership and makes every effort so that it becomes her code too. She is entitled to say what she likes or doesn’t like and what she feels is missing or should be changed. There is no place for pet projects or for pet peeves.
A code review among fellow team members must be an opportunity to share knowledge in at least three areas. Firstly, there’s knowledge about the additions or alterations that the code comprises. These changes will touch on overarching domain knowledge. And thirdly there may be a thing or two to learn about generic coding practices, language/tooling features, and adherence to standards that the team/organization commits to. While there will be differences in seniority, everyone can learn from each other. The senior developer who has been with the project from the outset should still have their work reviewed by the new junior hire. This is not because he can’t be trusted to do his work without it, but because it’s a crucial opportunity to share and teach.
Opinions expressed by DZone contributors are their own.