Witch-hunt code review (and other bad CR habits)

I believe we can all agree that code review is one of the most important tools in serious development team’s toolbelt. Not only it allows us to spot errors and potential bugs earlier (than having landed in production), but it also greatly enhances knowledge spreading and code ownership. We are probably familiar with some rule about it, such as “always find something good about the change under review”.

But after a couple of years of practicing CR, I spotted some patterns leading to bad code review. By bad, I mean mostly “discouraging”. I personally call it “witch-hunt code review”, because it resembles medieval with trials, when there were accused persons and an effort was to find evidence of their guilt, not to find whether their actually guilty.

Outsmarting Rubocop

This is probably most irritating I ever experienced. You see, in our projects we had Rubocop to ensure static code quality and it was plugged in to CI to enforce it. However, for reasons beyond the scope on this post, the CI was not run on PR creation, but only after merge to master. As a result it was developers responsibility to run it manually before submitting a PR.

Of course, sometimes we tended to forget it, causing CI to fail after merge and creation of “Fix Rubocop offences” commit ad hoc. As a result, some “smart” colleagues discovered that it’s a great thing to do during code review: make sure that Rubocop was run. Unfortunately, they tended to rely on the hunch rather than checking out the correct branch and run a check which took up to 10 second. As a result, you received a comment like “hehe Rubocop won’t let it pass” which were simply not true.

Shouting for tests

This was another common thing and a “safe” code review comment. After all, we all agree that tests are good and tests are a must. So why not write?

Hey! I don’t see any tests for the change. Can you add it?

Everything is fine when test are really missing. But if you are fixing something in an old admin panel, where only front-end logic is some jQuery to disable/enable checkboxes, chances are that only backend is tested. Actually, testing such frontend efficiently is a very hard task and you don’t include it in a “small but urgent fix”. It is something that should be discussed (how to do it best) and have separate tech ticket, where at least two people are included.

But that, unfortunately, does not stop some people from using this shortcut code review comment.

Enforcing one’s own point of view

Sometimes there are thing that are under a longer company-wide debate. For example, whether to use many slow integration tests or just concentrate of few most important and squeeze the most out of unit tests. There are, of course, two sides of such debates. One of the most harmful things is to tell ex cathedra to adhere to what you side wants. Sure, you can use it later as argument, because “people already do that”, but it is not moral. Especially if you are senior and are bending juniors to your will.

Another example of this is stating your opinion as if it’s an objective truth. Maybe you have a guy in the team who watched the presentation about, say, that using helpers in Rails is plainly wrong. He did not start a discussion about that, trying to convince his colleagues. Instead, once in a while he posts something like that during code review:

Oh, I see you are using helpers. But helpers are wrong. Here is a link [to 2.5 h presentation] where you can find out more.

The problem here is that the guy is right, because helpers are usually wrong. But the way he expresses that is useless. The team had not discussed it and does not know whether they agree or not. Even if more people are convinced, they probably don’t know what to use instead of helpers. That is why it should first be discussed and included in company code guidelines. Otherwise it can result in even worse code (but without helpers).

Archeology

I’m kind of puzzled that I have to write about it, but commenting on a PR that has been long merged and ticket was closed is usually not the best idea. Especially if you are going to write “you did everything wrong here”. Usually in company working culture there is no place for going back, reverting PR, doing it over and submitting again.

Overengineering and blind faith in abbreviations

This is more of a generic problem, not just code review-related, but it appears during code review quite often. Examples are:

Overengineering:

Hey, maybe this method could be more generic, because in two years we may need to use it also here and there.

DRY. Don’t Repeat Yourself is generally a good rule, but it requires a context. In some cases blindly following it results in less readable code. It is easy to spot two similar method in two very far ends of the project by looking at the diff on Github, however it’s not always easy to see a bigger picture.

I see duplicated code. Can you perhaps extract it to a separate module and include in all three places where it’s duplicated?

Other?

This is probably just the tip of the iceberg. Care to add something to the list? Let me know in the comments.