Developers and “bad” code

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.

About these ads

12 comments on “Developers and “bad” code

  1. Julia says:

    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.

    • beelsebob says:

      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.

      • Garson says:

        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.

  2. Shish says:

    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.

    • beelsebob says:

      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).

  3. Christoph Wienands says:

    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.

  4. Bill says:

    “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.”

    I disagree. Usually I can figure out what code does. What I want from comments is WHY the programmer chose to do something a particular non-obvious way.

  5. Anton says:

    Dear Sir!

    You and your friends are totally wrong, it’s not your arguments, its your attitude. Debating over which code-monkey is the better is a losing game. As for oneliners I can honestly say that I have never seen a oneliner that was maintainable. It’s like you said, take a piece of code and compress it. Well for many reasons that is not an good idea, but for readability fewer lines is never a good thing.

    Stop sissying about with your comments and come clean about one key element of development: Simplicity is everything. Now all you have to do is ask yourself which is the simpler: One line of regular expression or the 20 or so lines of procedural code.

    • Bill says:

      “for readability fewer lines is never a good thing”

      I don’t agree with this statement. It all depends on context. There are many times when using a particular concise expression is more readable — for someone who is competent in the language/environment. Who is doing the reading matters: a beginner or a professional. Just because something is difficult to learn does not mean it isn’t good. Here’s a simple example:

      char* strcpy( char* to, const char* from )
      {
      while( *to++ = *from++ )
      ;
      return to;
      }

      This is much easier to understand for an experienced programmer than the expanded “simpler” equivalent code.

      I do agree that compressing lines just because you can isn’t good. For example:

      if( (answer = myfunction( x, y, z ) )

      should probably be written on two lines as:
      answer = myfunction( x, y, z );
      if( answer )

      especially if myfunction has a longer or more complex list of arguments.

      In programming, as in writing, say what you mean directly and simply. Don’t intentionally try to be clever, but don’t “write down” to your audience either. Assume the maintainer is competent in the tool set being using.

  6. Anton says:

    I am almost compelled to say I stand corrected, because you seem to have an sincere goal of actually doing something good for the developing community. So lets elaborate a tad on the subject at hand.

    First, I would like to give some input on my background so you can understand my background. Having spent spent some 15 years as a developer (consultant) I have had ample opportunity to look at some really bad and some really good systems.

    It depresses me a lot to see, too often that a system is chucked in the bin and replaced by an almost exactly twin. The system that prevails over time and thus honors its creators are often the simple system. The more ‘advanced’ systems…well no one has the inclination the expand on them as they are too tedious to maintain and often you will have to rewrite so much of it anyway.

    When I say simple, I indeed support the idea of ‘writing down’ to the reader and fully enforcing the KISS rule. Who am I to believe that the next developer is competent in the language/environment?

    Therefore I conclude that ‘Good’ code is, roughly speaking, an expansion of a few lines of the aforementioned compressed oneliner.

    This is especially true in an SQL statement where the readability of the code is tenfold if you divide your composed statement into several small parts.

    This discussion, as refreshing as it is, merely scratches the surface of the issues of building good systems. The make or break factor is never about the syntax but of the complexity of the system itself.

    Therefore a lot of idiots buy into whatever pattern is popular for the day, OO, multi-tier or the most recent one MVC. With that kind of arsenal at your disposal you can get into a lot of trouble. Just to explain a little on what I mean I would like to state an example.

    Imagine a new technology that is coming, you are excited about building some system in it. This is your first time. Do you think it will be good? No, probably not. You actually wont build it ‘good’ until you build it for the third or fourth time. The nature of most programmers however is to constantly improve and therefore the are constantly moving on to the next pattern instead of excelling in one pattern.

    In my opinion therefore a straight procedural system is always better over any other pattern, giving you ample time to reflect on what problem to solve instead of the inner workings of an architecure that goes sideways from the the organization from day 1 it is implemented.

    This is not my humble opinion it is the facts of how it works in reality.

    Thank you for your time and Godspeed with your next system.

    /A

    • beelsebob says:

      I think you and I are rather violently agreeing in some ways. In general, the point both of us are trying to make is that it’s good to “write what you mean”, that is, the code should read like the english description of what it’s doing and why. Your example of a mega-regex is almost certainly a time when a one liner should be split out into more, clearer code; on the other hand, the LINQ example I give in a comment above, I’d hope you’d see is much clearer in one line.

      You also touch on one of the comments that prompted me to write this a while ago – that someone asserted “this isn’t SOAP, therefore it’s bad” – as you rightly point out, what they really mean is “this isn’t written in the flavour of the month, or isn’t written how I would choose to do it today”, and lacks any understanding of why it is written the way it’s written.

      Where I disagree is that a straight procedural system is always better… It’s often very clear, but then, MVC has its uses, functional programs are often significantly clearer in their intention (though often not in their runtime characteristics).

  7. Anton says:

    Dear friend. I know officially proclaim a truce between you and me. Its obvious that we agree
    on to much stuff to bother with a fistfight.(BTW: You are correct on the LINQ)

    Upon leaving this great blog I would like to take the time to discuss a EMS (yes you may google it)
    I recently designed and implemented in little over 3 months from ground up. An EMS is a highly sophisticated and advanced piece of software that is pretty fucking far from the day-to-day programs that mostly never does anything more important than fetching rows in a database based on some key.
    If someone had told me that they designed such a system AND delivered it to production in less than 4 months I would definetely call them a liar, because it simply cant be done.

    But I and two more guys did it.

    On the first day, when I met the team I told them the basics: This system is gonna be a straight procedural system, simply because we have no time to wonder about what kind of objects we should use to represent real-world entitys. If I see a class definition anywhere I will delete it.

    You see, most ‘modern’ day programmers have gone completely bananas over all the patterns that exists that they actually forgot that it is possible to create really great apps with really little code.

    If you want to that is…Just have a look at the app-industri. These guys that dont have access to the big supermotor that a pc is, and actually have to household with what they got wouldnt get far if the tried to build some of the monsters that is the result of a pattern such as MVC.

    In my opinion if you use MVC you are an idiot.

    Because you are writing code that never had to be written in the first place. And that statement is also true for any other pattern that exists out there like OO and multi-tier etc. And I am probably the biggest idiot of all for having believed in these patterns and actually built a whole lot of systems in them.

    They are wrong and dont work and thats it. No need to cry about it, just forget them

    My 0.02$

    Best wishes

    /A

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s