Spaghetti code is a term we seem to hear a lot in the programming realm, but we need to make sure we know how to recognize it when we’re scanning code.

This last week I had the pleasure of helping someone with a web development problem on a certain new developer focused site. While I was able to solve this person’s issue for now, what I saw saddened and angered me. The code that this person posted was THE essence of spaghetti code.

Example of Spaghetti Code

First I want to say that I do not mean to call out the poster, I have total faith that he is not the original author. In general, it does not matter WHO wrote the code, a good programmer will take responsibility and fix this broken window unpinned grenade without a finger of blame.

Here is the code, you can see it in action here. This page has a bug needing solving… a <div> containing to total cost is not properly updated when the second <select> box is changed. Not a tough sounding bug, right? View the source and see if you can you spot it. It’s OK take your time… it took me all too long. This, my friend, is why spaghetti code is bad: it’s difficult to maintain.

What makes it "spaghetti code"

Sadly, to the untrained eye, this code might not look THAT bad. Notice at line 8 we see declarations for "Constant Variables used to identify array locations". If only there was some way for something to have properties which have values… instead we see everything bound to an array index. We haven’t made pasta yet, though.

The three deadly sins in this case are:

• Scripting logic is strewn throughout the page instead of being kept in one place
• The code is difficult to extend
• It is difficult to reverse-engineer, making it a maintenance nightmare

Another real problem is that there doesn’t seem to be any structure to how and when functions are called. updateFields() is explicitly called in every <input> except for one, can you find which one? You also may have missed the random <script> tag at line 252 as well. Just try following one of the change event handlers in either <select>.

What to do about it

In this case, frankly, I would hardly bother with refactoring. I’d be re-writing it, keeping it simple. Proper use of a map and one controlling function that delegated calculations would do wonders. That’s not the point of this article, though, so I’m not going to waste any more time on it.

It’s important that you don’t waste time deciding who’s at fault. It’s petty and it will not solve anything. As a good programmer, it is your job to recognize code like this, take responsibility for fixing it correctly, and setting up tests to make sure it (almost) never breaks. If your manager doesn’t want to fix "what’s not broken", ask which is worse, spend a bit of time now refactoring into something maintainable, or spending twice the effort when a bug is found.

You can wear a guard now or feel the pain and wear a band-aid later. Anyone else care to share any WTF spaghetti code?