Every good programmer has gone through this. You discover a new tool, and it seems shapely and fit for purpose. You start using it, tentatively at first, gradually getting more and more used to its quirks and features. Over time, trust between you grows, and your casual friendship blossoms into something deeper. The program becomes part of that sacred subset of utilities you can't imagine yourself without. All is bliss... Then, one day, you decide to look at the code. Maybe you want to extend it, maybe you're just curious. The moment you fire up your editor on the first source file, you sense that something is wrong. Without reading a line, you notice a certain visual complexity to the code - something to do with deeply nested and over-long functions. Looking closer, you quickly realise that tangles of ifdefs snake through the source like a canker. Weird indentation and non-idiomatic constructs are everywhere. The project's structure sucks - there's no proper component isolation, its innards are a nest of subtle and devious co-dependencies. Beneath the skin of the streamlined program you thought you were using lies a grotesque, bloated, unmaintainable monstrosity. You're heartbroken - you've trusted this tool for years, and now it betrays you like this. It was all a lie - nothing will ever be the same again...

I know from personal experience that this is a very traumatic process, so it's with great sympathy that I read a recent article by Marco Peereboom - an vocative and haunting lament with the poetic title "OpenSSL is written by monkeys". Marco modestly claims not to be a great programmer, but he is a contributor to OpenBSD, a project that has a frankly psychotic focus on code quality. So, lets see what a graduate of the OpenBSD Academy of Programming makes of the OpenSSL codebase, as illustrated by this illuminating extract:

#ifndef OPENSSL_NO_STDIO
/*!
 * Load CA certs from a file into a ::STACK. Note that it is somewhat misnamed;
 * it doesn't really have anything to do with clients (except that a common use
 * for a stack of CAs is to send it to the client). Actually, it doesn't have
 * much to do with CAs, either, since it will load any old cert.
 * \param file the file containing one or more certs.
 * \return a ::STACK containing the certs.
 */
STACK_OF(X509_NAME) *SSL_load_client_CA_file(const char *file)
    {
    BIO *in;
    X509 *x=NULL;
    X509_NAME *xn=NULL;
    STACK_OF(X509_NAME) *ret = NULL,*sk;

    sk=sk_X509_NAME_new(xname_cmp);

    in=BIO_new(BIO_s_file_internal());

    if ((sk == NULL) || (in == NULL))
        {
        SSLerr(SSL_F_SSL_LOAD_CLIENT_CA_FILE,ERR_R_MALLOC_FAILURE);
        goto err;
        }

    if (!BIO_read_filename(in,file))
        goto err;

    for (;;)
        {
        if (PEM_read_bio_X509(in,&x,NULL,NULL) == NULL)
            break;
        if (ret == NULL)
            {
            ret = sk_X509_NAME_new_null();
            if (ret == NULL)
                {
                SSLerr(SSL_F_SSL_LOAD_CLIENT_CA_FILE,ERR_R_MALLOC_FAILURE);
                goto err;
                }
            }
        if ((xn=X509_get_subject_name(x)) == NULL) goto err;
        /* check for duplicates */
        xn=X509_NAME_dup(xn);
        if (xn == NULL) goto err;
        if (sk_X509_NAME_find(sk,xn) >= 0)
            X509_NAME_free(xn);
        else
            {
            sk_X509_NAME_push(sk,xn);
            sk_X509_NAME_push(ret,xn);
            }
        }

    if (0)
        {
err:
        if (ret != NULL) sk_X509_NAME_pop_free(ret,X509_NAME_free);
        ret=NULL;
        }
    if (sk != NULL) sk_X509_NAME_free(sk);
    if (in != NULL) BIO_free(in);
    if (x != NULL) X509_free(x);
    if (ret != NULL)
        ERR_clear_error();
    return(ret);
    }
#endif

His objections boil down to the following:

• The indentation style is weird, and in many circumstances hard to parse.
• The project uses a mixture of CamelCase and underscore-based function naming.
• The error cleanup strategy is bizarre - using a goto to jump into code guarded by an "if(0)" is distinctly unlovely.
• In this example, the function name mis-characterises what the function actually does. The somewhat shame-faced comment doesn't fix the problem, it just makes it funny.
• The project suffers from ifdef-itis.
• Most importantly, the code does not "read" well. In this case, we find multiple levels of indirection, and no clear flow to the function.

So, while Marco's problem started with the project's shoddy documentation and API, his actual code criticism focuses on issues that are apparently superficial. He hasn't discovered a substantive bug or architectural weakness in the snippet above. Instead, what matters to him are simple virtues like consistency, style, and readability. Marco is saying, in fact, that the OpenSSL code sucks because it lacks superficial beauty. I couldn't agree with this position more.

I'm reminded of a recent blog post describing "the perfect interview question" for programmers: ask them what bothered them most when reviewing other people's code. The blogger argued that a response focusing on superficial code quality meant that the interviewee was obviously not an "architectural thinker", and was therefore a poor candidate. This is utter tripe. Good programmers know that a lack of superficial code quality and consistency is the best indicator of deeper systemic problems in a project. If you ever need a quick estimate of the quality of a codebase, this is what you should look at first. If you ever have to work on a project with poor code quality, fix the superficial issues first. Ugly code will obscure deeper architectural issues, increase defect rates, make code review hell, and make the project hard to refactor. This is advice so basic that it usually does not need to be given - good coders understand the importance of superficial beauty at such a deep instinctive level that they will feel compelled to fix cleanliness and neatness issues before working on deeper problems.

Superficial beauty is not something that is discussed nearly enough in the Open Source world, so I'm going to don my flame-retardant poncho, and name some names. In keeping with this post's starting point, I'm going to focus on projects in C. Lets start with the ugly. The codebase for Vim, a tool that I spend hours using every day, turns out to be a frightening and inscrutable thicket of #ifdefs. The Linux kernel is immensely variable in quality - some of it is very good, some of it - especially less widely used drivers - is unspeakable. The mutt codebase is pretty terrible, prominently featuring one of my pet bugaboos - mixing tabs and spaces, invisibly screwing up indentation depending on your editor configuration. The Wireshark packet sniffer - another project I use daily - is so bad that OpenBSD opted to remove it from their ports tree rather than encourage their users to use it. Wireshark wins a special prize for over-commenting. They've clearly abandoned all hope of communicating their intentions through the code itself, degenerating instead to things like this:

/* Now bump the count. */
(*argc)++;

I'll end the post on a high note, with some examples of great code quality. OpenBSD is undoubtedly one of the pin-up projects of the Open Source world, featuring code that is almost supernaturally clean, consistent and direct. If you're interested in taking a look, I recommend starting with some of their recent daemon development - their SMTP and NTP daemons are good candidates. Another excellent project to look at is the C Python interpreter, which shares many of OpenBSD's virtues. Note that I mean the interpreter itself - the the standard library is unexpectedly variable in quality. A more obscure project with great code quality is the Plan9 operating system. Sadly, Plan9 never took off (perhaps because it wasn't free software from the beginning), but the codebase illustrates many of the sound principles outlined in by Kernighan and Pike - both of whom were involved in Plan9 - in The Practice of Programming.

edit: Meanwhile, over on reddit dagbrown has pointed out procmail, which turns out to be an absolutely unparalleled phenomenon. Go on, have a look - I dare ya.