Поделиться через

A Case Against Code Reviews

  • Статья

I know, it sounds odd in the modern world where code reviews are almost like one of the Sacred Commandments, that is taken religiously and you could be burned at the stake for not respecting it. So, please, hold with me for a little heresy… No, I don’t argue that code reviews are valuable. It’s just everything in this world has cons and pros, and I’d like to discuss one unusual “con” of code reviews that usually does not come to light.

Anyway, imagine a small team very busy doing the work, that is, new features for your product. I stress that: everybody is busy. No kidding, real busy. And, surprise!, just like every other product, yours has bugs. And bugs are to be fixed, right? Right.

Now, how you’d fix a bug? There are generally, two philosophies about fixing the bugs. Well, may be more, and of course, there is that stupid “just fix the bug”, which completely ignores metaphysical side of the things we are discussing right now, but let me ignore this one for a while. So, these two approaches are patching vs. refactoring.

Now, what would you think is the right way to fix bugs? Admittedly, there are cases when patching is acceptable Examples? Quick Fix Engineering patches. Products, which are really done, and the less you touch them the better. Fixing embedded code of a space station passing a Jupiter, where every byte of the fix increases the chance of the whole fix to be corrupted and disable a multi-million irreplaceable spacecraft.

However, in most cases and in what’s considered “good engineering”, I strongly believe that refactoring is far superior to patching. It does not mean that every small bug requires complete rewrite of the product, not at all. But it means that you have to consider a bigger frame and not be afraid to change quite a bit, if necessary.

I know, I know, it sounds theoretical and questionable, so let me give you an example. Considering short size of an article, I’ll have to give a very simplified example, but quite a valid one nevertheless. Suppose, you have a method called DownloadCompletedInstant(), which is a callback at the moment, when some download is completed. In fact, all it does is to allow you to start downloading the next file. You find a bug, when one of our colleagues added to it something really really stupid, because he was confused with “Download completed” part of the name. In fact, it would not be stupid, if it would be in DownloadCompleted() callback instead of DownloadCompletedInstant() one. So, now you have two options:

1. Just move your colleague’s code from DownloadCompletedInstant() to DownloadCompleted()

2. Same as (1) plus renaming DownloadCompletedInstant() into ScheduleNextDownload(), hence avoiding a chance of future confusion.

So, what would you do? (2), clearly, right? However, there is a little problem. The renamed call is used in ten other files. So, although the change is microscopic, it affects much more files than a simple patch.

Now, your team is doing code reviews, so what’s the higher chance of getting it fast – posting a change with just a few lines of changes in a single file, or a changes with several dozen lines changed in a dozen or so files? And don’t forget, you want it fast. In part, because you want to fix the bug and get to the next, in part, because you don’t want the fix sitting on our computer and not being checked in. But! Everybody else is busy too. Everybody else has their own bugs. So, naturally, you would rather get a change with minimum affected lines just to get your colleagues do code review faster.

The result? The product gets a patch, not refactoring, and in a couple of months somebody will again misuse DownloadCompletedInstant() for something else, as bad as this time or even worse.

So, does it look right? Is it just me or is there a real problem somewhere here?

Comments