Writing maintainable code

Three years ago, I gave my first talk at Yahoo! entitled, Maintainable JavaScript (slides). The point of the talk was to encourage people to use more rigor in their JavaScript coding. A lot of people who write JavaScript for a living began as hobbyists and hackers, including myself. All of the best front end engineers are self-taught, of course, because this stuff hasn’t been taught in colleges and universities. I took the opportunity to simply point out that the same coding practices that are taught in traditional software engineering programs can also be applied to JavaScript. And of course, I threw in some of my own personal findings gathered through years of web development.

What is maintainable code?

In the presentation, I said that maintainable code has the following qualities:

  • Understandable
  • Intuitive
  • Adaptable
  • Extendable
  • Debuggable

I’d also now like to add “Testable” as a sixth quality of maintainable code. As I thought about this more recently, I realized that all six of these qualities really boil down to a single concept: don’t be confusing.

Confusing code doesn’t embody these qualities and makes everyone’s job harder. Over the years, we’ve become better at identifying bad code. Bad code causes runtime problems, whether they be actual thrown errors, performance bottlenecks, or accessibility issues. Bad code is typically identified by bugs that require spot changes to code for a remedy. Confusing code is more insidious.

It’s hard to uncover confusing code without context. Any single line or series of lines of code, when examined in a vacuum, is confusing. Context is what determines if that code is confusing or not. That necessarily means that the only way to uncover confusing code is by thorough review.

Anyone who’s worked with me knows my passion for code reviews, as I believe they are the best way to not only catch more esoteric issues but also as a way to socialize best practices within a group. Code reviews are never at the top of anyone’s “to do” list, but they are vitally important. When confusing code is shown in the light of day, it’s much easier to identify and fix. What’s more, when a group of people are looking at confusing code, they can all agree that it’s confusing and come up with a common way to fix it.

Confusing JavaScript

Confusing code comes in many forms but has an overriding quality: it’s hard to tell if it’s intentional or a mistake. JavaScript is the easiest of the three (JavaScript, CSS, and HTML) within which confusing code exists. A classic example:

switch(type){
    case "string":
        handleString(value);
    case "number":
        handleNumber(value);
    default:
        handleValue(value)
}

This code looks innocuous enough. Decent engineers will look at this code and say, “hey, each case statement is missing a break.” If you were fixing code in the same file, you might even be inclined to help out and just add a break after each case. But are you sure there’s an error here? How do you know that the developer didn’t intentionally omit the break in each case? There’s really no way to tell, so you could be creating a bug by fixing this code, but for all you know, this code could be already causing a bug that you could fix. This is confusing code.

How do you make it into good code? By providing context. In this case, the surrounding code doesn’t provide enough context, so adding a comment is the best way to go. For example:

switch(type){
    case "string":
        handleString(value);
        /*falls through*/
    case "number":
        handleNumber(value);
        /*falls through*/
    default:
        handleValue(value)
}

This code is much less confusing. You know that the intent is for each case to fall through to the next, so you won’t accidentally fix this code when you come across it. Further, if your team agrees that this is the pattern to use in these situations, then you know that each case statement must be terminated by a break, return, throw, or a /*falls through*/ comment. If a case statement doesn’t end with one of those, then it’s likely an error and should be filed as a defect.

JSLint

In case you somehow haven’t heard, JSLint is a tool created by Douglas Crockford to validate JavaScript code. It’s described as a tool to help identify bad code, which it does, but it also identifies confusing code. In fact, it identifies more types of confusing code than it does bad code. What exactly qualifies as confusing code is subjective, and Crockford and I are not in agreement on 100% of the things that JSLint points out, but this is still the best tool available to help identify potentially confusing code in additional to bad code.

Confusing CSS

Don’t be fooled, confusing code can exist in the other layers of a web application as well. CSS has some interesting syntactic issues that could lead to confusion. For example, the padding property can have one, two, three, or four parts to its value:

/*same padding all around*/
padding: 10px;

/*padding for top/bottom, different padding for left/right*/
padding: 10px 20px;

/*padding for top, different padding for left/right, different padding for bottom*/
padding: 10px 20px 15px;

/*different padding for top, right, bottom, and left*/
padding: 10px 20px 15px 25px;

Some will say that all of these are fine and not confusing. Personally, I find the third option quite confusing, as it’s not clear that you intended for a different bottom padding. You might have meant to use two or four parts. This form is also the least intuitive of all the options. There’s a couple of easy ways to disambiguate. The first is to agree to always use one, two, or four parts for properties such as padding. This has the pleasant side effect of really making you stop and think if you need just one dimension to be different. Here’s how this looks:

