How to make enemies

Every developer blames his or her predecessor. Whenever we get to work on, or maintain, a codebase that was developed by someone else, the first impression is always a negative one. Along with second, third, forth…up to nth. Someone else’s code is always worse than what we would’ve developed, given the opportunity. The design is wrong. The code is messy. There is insufficient documentation. The problem could’ve been solved in a more efficient way. Variable names too short. Variable names too long. Truth of the matter is — even when we look at our own code years later (and sometimes much sooner) we criticize it in exactly the same fashion. But after years of dealing with unrealistic deadlines, last minute changes in requirements, and “visionaries”, who cannot explain what they want, yet want it to work exactly the way they envision it, I’ve learned to make a distinction between “necessary evil” and complete incompetence.

Consider this:

$AWFUL_JS_HACK .= '';

At a first glance it looks … well … awful, as the variable name suggests. Variable named $AWFUL_JS_HACK in itself creates a feeling of impending doom, and along with a non-sensical mix of JQuery and Perl to find patterns in names of variables and manipulate the display, the code is not only unreadable, it's just a bad approach to solving a problem. The problem looks to require some reengineering of either data structure, or preceding code, which would've been much cleaner and easier to maintain. Yet, when I see something like this in a code, I am more sympathetic than pissed off.

Developer acknowledges that this code is an awful hack, yet still put it into production. Based on my own long standing expertise with awful hacks, I can say with a high level of confidence this code went in because of last minute change before launch or in response to a production issue that had to be fixed immediately. Does it contribute to technical debt? Absolutely. Is it worth it? Maybe. Could it also be that the developer was lazy and chose a quick way out of the problem? Of course. But at least there may be a valid excuse.

Significantly worse is this little gem:

CREATE TABLE users (
    userid integer primary key,
    email text not null,
    firstname text,
    lastname text,
    phone text, //partner
    ipaddress text,
    regdate timestamp not null default now(),
    actual_phone text //phone
);

I can envision a situation where the initial column phone was not used and, when the request came in, instead of creating another column partner, the decision was made to reuse the existing column. Maybe the developers proposed a plan to create a new column, and to modify the code accordingly, and product manager, or a client, mandated a quicker solution. Who knows. However, the approach is so inherently lazy and potentially damaging that it's hard (altho still possible) to justify.

And then there is code that makes you want to hack Social Security Administration servers, get previous developer’s home address, buy the tickets on the next flight out, knock on the door, and go Jay and Silent Bob on their ass.

const PROJ_CONSTANT_PLACEHOLDER_FOR_COMMA = ',';
const PROJ_CONSTANT_PLACEHOLDER_FOR_RIGHT_PARENTHESIS = ')';
const PROJ_CONSTANT_PLACEHOLDER_FOR_LEFT_PARENTHESIS = '(';
const PROJ_CONST_PLACEHOLDER_FOR_AMPERSAND = '&';
const PROJ_CONSTANT_PLACEHOLDER_FOR_SPACE = ' ';
.
.
.

I am not really sure if anything needs to be said about this … Even abstracting yourself from the idiocy of this approach — this is a definition of a lose-lose situation. You either use the constants to follow the code guidelines, wasting time and contributing to the world’s worst naming convention, or you don’t, and be responsible for breaking code consistency.

Short of examples similar to the constants one above, at the end of the day, I get much more frustrated by blatantly bad architecture decisions than by coding styles. Frankly, I get as annoyed seeing someone’s curly-bracket-per-line approach as someone who would have to review my ternary approach. I find curly bracket on the same line as condition to be much more readable, but there are plenty who disagree with me. And it’s ok. Those are minor annoyances that won’t go away, you will always think that your style/approach is better than someone else’s. However, there are certain coding and architectural decisions that are made in applications that cannot be justified by preferences or styles, like the constants example above. Those are just bad. Those are the ones that make you go “WTF?!” Those examples make you want to hurt people. And not metaphorically.

function unreserve {
    $cutoff  = date('Y-m-d H:i:s', (time() - (12 * 3600)));
    $sql     = "SELECT id, another_id AS another_id FROM carts 
                     WHERE timestamp < :time";
    $returns = $dbh->fetchAll($sql, array('time' => $cutoff));
	
    foreach($returns as $item) {
        //...do some operations on stale carts that have not been acted on
// ----- Reset inventory -----
        $dbh->execute("UPDATE inventory SET reserved = false WHERE id = :id", array('id' => $item['id']));
        // ----- Clear carts -----
        $dbh->execute("DELETE FROM carts WHERE id = :id",
	                      array('id' => $item['id']));
    }
}

Let’s look at the function above. Part of a large e-commerce site codebase, the function is intended to check carts that are older than 7 days old and release inventory for those items. n+1 problem has always been my pet peeve, so this little snippet doesn’t contribute to my appreciation of the predecessor. However, this piece of code doesn’t make the list because of n+1 problem. It makes the list because this function is one of the worst examples of bad engineering I’ve seen over decades of looking at all sorts of different codebases. And it’s not how it’s written. It’s how it’s tied into the overall process. The function is called from one place in the whole codebase, and that place is … *drumroll* …header.inc. That’s right, the code is executed on every page load. Every. Single. One.

I was considering leaving it on this very high note, since it’s difficult to present an example of equivalently bad judgement. Yet, I have to give a shout out to my DBA friends with this brilliant schema design choice.

mysql> desc sample_table;
+ — — — -+ — — — — — — — — — — -+ — — — + — — -+ — — — — -+
| Field | Type | Null | Key | Default | Extra |
+ — — — -+ — — — — — — — — — — -+ — — — + — — -+ — — — — -+
| val | int(10) unsigned | NO | MUL | 0 | |
| key | char | YES | | 0 | |
| id | float(8,3) unsigned | NO | PRI | NULL | auto_increment | 
+ — — — -+ — — — — — — — — — — -+ — — — + — — -+ — — — — -+
mysql> select * from sample_table limit 5;
+ — — — -+ — — — — + — — — — +
| key | val | id |
+ — — — -+ — — — — + —  — — — +
| foo | 1       | 1.000  |
| bar | 022908  | 2.000  |
| baz | 041770  | 3.000  |
| qux | 041996  | 4.000  |
| quux | 048477 | 5.000  |
5 rows in set (0.00 sec)

So in conclusion, remember — any code you write will be bashed. Perhaps even by future you. So make sure, when the time comes, it’s not bashed for a reason.