Your code should be as boring as possible
by Emmett Shear
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!
-------------
If you care about code quality, come work with us! We believe in building engineering focused product teams and are planning on changing how the world shares video with Socialcam.
Knowing that you use Python and PHP extensively, I suggest you consider that one reason boring code results in production readiness more often than not for you is due to your choice of dynamic type systems. It would be nice to see if the correlation between "interesting" (read as: complex and/or using more features of the language) code and production readiness is itself correlated with type system of the choice of language, or even just the choice of language. How about you add a qualitative feature to your data for choice of language and see if anything changes?
Good engineered code isn't boring. It gives you a very pleasant feeling when you build something which is just simple, clean coded, runs the tests and you know it does the thing it should do the best way you know how to do it.
I know, this code is very rare in production code but it exists and you're totally missing this one.
You did not discuss the role of algorithms. Even *if* the code is boring, it is useless if the algorithm not correct or optimal. And the code might be less boring if the reviewer is not familiar with the algorithm! :)
One potential problem with boring code is that since it reduces your attention, you might overlook some trivial mistakes.
Plus, totally agree with you on the API change issue, Code breaks in a snap with new headers and you don't even know why!