What spaghetti code looks like
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?
That’s not spaghetti, that’s crap :(
You forgot about the duplicated code, another maintainance issue.
this.time[breakfast][midweekcost] = 75;
@Orn: Agreed, it is crap, but there is recognizable spaghetti. There were lots of things I uh… bit my tongue on because I didn’t want to rant too long about how bad the code is.
You’re right though, it’s hard to say all the things that are wrong with it.
This is all nice, but let me tell you how this “rewriting” of the spaghetti code will go on.
You will change the code which is perhaps while written poorly still worked fine, replacing with your new shiny piece. You will even write a bunch of tests, and for a while you’ll feel very good about yourself (rightfully).
Now, after a while, those pesky sqe guys will find a few kinks in your shiny armor. After a while you’ll understand why some of the things that you removed with the “old crappy code” were there in the first place. By the time you get to fix them it will be to close to a release so you’ll have to do a few “small quick fixes” to patch it up. Then some more. And more.
After a while only you yourself will be able to recognize remnants of the original “totally new beautiful design”. For other people it will look like another piece of spaghetti code. The cycle will be complete. This is normal.
But the moral is: unless there is a real need to change some code - like fixing a bug, adding new feature, don’t do it. If you do change it, go in, fix the bug, and that’s it. Stuff like this should not be done for just for the sake of changing. Yes, please do listen to that manager that tells you not to touch it unless its broken - he may have seen all this before.
Dmitri
Dimitri did you write it or something lol. If you follow good coding practices and design patterns you wont reach the point of spaghetti code.
I agree its terrible code and something else that really peeves me is inline event handlers on elements.
I’ve had to work on spaghetti code before and in many cases I’m not being paid enough to re-write it all just to maintain it. The last time I did that I ended up ditching the project halfway through, it was just too much effort to try to fix someone else’s poorly designed code.
@Dmitri:
That is the kind of attitude that promotes bad code, and frankly, there is no spaghetti code cycle. Just as Joshua said, if you follow good practice, comment your code for later, and THINK about your design then most bug fixes will be trivial.
This is because good design plans for change and even bugs, because they will show up.
There are rare times for quick fixes, but they should always be replaced by a solution that fits the design. Good refactoring is never change for change’s sake.
@Joshua:
I agree, there are times that projects need to die. If we spent all our time trying to upgrade old crappy stuff we’d never move forward.
Dmitri makes a good point: once spaghetti code exists, there *is* a spaghetti code cycle. If code has been produced without tests and without code review, then simply “rewriting” it can often follow the script outlined by Dmitri. I wholeheartedly agree with avoiding spaghetti code in the first place, but, if you’ve already got it, the solution is less clear.
You described a case in which you figured out the code enough in order to determine what was causing the bug. Armed with this knowledge and the “obvious” design implied by the form itself, you’ll want to rewrite it and replace it. However, it’s entirely possible that you missed something (it’s spaghetti code, after all) and that that something will bite you later in the release and testing cycle. Granted, you should be able to fix it very quickly, but it’s a bug that wouldn’t have come up had you just fixed the original bug without introducing new code.
The decision to rewrite should definitely be based on how well-tested the existing code is.
@Marco:
I still disagree about the spaghetti code cycle. A responsible coder would never let something properly designed to degrade so much. Bugs would be fixed without the downward spiral to crappiness.
With any luck, there are requirements to go back to or a programmer/client to ask. Obviously, rewriting should not occur if no one really knows what the program is supposed to do in the first place.
Well what if you are in a Perl obfuscation contest? Spaghetti code can look beautiful when looking at with the right perspective. :P
Good article Eric, keep em coming.
@Marco: “… but it’s a bug that wouldn’t have come up had you just fixed the original bug without introducing new code” - or perhaps you ADD new bugs when trying to fix the bug that shouldn’t have been there if the code had been written more clearly as well as thoroughly tested. After all, you cannot be entirely sure about what the code is doing, even when and if you know more or less what it is supposed to do.
However badly written, I wouldn’t call this ’spaghetti code’ though. The term ’spaghetti code’ was applied orginally to (Basic and Pascal) code when the execution jumped from one point to another through ‘goto’ statements…
@Wouter:
Agreed that I might have used “ugly” instead of “spaghetti”, in either case the code is all over the place. Maybe it’s “paint splatter” code…
[...] Spaghetti Codes Nedir? Kodlama tekniğinizi tekrar gözden geçirmenize neden olacak bir yazı! [...]