As I've done more code reviews at Justin.tv, I've started to notice a pattern. When I get a diff and I have trouble keeping focused because it's just that boring, this is a sign that the code is pretty much ready to go into production. This isn't to say that the code should be repetitive; repetitive code is not boring. I imagine it might be boring if I read it, but the instant I see the same pattern more than once in the code I just make a note to refactor it and move on. So a really repetitive code review actually is one of the more interesting ones to read - if you don't find noticing repeated patterns in code fun you're probably not a programmer. A really boring code review has very little obvious repetition.
Another thing that often makes a code review interesting is implementing a new generic abstraction. Personally, I love generic abstractions. New kinds of iterators are a particular favorite of mine, because they allow you to so cleanly separate what you do to each item from how you find each one. So you can imagine, when I see a new iterator in a codebase, I get interested. Unfortunately, I find that when a programmer implements a new iterator it's nearly always because they either don't know of an existing identical one or have not noticed that their new iterator can easily be produced by composing common ones. You can generalize this idea to most kinds of really generic abstraction, because it isn't that often that you invent a truly useful generic abstraction. This isn't to say, never implement new abstractions, because when you do find one it's a really big win. But it's still a warning sign for me. A boring code review is also idiomatic, which is another way of saying it doesn't do common tasks in unusual ways. It's ok to do tricky or strange things in new ways, and in fact these are exactly the kinds of interesting parts of the code that are a joy to review. They still raise a red flag for me during the process, because it's hard to tell at first glance if something is a common task or not. Comprehending a really good hack is definitely not boring, and that's probably the best reason for a code review to be interesting. There's actually one kind of boring code review that I really dislike: changes to configuration files and dependency. I could cheat and claim that those are interesting because they're so likely to go wrong, but it's not true. Changing the version of a dependency is boring but also dangerous. A short list of things that go wrong when you change dependency versions:
- The API changed and your testing just missed it.
- The API changed, but in a non-obvious way. For example, what once was one type exception is now two different types and you only catch the old one.
- The API didn't change, but an implicit invariant you were relying on changed.
- The API didn't change, but the runtime complexity of an important function changed and you run out of resources.
- ... and so on However, the change in the diff that causes these problems doesn't look anything like that. It looks like someone changed 1.4.1 to 1.4.2. If you can, you can fix this problem by code reviewing the change to the dependency. That probably won't be boring! But depending on your situation, this may not be practical. You may be using a proprietary binary, or the dependency may simply be so large that you cannot review it reasonably. In that case, the best course of action is to read the patch notes thoroughly, test as well as you can, and pray. In fact, this may be the real reason why upgrading a dependency is such a dangerous thing to do. It doesn't trigger the alarm bells that it should because it seems so boring. I've noticed that anyone who has been writing production software for a significant length of time has learned to flinch away from upgrades the way you would flinch from putting your hand on a stove that might be hot. In short, interesting code is often the sign of hack, and it's up to you to figure out whether the hack is a clever or useless one. And just because the code is boring doesn't mean it's not hiding hidden problems. But I still find myself surprisingly pleased with a nice, boring code review. Agree or disagree? Comment below and tell us!