Code Reviews Waste Time and Energy
Update (2020-08-08): This is a republish of my seventh blog post, dated 2012-07-19: Code Reviews Waste Time and Energy. Devblog won't let me backdate this before 2015. This also happens to be the last blog post from my original blog, http://mkrebs-programming.blogspot.com/.
I'm not quite as cynical now about code reviews as I was back when I wrote this post. That may, in part, have to do with me changing teams since then -- and consequently using an entirely different code review system. I'm not dealing with open source anymore either, and the coding style is very consistent now. Maybe someday I'll write down my current thoughts on code reviews. (I still have gripes, but they're different gripes.)
When I first heard of using code reviews, I remember thinking they seemed like a waste of time. But, to be fair, I had heard of them in the context of Extreme Programming -- which I had already thought was a waste.
Now I work for a company that requires code reviews for every change you make, and I know they're a waste of time. But what I hadn't realized, until after I was subjected to them, was that they are also a waste of energy. I'll describe below a general scenario that has played out on more than one occasion where I've needed a code review.
First, you have a change you want to commit. So you go through the motions of uploading the change to the code review server and add a reviewer. Then you wait.
You get your review back, and the reviewer has marked up half your changes as being against the current coding style that you're supposed to be using. But here's the thing: you were just copying the same style that already existed in that file. Coding styles are an ever-changing thing at companies, so this isn't that unusual, and in my case I mostly work on open-source so the file could very well have been written at a different company.
But you make the changes, because you can't commit without the reviewer's approval. In the meantime, another reviewer has been added, either because the first reviewer thought he/she might have something to say too, or because they have a watch on code reviews and wanted to add their two cents. Now, the second reviewer sees your updated changes and flags the review saying you need to make your changes consistent with the style in the rest of the file if you want to commit it.
Okay, fine. In order to make both reviewers happy, you now go and change the rest of the file, along with your change, to match the current style. Mind you, don't forget to re-run all your tests on the updated changes! Finally, you upload your latest round of changes. If you're lucky you can just commit everything now.
You think you're done? Maybe not. Because in about a week, you have a third person trollin-- er, browsing around the source code and they come across your change. They add a comment to the now-closed code review saying you need to redo your changes because they were too big. You can choose to ignore this (which I've done sometimes), but you run the risk of making enemies -- which sucks if you end up needing that guy/gal to review your next change.
How I usually choose to resolve this is to just placate each person along the way. Eventually, everyone will get bored and move on. Once, I had two reviewers that were feeling particularly stubborn, and they made me change my code back and forth (the first reviewer insisted I use different logic than I did, and the second reviewer insisted I use the logic that I had used at first). In that case, I tried pointing them to each other and told them to work it out; eventually, one of them gave up. Of course, a third reviewer came in two weeks after the commit and told me to use a third kind of logic!
All in all, most of my code reviews are a waste of time and energy. At best, I get someone who has better things to do and so just tells me to go ahead. I wish I could say that was the norm.
I should mention that I rarely have bugs caught in my code reviews. I don't know if I'm that good (I do test my changes a lot before committing), or if my code reviewers are that bad.
Maybe I should intentionally throw a bug into my uploaded changes. That way the reviewers can still get that sense of accomplishment, while leaving the piddly stuff alone.