Braces and Scribbles

Some people may think I have a perfectionism problem.  It all depends on whom you ask and what it relates to.  I am a computer programmer both by trade and hobby.  In the years I have spent in my chosen field I have learned a lot about it and have encountered endless amounts of code written by others.  One of the biggest pet peeves I have always had is poorly done code.  There are two aspects of this I would like to discuss: improperly formatted code analogous to the scribbles of a 2 year old child and illogical or non-reusable trash code.  While I don't mean to offend people most of the time I believe that if you are a programmer that falls into one or both of these categories you should sincerely reexamine your methods and try to change for your fellow programmers' sanity.

Problem #1: Code Vomit

Take a look at this code sample:

function whatsMyName($name) {


    if ($name== "Dick Grayson") {
       $name = "Nightwing";
        }

      return "Hello, {$name}!"
  ;

}

Did you notice anything wrong with that code?  Is it readable?  I know it's a simplistic example and much easier to read than many other things I've seen but the principle still holds true.  That code has excess whitespace everywhere, misaligned code, inconsistent operator separation, etc.  I see code like this more often than I would like and it drives me crazy.

Now take a look at this sample of the same code:

function whatsMyName($name) {
    if($name == "Dick Grayson") {
        $name = "Nightwing";
    }

    return "Hello, {$name}!";
}

Isn't that much more readable?  I know some people can't be bothered to care about formatting while they're trying to think through some solution to their programming task at hand but the unreadable code I see often has a secondary problem.  Many times where I see such illegible code I also see spaghetti code: code riddled with a complex control structure that has not been well thought out.  This sort of code reminds of an XKCD comic titled "goto":

...which leads me right into the second aspect I want to discuss.

Problem #2: Brain Malfunction

Okay, this will be an extreme example but it is a real example from a project I worked on and I'm sure some fellow programmers have seen code that is just as bad if not worse.  There is actually a site dedicated to IT and code perversions called The Daily WTF for those who haven't heard of it before.  Anyway, on to my example:

The task at hand is to create three simple dropdowns for selecting a month, day and year.  As part of the requirement you need to automatically have the current date or a previously stored date selected.

Bad Approach:

$currentDate = date("Y-m-d");
$currentYear = substr($currentDate, 0, 4);
$currentMonth = substr($currentDate, 5, 2);
$currentDay = substr($currentDate, 8, 2);

echo "<select name=\"month\">";

if($currentMonth == "01") {
    echo "<option value=\"01\" selected>January</option>";
}
else {
    echo "<option value=\"01\">January</option>";
}

if($currentMonth == "02") {
    echo "<option value=\"02\" selected>February</option>";
}
else {
    echo "<option value=\"02\">February</option>";
}

if($currentMonth == "03") {
    echo "<option value=\"03\" selected>March</option>";
}
else {
    echo "<option value=\"03\">March</option>";
}

...<snip>...

Okay, I really didn't want to put the whole thing there - I think (hope) you get the point. If you, as a programmer, don't see anything wrong with implementing the task this way then you might want to re-evaluate your chosen occupation. This is a real example of some horribly inefficient/ridiculous code I have seen before. There is a significantly better way to implement the same feature and when I did so I slimmed a single 2000+ line file down to something more like 50 lines of code.

Better Approach:

$currentDate = time();

foreach(range(1, 12) as $month) {
    $month_name = date('F', mktime(0, 0, 0, $month, 1));
    $selected = $month == date('n', $currentDate);

    echo "<option value=\"{$month}\""
        . ($selected
            ? " selected"
            : ""
          ) . ">{$month_name}</option>";
}

...or sometimes (not debating whether this is good practice or not) I will even put code inline if the variables won't be used for anything else:

$currentDate = time();

foreach(range(1, 12) as $month) {
    echo "<option value=\"{$month}\""
        . ($month == date('n', $currentDate)
            ? " selected"
            : ""
          ) . ">"
        . date('F', mktime(0, 0, 0, $month, 1))
        . "</option>";
}

Taking this approach requires far fewer lines of code and is significantly easier to edit and adapt. Code such as this is much more reusable as well.