Today’s awfulness comes from Tim H, and while it’s technically more than one line, it’s so representative of the code, and so short that I’m going to call this a representative line. Before we get to the code, we need to talk a little history.
Tim’s project is roughly three decades old. It’s a C++ tool used for a variety of research projects, and this means that 90% of the people who have worked on it are PhD candidates in computer science programs. We all know the rule of CompSci PhDs and programming: they’re terrible at it. It’s like the old joke about the farmer who, when unable to find an engineer to build him a cow conveyer, asked a physicist. After months of work, the physicist introduced the result: “First, we assume a perfectly spherical cow in a vacuum…”
Now, this particularly function has been anonymized, but it’s easy to understand what the intent was:
bool isFooOrBar() {
return isFoo() && isBar();
}
The obvious problem here is the mismatch between the function name and the actual function behavior- it promises an or operation, but does an and, which the astute reader may note are different things.
I think this offers another problem, though. Even if the function name were correct, given the brevity of the body, I’d argue that it actually makes the code less clear. Maybe it’s just me, but isFoo() && isBar() is more clear in its intent than isFooAndBar(). There’s a cognitive overhead to adding more symbols that would make me reluctant to add such a function.
There may be an argument about code-reuse, but it’s worth noting- this function is only ever called in one place.
This particular function is not itself, all that new. Tim writes:
This was committed as new code in 2010 (i.e., not a refactor). I’m not sure if the author changed their mind in the middle of writing the function or just forgot which buttons on the keyboard to press.
More likely, Tim, is that they initially wrote it as an “or” operation and then discovered that they were wrong and it needed to be an “and”. Despite the fact that the function was only called in one place, they opted to change the body without changing the name, because they didn’t want to “track down all the places it’s used”. Besides, isn’t the point of a function to encapsulate the behavior?
[Advertisement]Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!
Source: Read MoreÂ
