It was Brian’s first day at AutoDetective, a website for comparing listed car prices vs. blue book values. His work inbox was overflowing with style guides, best practices, and notes from the dozen or so other developers he would be working with. His interviewer, Douglas, had mentioned that the site ran on a substantial chunk of legacy code, but Brian had experience with plenty of old code.

He spent most of the day digging through the source, getting a feel for the in-house development style. It didn’t take long before he noticed how … off the code was.

An old car mouldering in a rotting garage

It wasn’t just legacy code. It was obtuse legacy code.

Just Don’t Touch It

Douglas came to debrief Brian at the end of the day. Brian explained how he spent the afternoon going through the codebase, looking for a project to get his hands wet. “There’s a lot of inefficient code,” he said. “I figured I’d fix something small.” He pointed to a bit of code:

if (isset($_SESSION['relogin_data'])) {
 $a_relogin_data = $_SESSION['relogin_data'];
 $arr_keys = array_keys($_SESSION);
 for ($i=0; $i < sizeof($arr_keys); $i++) {
 unset($_SESSION[$arr_keys[$i]]);  
 }  
 $_SESSION['relogin_data'] = $a_relogin_data;
}

“Why are we unsetting the entire array like this? I’ve pared this down in my local branch. I was about to send a pull request–”

“No,” Douglas said. “That’s our legacy code. We can’t change a single line in that.”

“But it’s inefficient–”

“It doesn’t matter. None of us who still work here understand this code. It’s like a house of cards: if we change something little, the whole thing could come crashing down.”

“Oh-kay.” Brian quietly deleted his local branch.

Stovepipes Attached to Stovepipes

Every day, when he finished his assigned trouble tickets, Brian would attempt to make sense of the huge lump of legacy code that no one in the company could ever touch. For instance, this ultra-robust array building method, which skips simple assignation for laborious key/value reassignments:

while ($row = mysqli_fetch_assoc($result))
{
 $array_keys = array_keys($row);
 $array_values = array_values($row);

 for ($i=0; $i < sizeof($array_keys); $i++)
 $dataset[$count][$array_keys[$i]] = $array_values[$i];
 $count ++;
}

The code below checks if there’s a connection already open with a given name. All database errors in the legacy codebase return false, so there’s no way to tell if it’s a bad password, a server outage, or trying to reuse the same connection name twice:

if (isset(DBClass::$open_connections_[$conn_name])) {
 // connection is already open
 return false;
}

Everything, and one means everything, was stored in $_SESSION. Dumping session data was almost useless with how much was stored in it on a regular basis:

public function getUserInfo()
{
 return $_SESSION['logged_in'];
}
$user_info = $login->getUserInfo();
$_SESSION['user_info'] = $user_info;

// ...

$_SESSION['logged_in'] = $this->a_logged_in_;
$_SESSION['view_mode'] = $_SESSION['logged_in']['view_mode'];
$_SESSION['user_info'] = $_SESSION['logged_in'];

Then there was the utterly useless, such as a defined destructor that only calls its parent (unnecessary in PHP):

 public function __destruct()
 {
 parent::__destruct();
 }

A Unit Test Too Far

One afternoon, Brian willed himself to Douglas’s office. When his boss asked what the trouble was, Brian pointed him to a bit of code he had been staring at for hours. “I was working on #75693, and trying to figure out where $this->SERIAL was getting set. It wasn’t in the class definitions, which would be natural. Instead, I found it in this out-of-the-way include.”

<?php
class MyClass {
 private $SERIAL = "";
 public __construct() {
 include(Loader::get("configuration file"));
// ... more code ...
 $this->db_loadData($this->SERIAL);
// ... more code ...
 }
}

// the included file:
<?php
switch (get_class($this)) {
 case "ClassName":
 $this->SERIAL= "H57-451";
 case "SecondClassName":
 $this->SERIAL= "H57-452"
// ... hundreds of case statements ...
}

Douglas started, “You know we can’t touch–”

“Right, I know, and I’m not. But how can we keep maintaining the codebase like this? Let me document this code. I’ll write some function comments, put together some unit tests based on existing behavior. That way–”

“We can’t, Brian. None of us have the time. I work with eleven other developers. None of them have the time to maintain unit tests, and none of them want to. And commenting the code might cause it to break.”

Dejected, Brian returned to his desk. The years of inefficient code he had witnessed ate at him. Like Sisyphus pushing his rock up a hill, he knew he’d never get a chance to fix all of it. But if he waited long enough … found the time to document it … he might get his chance. One day.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!