I’m quickly realising that “bad” code is a vast exaggeration.
Listening to many developers talking to me about code that they consider to be bad, it is becoming clear to me that what is meant by bad is not necessarily what the word implies. Instead, many developers seem to mean “I don’t understand this code”. Worse, it’s not that they don’t understand, but that they don’t want to understand. Here’s a couple of examples to explain:
A developer recently told me that he found that writing LINQ one liners was bad practice*. After quizzing him a little he cited several examples of LINQ that he did not instantly understand. After writing imperative versions of the same code, I at least came to the conclusion that the LINQ one liners were in fact more clear than the procedural code. I relayed this to the developer in question, and indeed, he admitted that he did not find any of the procedural versions easy to understand. In short, this developer baulked at the idea that code had been compressed into one line, and did not consider that this description might be easier to understand than the alternative. The sole reason being that he was not used to working with LINQ, and did not have his brain prepared for understanding LINQ statements.
Another developer recently told me that his colleague had written some bad code. He cited several reasons why he considered the code to be bad which all seemed quite reasonable – indeed, I was convinced that the code was poor. On consulting the other developer though, my opinion changed. The second developer explained that he too felt that the code was ugly from that point of view, but that if he had implemented it another way, it would have been more ugly from another point of view. His arguments were enough to sway me that his code was not bad, instead, he’d just thought about the problem from another angle. Of course, one could make an argument that none of the devs (including myself) had thought for long enough about this code. There probably was a solution that solved both sides of the argument neatly, but at some point, we have to write code and produce a working product. The key point here though is that the original complainant had not understood the problem fully, and had therefore declared the code to be “bad” prematurely.
The key to both these problems though was a lack of understanding. A developer’s job is to wrap their head around a problem fast, and to understand it from all angles, in these two cases I’m not convinced that’s happened. In future, I’m going to treat developers telling me about “bad” code with a large pinch of salt. Instead of assuming that the code is actually bad, I will assume that the developer in question has simply not understood the reasoning behind the code yet.
A second piece of this puzzle is the hunt for perfection in developers. Few developers will ever tell you that they consider any piece of code to be good. This includes code that they themselves have written. For any given piece of their code, a developer will typically list several things that can be improved, often in conflicting ways. This contributes heavily to the lack of understanding. A new developer on this code will not only have to understand the original developer’s reason for designing the code in a certain way, but they’ll have to understand where and why they made ugly implementation decisions. It may simply be that they haven’t had time yet to clean up the problems, or that there’s a trade off involved. The fact remains though that this introduces an extra variable to the lack of understanding.
Ultimately, what this boils down to is that no developer is happy unless they have their head well and truly wrapped around a problem. When first starting to understand another developer’s code this is not true. The result is that all too often code is declared to be “ugly”, “bad”, “messy” or any number of other derogatory terms. Instead, typically what is meant is “I don’t understand why this developer did this”.
My gut feeling is that this actually lessens the impact of terms like “terrible” code. These terms should be reserved for code that is actually erroneous, or is inefficient to the point of being in a complexity class it clearly does not need to be in. So please developers, stop using the term to brand all code that you read. Instead, make constructive criticism of the code, and try to understand why it was done that way in the first place.
* LINQ is a functional programming inspired API that allows developers to write clear, concise “queries” to extract data instead of complex loops in loops.
My short response is: You’re Wrong.
If code is not easy to understand by someone, usually because there is not a comment saying what the code does. That is bad code. If you can’t easily tell what the code is doing, that is bad code. The point of code is that is should be understandable and maintainable, with the later being typically dependant on the former.
Comments + sensible variable names + sensible formatting == good code.
This is why one liners are a silly thing, they by their very nature are not good code.
I agree entirely up to the last line. Indeed code should be easily understandable, and indeed “bad” code is often code with not enough comments. But that doesn’t mean that the developer attempting to maintain the code should have to put no effort into understanding what’s going on at all. By definition, developers are being paid to solve complex problems. This means that not all code is trivial, and not all code is instantly understandable. The maintainer must put some effort in.
Writing off one liners as silly, or by their nature worse code than more code doing the same thing is incorrect though. To go back to the LINQ front… Which of these is more understandable by someone who has invested no time at all in learning LINQ. Which is more understandable by someone who knows LINQ well?
lowNumbers = from n in numbers where n < 5 select n;or
lowNumbers = new List();
foreach (int x in numbers)
{
if (x < 5)
{
lowNumbers.Add(x);
}
}
I would argue that given a small investment in learning their tools, the former one liner is far simpler to understand. It neatly says “give me all the numbers that are less than 5″, while the latter says “here’s a new list, we’re going to loop over the old array, and we’re going to test each element for being less than 5, when we find one we’re going to add it to the list”. The former explanation (at least in my mind) is far clearer.
I don’t know squat about LINQ, and I find the one-liner much clearer and easier to understand. This might be a particularly easy example,since I don’t know LINQ, but I could just read it through and understand what it was doing. The second example, I had to think a bit more at each step.
I’m not sure where this fits in, but I have seen code which I consider bad which doesn’t fit any of the above situations — as an example of badness, a single SQL table had column names in uppercase, lowercase, camelcase, and those three again with underscores. And then because a comment with a parent and comment without a parent are different things, they were stored in different tables — and what was CommentTitle in one table was comment_title in the other. The code was easy enough to understand, and aside from the bugs it ran fine, but it was an absolute nightmare to maintain and extend.
Absolutely – bad code does exist. My post is trying to highlight that an enormous amount of code gets highlighted as bad, and ratted upon when it’s no worse than any other (including the ratter’s own code).
Right now I am performing a lot of code reviews on algorithm code written by junior developers. What I can say is that Linq applied to algorithmic data structures does not force people to think about good, halfway performant design (I’m no talking about queries on your business logic objects). You need to perform some lookup on a set of objects? You need to acces objects in some order? Just use a plain old list and a Linq oneliner. Done, and they have even done it very elegantly
Better solutions for performance-critical code (often) are to maintain sorted lists, dictionaries, and the like.
I theorize, that if people had to implement this in an imperative way, they would much sooner recognize that they are coding causes a large number of iterations (potentially nested) over sets, large number of comparisons, etc. simply by the fact that their code looks complex.
Or in other words if my theory holds up then I should be able to argue the reverse, that if you can write your algorithm elegantly in an imperative way, chances are much higher you achieved a decent design. An iteration over a list to find an object looks more complex than a look-up via a dictionary.
Let me know what you think.