Pet Peeve of the Week: Commented Code

My fellow engineers at Amie Street can attest that I am full of almost inexplicable hatred for some very specific things. Today I would like to rant about my hatred for commented code.

By commented code, I don't mean the good kind of commented code. You know, the kind where it answers the questions you have about how something works but doesn't ramble on. I love this kind of commented code and try to write it wherever possible. Good commented code looks something like:

    /**
     * Parse a color parameter in the request. Expected format is
     * six digit hex (like in HTML but without the #)
     */
    private function getColorParam($request, $param, $default='FFFFFF') {
...

or maybe:

        // Copy topleft corner
        imagecopy(
            $targetIm, $templateIm,
            0, 0,
            0, 0,
            $slice_l, $slice_t);

Today I'm ranting about the bad kind of commented code. You know, the kind of commented code that is actually code that has been commented out. It looks like this:

public function newReleasesAction($request) {
    /*
    $n = date('N', time());
    if($n==2){
           // Its tuesday
        $dateString = date("F jS", time());
    }
    else{
           $dateString = date("F jS", strtotime('last Tuesday'));
    }
    */
    $dateString = date("F jS", Amie_DB::getUnixTimeForNow());
    $this->stash('newReleasesDate', $dateString);

Note that the majority of the above code has been commented out with a block comment. Rather than answering questions, this kind of insidious "comment" leaves you with more. Mainly, why is it commented out? The developer trying to read this code can make several different guesses:

• The code isn't necessary anymore - Maybe this code used to be important, but the variable it's setting wasn't used anywhere so someone commented out.


• The code didn't work properly - the commented code was causing problems, so someone replaced it with the line below the comment block. The replacement code may or may not do the same thing as the original, but the developer needed to patch the issue to get the page to load, or whatever.


• The code was commented out for debugging purposes and someone forgot to uncomment it. This kind of thing happens all the time - how often have you made a commit with random error logs like "Got here" in your codebase?


• The code was replaced with a better version (or refactored) but someone had an emotional attachment- for some reason some developers are afraid of the delete key and the rm command.

Frankly, every one of these reasons is stupid. One by one:

The code isn't necessary anymore

Delete it! If you ever find that it becomes necessary again, this is why you use version control.

The code didn't work properly

Commenting it out just risks that someone else will come and uncomment it again at a later date. If you don't know how to fix it, you should replace the broken code with something like "/* TODO: the algorithm that used to be here had a flaw. See bug #234291. Replaced algorithm with hardcoded data below */" or something of that nature.

The code was commented out for debugging

Can't say I haven't done this plenty of times myself, but before you commit it's good practice to do a quick diff to make sure you haven't inadvertently added useless log messages or commented out code that should be working.

The emotional attachment

Get over it! If you ever want to admire your old work you can always check out an old version of the code from the repository. It's pretty easy to pull back old code, but being a code packrat is kind of like taking your leftovers at dinner, putting them in a shoebox, and shoving them under your bed in case you're hungry in a couple of weeks. When you come back to the dead code it will be full of bugs that have grown since it was last used.