Given that people can get really touchy on the subject, let me state this up front: I’m not saying we should tolerate lousy code.
Over the course of my career, I’ve seen something that I will call “the cult of abstraction.” On the surface, this sounds like a good thing, until you need to debug their code. While on the surface, it’s all nice and pretty, and uses classic design patterns, when you actually want to find out what it does, you discover that:
- what it does is essentially simple, but
- has excessive amounts of indirection and abstraction so that understanding it is a day’s worth of work tracing through the nest of indirections
The real world is messy and problem domains are often messy. That makes for complex and complicated code which can be bad. However, sometimes it’s really more comprehendible by leaving the thing the way it is rather than increasing the conceptual load by adding abstraction and indirection to make it “clean.”
Ultimately you still need to understand the problem of the domain it’s trying to solve. Obscuring the nastiness of the domain can decrease the clarity you need to understand what’s actually happening.
Some of you have started to wince, so let me qualify things: there’s the essential complexity of what you’re trying to do versus the accidental complexity of the language and tools you’re using to express the rules. For the latter case, refactoring/indirection/abstraction can indeed be a great tool. For the former case, refactoring/indirection/abstraction can still be a great tool, but you have to be much more careful. Sometimes the domain itself allows for obvious applications of these things, but when it doesn’t, that’s when you need to be careful.
As an anecdote, at one of my previous jobs, I wanted to conditionally load something at server start based upon a command-line flag. My original patch was four lines, one of them being a blank line, and another being a close curly brace by itself. It took about twenty minutes to do and test, and I sent it out for code review by the other team. By the time it was done, by request of the code reviewer who was of the “cult of abstraction” bent, it was at least an interface, two classes and a Guice module or two, and took about two days to build and test. Lest you think that I’m being unfair to the other team, while this code was common code, I spent enough time in the code to know what was what (and later my thoughts on the subject were confirmed by several others who’d had similar experiences). Unfortunately, due to the policy of mandatory code review by an owner of the code, and the whole team’s cult-ness, I couldn’t just push the simple change, nor was there a more sympathetic reviewer I could appeal to. But I ask this question: at the end of the day, which code is more understandable to people who’ve never seen it before?
Fortunately, I can answer this one. Many moons later, I was debugging along, and was trying to dig into things to figure out what was going on. I finally got to the bottom of a chunk of code and was thinking “Who writes code like this? So much code for such a simple thing! GRRRR!” I was so mad, I actually looked up who the author was so I would know whom to burn in effigy. To my dismay, looking through the history of the file, it was me. It wasn’t the exact change that I talked about in the previous paragraph, but it was a similar change I had made later after I had learned the propensities of the other team.
The ultimate point I’m trying to get to is this: cleanliness and refactoring and abstraction are all good, when used to simplify understanding of the code. That is:
Cleanliness, abstraction and indirection are good when the comprehendibility of the code afterwards is increased by more than the conceptual load imposed.
This should sound obvious, but way too many times I’ve had to deal with coders who insisted on these things at the expense of comprendibility.
Random data point: in my experience those of the cult of abstraction ilk tend to be those who would rather endlessly polish a piece of software rather than ship.
For any of the engineers at my current employer that I’ve encountered, this does not apply to you.