ByOn February 27, 2013 At 2:24 pm
Responses : No Comments
One of the tools we use when developing production web services is a script that runs nightly to compile a list of any errors in the webservers’ error logs, and email anything abnormal to the developer list. This often helps us to catch broken links, uninitialized value warnings, and code paths that we may have missed in testing. Over the years, we’ve seen our fair share of issues come and go, but on one particular morning, I opened the email, and was greeted with the following:
sh: York: not found
Now, this is quite possibly the greatest line I’ve ever seen in an error log. It’s four simple words, it makes no sense, it communicates that arbitrary shell commands are being executed somewhere, and there is absolutely no way to search the codebase for it. It’s a beautiful nightmare in its simplicity.
After several hours and three smashed keyboards, we found the offending code. It was a mess. The page had started its life as a tangled web of hacks, and every modification added more on top. Eventually a second copy of the page had been created, and modified to work in a slightly different manner. When the second page failed to find the necessary data, it fetched the data from the first page–using a command line call to curl, with the “state” argument passed to the command line. So any two-word state would attempt to execute the second word as a separate command, hence “York: not found” when the Empire State is selected.
“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.”1
Now, consider a project with poorly written, unreadable code. Variables have names like
$bar, spacing and indentation styles vary every few lines, and large blocks of code are repeated multiple times throughout. It is obvious that some of the functions don’t work, but as far as anyone knows, they never get called anyway. There could even be a few
goto statements hidden away in the codebase’s dark alleys. Everything just looks like a previous developer came along and vomited code into the repository. In the absence of directed code refactoring, the tendency is for developers to ignore the need to write elegant code and continue to write in the same style as the rest of the project. If nobody else cares, why should they? They continue to leave the code unindented. They use short, unintelligible variable names. They continue copying and pasting blocks of code, instead of creating function calls. Comments are completely out of the question.
Eventually, some minor bugs are discovered. In the spirit of the project, they’re fixed with yet another hack. Since nobody is really sure what most of the code does, each new revision creates a few more bugs. There are innumerable potential security exploits. Because all the code is so entangled, it’s impossible to refactor in small pieces. Nobody is sure what the original purpose of the code is, or if it does what it was supposed to do in the first place. Some people will argue that, as long as it works, who cares what it looks like? The users can’t see the code, so it doesn’t matter to them if there are five different indentation styles used between nearly identical functions that have been copied around the codebase at random. There are plenty of arguments against this mentality that I won’t repeat yet again, but there is one downside to writing bad code I want to stress: it leads to more bad code being piled on top, even by developers who wouldn’t normally do that. And after enough garbage accumulates, someone sets it on fire.
The developer who made the changes in the earlier example wrote good code when working on modules that were better organized. But, in this situation, he added to the pile of garbage and that lead to a potential exploit being pushed live. He thought to himself, “well, there are broken windows here, I guess I might as well commit a crime.” And this certainly was a crime.
Yes, the need to use “best practices” must be balanced against deadlines and business needs, but once you’ve developed good coding habits for yourself, it doesn’t usually take much longer to do things right. And it may save you the headache of fixing a problem caused by someone else who assumed that nobody cared.
1. James Q. Wilson and George L. Kelling. “BROKEN WINDOWS: The police and neighborhood safety” Retrieved 2012-05-16.
This post was originally published on: http://omniti.com/seeds/broken-windows
Posted by Corey Cossentino
Corey Cossentino is a Web Engineer at OmniTI. He focuses on architecting and developing scalable web applications that can be quickly and easily adapted to industry's changing needs.