/*Don't use*/
padding: 10px 20px 15px;

/*Better*/
padding: 10px 20px 15px 20px;

Even though you end up using the same value for the right and left parts, I’d argue that it’s easier to tell that the result is intended. Another option is to always use the specific padding property for the one-off dimension, such as:

/*Don't use*/
padding: 10px 20px 15px;

/*Better*/
padding: 10px 20px;
padding-bottom: 15px;

Both this and previous example have the advantage of making this decision explicit: you meant to change just one dimension of padding and therefore, it must be correct.

Beware of confusing code

Confusing code is the second-worst (next to bad code) type of code to have in your source because it can introduce subtle errors that can go unnoticed for long periods of time. I like to say that code is like rabbits: it multiplies when you’re not looking. If there’s one instance of confusing code in your system, it doesn’t take very long before there are two. That’s because of the natural flow of a software project. Someone is looking for an example of how to do something new and they come across the confusing code. The confusing code gets copied and now there are two instances in the source. The next time someone goes looking for an example, they’re twice as likely to find the confusing code as an example (and of course, finding two examples of the same approach validates the approach in the seeker’s mind).

The same is true with good, maintainable code. The more examples of good code that are in source, the more likely that others will copy it. And that’s what you want.

Comments

  1. J. Albert Bowden II

    i don't see what's confusing about using three values in declaring a margin property in css. the rule of three is 1st top, 2nd sides, 3rd bottom. there's nothing complex about it. your work arounds are fine, but you are re declaring those values in the browser which is not optimal.
    i'm a big fan, i'm not going to say that you're wrong or this or that. I just think maybe you could get a better example for how to use code properly. i don't doubt your expertise, but because you find something personally confusing, that is clearly detailed in the spec...isn't that on you?
    if i'm way off, please let me know.
    always a fan.
    albert

  2. Philip Tellis

    I just tell people to read the Elements of Programming Style by Kernighan and Plaugher.:

    http://en.wikipedia.org/wik...

    For those short on time, I have a fortune cookie file of all the tips:
    http://db.glug-bom.org/lug-...

  3. richtaur

    I also get confused when functions fire immediately but you can't tell by looking at the first line, like:

    function() {
    // this code runs immediately
    }();

    I prefer:

    (function() {
    // this code runs immediately, and the first ( helps imply that
    })();

    This is becoming a common practice but I still see the former used, and it throws me off.

    I also like to supplement my CSS with extra helpers. Sometimes I'll have code like:

    #foo {
    z-index: 1000; /* arbitrary; but must be higher than #bar and lower then #fubar */
    }

    Otherwise one might think that 1000 is the exact number needed when reading later.

  4. aarontgrogg

    Nick:

    I love the principles of the article, but I think the JS example lacks the enforcement you're trying to make, what else would a switch do but /* fall through*/?

    And I completely disagree with the CSS example. While I agree that it might not be obvious what you wanted, the outcome is either going to be right or wrong on the page. If it's right, no one will ever come looking at your CSS; if it's wrong, someone will do what they need to do to fix it. I find it FAR more confusing when I come across someone else's CSS and find something like padding:10px 20px; followed somewhere later by padding-bottom:15px;. You've no idea the amount of time I've spent changing values in the first declaration, getting frustrated at it "not working", only to later find that other declaration... The danger, to me, is that the two can very easily get separated by edits and become very difficult to debug later...

    Again, love the principles, just think better examples would drive the message home better.

    Peace,
    Atg

  5. Nicholas C. Zakas

    @Atg - The other option in the switch is to break. The point of that example is that without an explicit indication as to what was intended, it's easy to falsely assume there's an error.

    Choosing examples is always the most difficult part of technical writing. I fully understand that some people may not agree with my choices or my definitions, as in your case. I refer you to the last line of the intro: "... when a group of people are looking at confusing code, they can all agree that it’s confusing and come up with a common way to fix it."

    Also, my name is Nicholas, not Nick. :)

  6. Omid

    Totally agree with you, the other widely used but confusing and unadaptable CSS coding is the ignored values, which are assigned but one line later are ignored by adding another style.
    Example:

    padding:10px;
    padding-bottom:5px;

    or

    border:1px solid #cccccc;
    border-left:2px solid #efefef;

    What do you think if it's going to be ignored in second line, well it can be written so:

    padding:10px 10px 5px 10px;

    By the way very smart article and necessary points for coders.
    Thanks

Understanding JavaScript Promises E-book Cover

Demystify JavaScript promises with the e-book that explains not just concepts, but also real-world uses of promises.

Download the Free E-book!

The community edition of Understanding JavaScript Promises is a free download that arrives in minutes.