Coding

Broken glass


Imagine, for a moment…

You’re the new guy at BlokkStone Games, and you’ve had a pretty good first month. You fit in with the other programmers, and your team has made some excellent progress towards releasing their new title: Carolina Jess And The Temple Of Gloom. It’s a retro-graphics cell-based adventure game that is looking pretty sweet.

Now occasionally – when a player is hit, or an enemy dies – you need to add a glow to a shape. You do this by calling an internal library function written long ago: “addGlowToShape()”. Everything was working fine… until you tried using shapes that weren’t exact squares. Everything still ends up being an exact square, every time!

Dutifully, you dig up the library file, find the function in question, and are greeted with… this:

/**
        add a glow to a shape
    */
function addGlowToShape(&$shape) {

    // getstartloc
    $x = getStartX($shape);
    $y = getStartY($shape);
    global $world;
    $cells = $world->getCells();
    $shape_name = $shape->getName();
    $cellsAffected = 0;
   
    // do it all
    for ($rowY = $y; $rowY < $y + $shape->getHeight(); $rowY++) {
        for ($rowX = $x; $rowX < $x + $shape->getRowLength($y); $rowX++)
            addGlowToCell($cells[$rowX][$rowY]);
            $cellsAffected++;
    }
    
    // @todo: return false on an error
    return $cellsAffected;
}

Augh! Oh god! What is… Is that comment supposed to mean anything? And… and you don’t even use $shape_name! What is going on in there?

This, my friends, is broken glass. Broken glass all over the place. And I bet you it didn’t start out like this. I bet you it started out very simple, very maintainable – like everyone’s favourite pet code – and then evolved into this horrid mess. But how, you ask? Easily. Someone broke a window.

The Broken Windows Theory

Alright, enough dancing around metaphors here. Let me explain what I actually mean.

In 1982, social scientists James Q. Wilson and George L. Kelling wrote an article containing the following example:

Consider a building with a few broken windows. If the windows are not repaired, the tendency is for vandals to break a few more windows. Eventually, they may even break into the building, and if it’s unoccupied, perhaps become squatters or light fires inside. Or consider a sidewalk. Some litter accumulates. Soon, more litter accumulates. Eventually, people even start leaving bags of trash from take-out restaurants there or breaking into cars.

So influential was this article that it was used to justify a crackdown on vandalism, fare-dodging, and lead to all kinds of “zero-tolerance” policing in the city of New York. The theory was that if the police cleaned up all the “broken windows” (i.e. worked tirelessly to clean up the streets and absolutely prevent lesser crimes), that the overall crime rate would improve. And from 1984 (when the program began) onwards, the rate of serious crime fell dramatically. Today, the City of New York City is not the scary, drug-and-disease-infested hellhole predicted in 1980’s movies. It’s not even comparable.

This is known as The Broken Windows Theory, and it is easily applicable to programming as well. In fact, it could be rewritten as such:

Consider a function with a few coding standard violations. If the violations are not cleaned up, the tendency is for maintainers to ignore a few more standards while editing. Eventually, they may even leave unused variables from previous revisions, or forgo commenting entirely. Or consider a class. Some uncommented functions accumulate. Soon, more functions are added with no comments to describe them. Eventually, people even start copying and pasting whole sections of uncommented code elsewhere.

Sure, it’s a bit of a reach, but I’m sure you’ve all seen some version of it happen in the projects you’ve worked on.

See what happens?

Okay, so back to our example. We can do one of two things here. One, we can just slip in quietly, fix the bug, and get the hell out of there. Or two, we can clean up the function while we fix the bug. Let’s just see what happens.

First, we just fix the bug:

/**
        add a glow to a shape
    */
function addGlowToShape(&$shape) {

    // getstartloc
    $x = getStartX($shape);
    $y = getStartY($shape);
    global $world;
    $cells = $world->getCells();
    $shape_name = $shape->getName();
    $cellsAffected = 0;
   
    // do it all
    for ($rowY = $y; $rowY < $y + $shape->getHeight(); $rowY++) {
        for ($rowX = $x; $rowX < $x + $shape->getRowLength($rowY); $rowX++)
            addGlowToCell($cells[$rowX][$rowY]);
            $cellsAffected++;
    }
    
    // @todo: return false on an error
    return $cellsAffected;
}

Not so bad. I mean, we fixed that bug – we’re checking the length of each row instead of just the original row. But that function is still a mess for the next person to come by. Why don’t we try cleaning it up?

/**
 * Add a glow effect to all cells in a shape.
 *
 * @param ModelShape $shape The shape to add the glow effect to.
 * @param array      $cells The cells from the world to change.
 * 
 * @return mixed The number of cells updated or false on error. 
 */
function addGlowToShape($shape, & $cells) {

    $start_x = getStartX($shape);
    $start_y = getStartY($shape);
    
    // Get the row to stop at
    $end_y = $start_y + $shape->getHeight();
    
    $cells_affected = 0;

    $success = true;
   
    for ($cell_y = $start_y; $cell_y < $end_y; $cell_y++) {

        // Get the cell to stop at for this row
        $end_x = $start_x + $shape->getRowLength($cell_y);

        for ($cell_x = $start_x; $cell_x < $end_x; $cell_x++) {
            $success &= addGlowToCell($cells[$cell_x][$cell_y]);
            $cells_affected++;
        }
    }
    
    return $success ? $cells_affected : false;
}

See? See what we did there? We fixed the actual bug, did the todo, and fixed another, subtler bug that was hidden until we started going in and doing simple things like adding missing brackets.

Granted, yes, this is also a significant change that you'd need to go over with your team - we changed the signature which means we need to change everywhere that it is called from. But that's why any modern IDE will have a global find-and-replace in it. Or even just a global find-and-mark if you're the paranoid type (and I am).

And the second-order effects from this are enormous. The next person to approach this method will likely see that it has been cared for, and not want to be the first person to mess things up. Code quality goes up, code comprehension goes up, and bugs go down.

So the next time you see "broken glass" in your project, don't just walk around it - clean it up.

Coding
Gulp.js – an AMAZING build system!
Coding
Code faster with simple Sublime Text improvements
Coding
Rage-quit support for fish shell
There are currently no comments.