Code Review Antipatterns

Don’t you love code reviews? Having your grand solution scrutinised, criticised, and optimised by your colleagues makes you feel awkward? It doesn’t have to be that way. Code reviews are actually fun if done right and if the participants are aware of the pitfalls. They are not just conducive to achieving a high level of code quality, but they are also an ongoing learning experience.

There are three basic forms of code reviews: informal reviews (“hey, can you have a look at this?”), formal reviews (a formal task assigned to one ore more peers), or pair programming. The latter form has the advantage that it is done at the time when code is created. It is also the only form where two individuals engage in a direct conversation. There is no time lag. This makes pair programming the preferential form of review for high-priority tasks.

Agile teams working with git (or any other version control system) often encounter code reviews as a step in their code merging workflow. The typical unit of code being reviewed is a merge request, aka pull request in this setting. So, here are a number of things to avoid when doing a code review.

Not doing code reviews - Let’s begin with the obvious. Four eyes (or six or eight…) see more than two. Errors, problems, vulnerabilities, and bad design gets spotted early on. Corrective measures are still inexpensive at this stage. There is a dual benefit. Code quality increases and knowledge is shared. At least two people of a team are familiar with each feature or function, namely the author and the reviewer.

Confusing code reviews with functional reviews - Code reviews focus on code. Duh! The review is primarily (but not exclusively) concerned with the non-functional properties of the software. Functional reviews, on the other hand, ensure that functional requirements are met. The latter activity is closer to testing, therefore it makes sense to perform functional reviews in a separate procedure, probably as part of testing.

Bitching, nitpicking, fault-finding, patronising - It goes without saying that treating colleagues with kindness and respect is necessary for productive collaboration. Code reviews are no exception. Offensive behaviours only derail the process, while courteous and supportive comments go a long way. The reviewer must guard against coming across as a know-it-all and keep office politics out of the dialogue. In written reviews, annotators should aim at being helpful without being wordy.

Discussing code style conventions - Code reviews should not be about lexical code style, formatting and programming conventions. There are automated tools for this. If you care about formatting and code style, use a linter. More importantly, decide on a set of programming style conventions and configure the linter accordingly. It is totally OK if the team agrees to leave certain options open to individual preference. The formatting and style of code that passes the linter should not be argued, because it’s simply a waste of time.

Ignoring naming - Programming conventions may or may not encompass identifier naming conventions. However, linters cannot determine whether a given variable name is good or bad. Machines don’t care about names, only humans do. Phil Karlton has described naming as one of the two hard things in computer science. By all means, do review the identifier naming in the code review. As a rule of thumb, if an identifier name leaves you puzzled the first time you read it, it is probably not well chosen.

Debate - Code reviews aren’t meant as a podium for discussion. The objective is to improve the resulting artifact, not to find out who has the better arguments. Sometimes, people do have different opinions about practices and solutions. It may be helpful to resolve these by giving examples and references to commonly accepted best practices. If that is to no avail, a third party (or fourth or fifth…) should be be involved. In a written review, consider involving a third party, whenever the dialogue drifts into debate.

Not getting a third opinion - Involving a third party into the review is not only useful in cases where two people cannot agree. See first antipattern: four eyes are better than two, and six are better than four. If an important architectural question is at stake, why not involve the whole team? See: mob programming.

Reviewing huge chunks of code - Code reviews should be limited in scope, ideally to a few pages of code that can be completely read and understood in 20 minutes. Studies have shown that thoroughness and number of spotted issues are inversely proportional to the amount of code being reviewed. Therefore, results are improved if smaller units of code are being reviewed. Including time for reflection and annotations or discussion, the entire review should not take longer than 45 minutes. Anything exceeding one hour is questionable.

Unidirectional reviewing - The general idea is that everyone should review everyone’s code. Most teams have both senior and junior developers on board or perhaps a team lead or an architect. This often leads to the situation that seniors only review juniors, but not vice versa. This is wrong. First of all, seniors can make mistakes, too. Second, juniors can benefit from reading code written by senior developers. Third, varying fields of specialisation can be leveraged. A junior developer might have special knowledge or skills in a certain narrow area.

Rewriting code - It’s called code review, not code rewrite! Rewriting someone’s code is just not appropriate in most situations. It’s akin to pair programming where the navigator just grabs the keyboard from the driver whenever he feels like it. It’s simply rude. If the reviewer has a better idea, he should annotate the merge request with code examples.

Overdoing it - Programming is not a beauty contest. Code does not need to be perfect to be functional and maintainable. For example, even though I might prefer a functional loop construct over an imperative one, it’s just fine if the imperative one works. Perfectionism sometimes gets in the way and leads to other antipatterns such as premature optimisation and presumptive features (YAGNI). Also: A piece of code written for a one-time migration or ops procedure does not require the same high standards as production system code.

Underdoing it - Being too forgiving is likewise unhelpful. If the code review is executed as a mere approval formality, it’s a waste of time. Code should always be inspected for things like security holes, logical errors, code smells, technical debt and feature creep. It is important to strike a balance between clean maintainable code and reasonable effort.

Not reviewing tests – Unit tests are often mandatory and sometimes functional and acceptance tests are also added to the artifact. These should also be reviewed to ensure that the tests are present, logically correct and meet the standards. It’s a good idea to verify an agreed upon code coverage percentage as part of the review.

Not using a diff utility – Most teams probably use a tool set with built-in diff and annotation capability. In case you don’t, you make your work harder than it has to be.