The term "spaghetti code" comes from a time when programming languages where far less structured. Older languages allowed you to jump anywhere in the program. Modern languages don't let you jump outside of the function scope and have control constructs like "while".
I'd argue the modern equivalent is the overuse of design pattern abstraction where every function has one or two lines in it. The symptom is the same: following the control flow of your program requires a maddening amount of jumping around in your editor, making it difficult to reason about.
There almost seems to be a cognitive dissonance with some people, where the terms "GOTOs considered harmful" and "spaghetti code" are thrown around but they see nothing wrong with hundreds of tiny functions that are only called from one place. It annoys me that the original point of those terms, that code is for humans, is lost.
With regards to the article's example, refactoring your code for unit tests is making life easier for the computer, not humans. And maybe that's why Bill is annoyed, being human and all.
And don't quote essays you didn't read.
edit: I removed a line but here was the gist of it: Unit tests are great but many languages don't support them well, especially compiled ones. We need to strive for a language that let's you isolate a module for testing without the type system or linker throwing a fit.
> they see nothing wrong with hundreds of tiny functions that are only called from one place
On almost every codebase I've been brought into, the simple guideline of "do less within your methods/functions" would lead to a higher engineering velocity for the organization. Yes, putting no thought behind the larger structure (e.g. how your public interfaces into/out of components are defined) will lead to too much jumping around, but if the functions are small and not leaking state then this is relatively easily remedied by someone with a systems bent. Small methods also convey to the reader the intent of the code by tying meaning to it through naming (if naming is thought out :)).
Large methods, on the other hand, build rigidity into the system and decreases its changeability. In this case, changes to the system necessitate reading and understanding the full implementation before modification. I will concede that I'm using "large methods" as a not-necessarily-true placeholder for "methods that do multiple things"
> refactoring your code for unit tests is making life easier for the computer, not humans
A machine does not care if you have unit tests. Unit tests are expressly for the human (team!) task of creating software that is maintainable and changeable.
I had an internship some time ago with a Large Company, and the codebase I was working on was very enterprisey-Java. It was really painful in this regard: an huge number of different classes, many of which were alternatives to each other, and pretty much everything was applied via homebrewed dependency injection. I didn't know this sort of thing had a name, but I think "ravioli code" matches perfectly.
One of my first tasks was to make a minor change and after an hour or so I still had no idea where the code was that needed changing. I called my manager over and he spent about 40 minutes trying to find it. He did, eventually. The change I made then took about 10 minutes to write and test.
That's 110 minutes of intern time + 40 minutes of manager time for a 10 minute fix.
I think the 'ravioli code' moniker is really a slight on good code. It just takes some time to become acclimated to the style of having many smaller pieces. That said, over-engineering is real. Unnecessary layering should go away.
I think the 'spaghetti code' moniker is really a slight on good code. It just takes some time to become acclimated to the style of having one long codeflow path. That said, over-engineering is real. Unnecessary assembly should go away.
"Magic code" is the modern "spaghetti code", where you still can't figure out what's going on unless you're deeply knowledgable about 10 layered frameworks that have invisible global state and the entire app magically appears from an annotation.
>> they see nothing wrong with hundreds of tiny functions that are only called from one place. It annoys me that the original point of those terms, that code is for humans, is lost.
I write code that way (hundreds of tiny methods/functions) because I find it easier to read and modify, and therefore maintain and I've largely done this from a junior dev's point of view where maintainance was most of the job I was doing day in, day out. If you find it hard to follow that sort of program flow maybe it's a sign that there's no rule and different people find different types of coding easier to maintain.
In which case we're all a bit screwed as we inevitably eventually will have to handle the code of someone who's had the opposite view than ours.
On the plus side, that means we don't have to sweat it so badly with the "right" way to write code and just accept that code eventually becomes one big mess with a gigantic, horrible 50-case switch statement in the middle.
> On the plus side, that means we don't have to sweat it so badly with the "right" way to write code and just accept that code eventually becomes one big mess with a gigantic, horrible 50-case switch statement in the middle.
No! No no. No it doesn't. Not unless someone makes it that way. Code doesn't have to become awful. It becomes awful when successive generations of maintainers choose quick easy partial-fixes over a bit of refactoring to fix the problem properly.
Code wants to be short, simple, clear, and easy to understand. People make it horrible by not caring (or by not seeing patterns).
> With regards to the article's example, refactoring your code for unit tests is making life easier for the computer, not humans.
I often find that writing programs with testability in mind leads to more self-contained code, with less dependencies and cleaner interfaces. All this tends to make the code easier to reason about. I'd be curious to see an example of code that's good for unit testing but bad for humans.
> I'd argue the modern equivalent is the overuse of design pattern abstraction where every function has one or two lines in it.
If you're writing functional code, very short functions are in fact considered best practice. This is because (obviously!) the basic unit of composition in functional languages is the function, and not the class. So you want to be able to compose those functions as much as possible.
In fact, I've also read and been told by old school programmers (I'm 40, so I'm talking 50-60 year-old C hackers) that even non-functional languages like C and Go should limit the length of functions as much as possible.
Experience backs this up, since some of the worst code I've ever worked had functions 2-3 screen lengths long, often with unrelated logic there within. Invariably, this was either C or VisualBasic. Say what you want about Java, and it is verbose, but at least it limits programmers to 1 public class per file, and Java programmers do tend to break their class logic into smaller methods.
So for imperative languages, the advice is frequently: don't limit functions to a line or two (although that's entirely appropriate and even recommended for Haskell and functional JavaScript), but preferably not more than a dozen or two lines.
And if you name your functions well, you usually shouldn't be needing to jump all over the screen to lookup a function. The name should tell you exactly what it does. If you're using statically typed languages with a decent IDE, the type signature should provide any additional needed info.
Google's C++ style guide: "Prefer small and focused functions."
Haskell.org's style guide: "Most haskell functions should be at most a few lines, only case expression over large data types (that should be avoided, too) may need corresponding space."
felixge's nodejs style guide (one of the more popular ones for NodeJS): "Keep your functions short. A good function fits on a slide that the people in the last row of a big room can comfortably read. So don't count on them having perfect vision and limit yourself to ~15 lines of code per function." (my experience with functional JS is that the best code bases strive to keep functions around 5-6 lines in most cases; check out React, which for any criticisms is very well-written).
Well that's one harmful pattern, but by no means the only one, you also have "The Blob". I once worked on a project where the code in general was pretty good but the programmers had seemingly no knowledge of design patterns at all. Code was contained in these giant classes, they actually used partial files with underscores in the naming to segment it, ugh.
Took quite some effort to disentangle that. As with most things no extreme is good, neither Blobs nor Poltergeists :)
Holy hell, partial classes. I'm not convinced that, outside of WinForms designer-generated control definitions, there is ever a legitimate reason to use them.
I just tore apart one of those kinds of monstrosities last week, where the person who originally wrote it, went so far as to do C-style name prefixing on the methods, i.e. Foo_DoThing(), Bar_DoOtherThing(), Foo_DoOtherThing(), but left it all in one honking monster class instead of splitting it out.
Yeah... they're really handy for separating generated code from user code... but having to segment classes with them is a rather foul code smell :)
Haha, wow, that's another way to go I guess. It never ceases to amaze me how inexperienced programmers can brute force problems with the limited tools they possess :)
I agree. Partial classes are for supplementing machine-generated code: winforms designer files, generated entity framework classes etc.
That is, wherever the IDE would overwrite any edits you made. I'm currently refactoring a 3 partial class monster usercontrol: 238 lines, 2370 lines, and 4526 lines. Fun.
> With regards to the article's example, refactoring your code for unit tests is making life easier for the computer, not humans. And maybe that's why Bill is annoyed, being human and all.
Maybe the tradeoff is actually between:
A. short term productivity / speed of development -- use longer functions without factoring out non-repeating code excessively
B. long term maintainability -- short and easily testable functions are definitely better
So use approach A. to code an MVP and approach B. for a decade-lasting enterprise application on whose development tens of people will rotate?
Oh, and... please, if you language allows you, use closures / functions inside function when you factor out a small bit of code that does not repeat, NOT private methods or non-exported functions! This increases code locality it's obvious that one line helper is used only once. And of course, give the mini-functions meaningful names, otherwise there is no point in doing this in the first place - you only factor out to increase readability. And prevent the mini-helpers from using variables from outer scope too much (if your mini-helper uses 5 outer-scope variables, then DON't factor it into a mini-helper).
(Advice above is extracted from a talk inspired by smth J. Carmack wrote, but can't remember exactly what...)
For me spaghetti code is called so because when you start pulling on one string of the code you end up spilling everything. A 5-layered for/if statement does not have this property but using abstractions everywhere does.
> For me spaghetti code is called so because when you start pulling on one string of the code you end up spilling everything. A 5-layered for/if statement does not have this property but using abstractions everywhere does.
So in theory a well unit-tested spaghetti-code would make you more relaxed because it would assert that spaghetti A interacts with spaghetti B in the C way.
> So in theory a well unit-tested spaghetti-code would make you more relaxed because it would assert that spaghetti A interacts with spaghetti B in the C way.
I don't follow. In my experience well-unit-tested and spaghetti code are mutually exclusive as the spaghetti code does not have separation of concerns.
A note on my opinion on abstractions to clarify: I do not think that abstraction is inherently bad, just that starting with the idea that every component has to be generic, each class must have an abstract interface and that code should never ever see into another classes implementation tends to force developers to make so much mental yoga that they end up tying the components together without realising it.
Most people don't have even the most basic understanding of how they reason about and write code. So they just follow "best" practices and other kinds of dogma. Nothing wrong with that. But it does make it harder to have a meaningful discussion about "bad" code and how to improve things. And I do think there is a room for a tenfold improvement in reliability/productivity compared to OOP.
> His code is just… ugly… to the point that you nearly detect a physical odor from it.
Unit tests don't magically bring quality code with them. But it does tend to make it easier to focus on encapsulation.
> "How can we overcome years of inertia, a nasty legacy codebase, ...
Yeah -- this is a real challenge. BTW Bill is not the problem so much as the codebase. If Bill refuses to consider unit tests, that's a problem that I think is solveable.
> Simply walking over to Bill ... "You have a tendency to write enormous methods...
Nothing would make any designer more defensive than saying "you do [bad thing]". Bill would probably confess that he, like everyone else, has contributed a few lines here and there to MegaObject::mega_func(). Your challenge is that Bill is apparently less motivated than you are to address the problem. Focus on the code itself.
"Bill, you know mega_func()?"
"OMG yeah that is a nightmare to debug."
"Well, I've been working on unit tests. It took a while but I found a way to create a unit test to cover MegaObject. Now I can test MegaObject without deploying to the target first."
"Hmm..."
"Newbie Fred took this test for his first feature, a one-liner fix in mega_func()."
"Oh yeah?"
"Yeah, and he was able to find and fix issues with his implementation without going to the lab. And then he refactored mega_func() by adding some encapsulation to those middle 70 lines. We only need that bit in FROB mode anyways."
In my experience, this usually ends with Bill continuing to do what he's always done and you & Fred trying to continue refactoring and adding tests but getting frustrated because more untested mega code keeps getting added so your target keeps expanding. If you're lucky, you'll reach critical mass and be able to start enforcing coverage or lint checks at the build step (at which point you can tell Bill "oh yeah you just need to add test coverage and it will pass" when he comes to you trying to figure out why is build failed), but more often than not you won't be able to keep up and you'll just get burnt out & cranky.
While I absolutely agree with "talk about the code itself" rather than beating around the bush in some manufactured discussion, why not just address the issue head on when is proper to do so, in CR, and build a culture where code critique is both acceptable and clearly ONLY a critique of the code at hand? I like to think that I could highlight mega_func in a cr to our team and go "extremely difficult to ingest this long a function as a human or modularly test it. Refactor would be welcome" and they would say "makes sense" and swap it in the same way as if someone pointed at one of my DB schemas and went "that's going to cause you problems because X and Y and I'm not sure what you were smoking when you decided on your constraints."
We're professionals here but we're not perfect, and the faster we come to a shared understanding that this is fine, and it's part of professionalism to have each other's backs, the faster I've found my teams able to broach these issues without insult.
“How can I get Bill to stop writing spaghetti code” is only superficially a technical problem. In reality, it presents a human problem.
Actually, it's a management problem. Management must establish and enforce proper process and culture.
Necessary but not sufficient:
1. Code standards, determined and agreed upon together
2. Peer review against those standards
3. Requirements Definition policies and procedures
4. Function Specification policies and procedures
5. Unit Testing policies and procedures
6. Technical debt policies and procedures (when to grandfather)
None of this has to be that complicated. When the rules are understandable, logical, and everyone agrees to play by them, this takes the monkey off everyone's back. You don't have to worry about approaching Bill about his bad code. You have a "business framework" you can rely upon to take care of that for you. Then no one's the bad guy. This is just good basic I.T. management.
Spaghetti code by others is the norm. I have maintained the shitty code of thousands of others. If they're gone, I cuss them all day long. If they're still here, I'm nice. I help management build the processes needed so that I can stay nice. I do my job. I expect management to do theirs.
Consistent enforcement is key. You can have all the policies and procedures in the world but if not everyone is motivated to follow them all the time, they're pointless. Also required: the ability to stick to the policies and procedures in the face of "Just ignore the rules this one time--we urgently have to ship this month!!" Cave once, and suddenly every month becomes "urgent".
You're absolutely right, but I'll just note that many, many companies do not have management with the technical acumen to facilitate those decisions. That kind of leadership is a luxury.
I agree with this pretty much completely, although I wonder a little how it works at places like my current one, with 4 total programmers.
I just wanted to add that probably every 'human problem' or 'people problem' at the individual programmer level is really a management problem. This is the same as how every manufacturing defect is really a process problem.
Works well with small teams. Small teams mostly implies smaller projects & scope, so that scales the documentation nicely. Also, you have fewer communication barriers to defeat.
Good points. Code ends up being a reflection of internal structure. Management is tasked with defining that structure. If it gets to the point where a developer has to fight for good structure then management has failed and there isn't much to do if they can't see any issue.
That's a big-company solution? When its just a room full of guys, maybe a better approach is to hire more carefully. A half-dozen developers need a skeletal process, especially in a startup.
I'm working on Gorgon Medusa [1] code, which used to be quite beautiful, but now it has snakes for hair and turns you to stone when you look directly at it, so I have to look at its reflection in my shield while I edit it.
There are 2 extremes in the 'code quality' spectrum: A) the programmer who ships 'stuff that works' but incurs large amounts of technical debt (AKA Bill) B) the metaprogrammer who will only ship anything 'perfect' and will push back on any changes he deems to break his crafted code. Of course, there is a balance to be attained and extreme are rarely the solutions.
I think getting the price down on better solutions takes some measure of innovation and thinking.
It may be test harnesses. It may be extensive prototyping.
It often involves investing in equipment.
From the coding perspective, it involves having structure be transparent to function. Everything is explicit; have requirements be represented such that they're almost derivable from the code base without testing. This doesn't mean you don't test; it means you do both.
The your concern is the consistency of the written requirements, the code base and the results of tests.
As an example, I worked on a thing that would cost tens of thousands for one field test. For about a half or a fourth of the cost of that, I was able to mock a field test in the engineer's desktop - not in a lab, but right there. This required a small amount of equipment, about two-four cubic feet worth. This was based on extensive sets of logged data, translated into models.
Of course it's by definition inadequate; there are lies, damn lies and simulations. But being able to anneal defects back through the models means you lose a lot less information.
I eventually put a thing together in a VM that mocked the entire system. I actually did feature-adds against that, that worked just fine.
I was (briefly) able to show about a factor of ten improvement in defect rate. But nobody on the team wanted to own this. the implicit message was "we do the thing; not that thing you just did."
It's now a failing organization, so I'm sure all that got dumpstered. That's fine; I lit a candle instead of cursing the darkness.
You make interfaces, write everything beyond off as a total failure, and worry about containment and the impending rewrite of his part later.
Its like managing a burning forest, you cant extinguish it after a certain dimension is reached, you can only cut the fire from the food and keep it from spreading.
The problem is, when your colleague is supposed to work on the same module and you are constantly left debugging stupid stuff out of "working code" he wrote.
Management scolds you for being "slow" while the moron gets to play the fast car.
Thoroughly agree that this is a human problem, but I think there are some amazing automated fixes for it, by incorporating code checkers into the continuous integration pipeline. Take a look at how the CII best practices badgeapp [0] uses pronto [1] to run rubocop [2] automatically for free via CircleCI [3] and surface those results via the Github Status API and email.
The whole point is to avoid having to manually complain in a code review. Instead, code reviews become a much higher level exercise discussing proper APIs and code structuring, not sphaghetti issues.
But you need to get agreement to implement/work with these automated fixes, and now you're back to it being a human problem. If you're 3rd engineer from the left proposing this, your human problem is convincing your team lead or management to invest in these tools. If you're team lead, your human problem is convincing the rest of the team the tools' value and getting them to buy-in to using them.
Right, but as team lead, there's a huge advantage to implementing the tools once, and letting individual developers argue with the robot over coding style (i.e., when they override certain rules), vs. trying to do all code review manually.
This is great management, interpersonal, and pragmatic advice for anyone in many conflict situations, not just in code disagreements. Diplomacy, ego-conflict reduction, data-orientation and so many more wise human practices are involved in this.
> But you’re about to accuse Bill — Bill! — of writing spaghetti code. Bill, who started with your company when they had carrier pigeons instead of emails, and who singlehandedly built the original version of the software with nothing more than COBOL, baling wire and duct tape. Just who do you think you are, anyway?
Ugh, I had this problem once. The guy who wrote our company's web store did it in pure javascript (no libraries) and C (not even c++) on the back end purely for credit card processing. He didn't believe in googling something that he could figure out how to do himself, and consequently he never encountered a single best practice.
In the end, he was so resistant to change that our boss had to fire him. He went from being the alpha-dog with a decade of seniority who was treated with utmost respect, to fired, within about two weeks of me volunteering to rewrite the e-store and the CEO accepting my offer.
On the one hand, I wish I'd known how to handle that more diplomatically. On the other hand, thank god I don't have to work with such an irritable primadonna. He was the only employee allowed to work from home, because he was deathly allergic to computers (no joke), and had a special setup at his house where the computer was isolated in one room, monitor pressed up against a window, and only a keyboard and mouse in the room with him.
> He was the only employee allowed to work from home, because he was deathly allergic to computers (no joke), and had a special setup at his house where the computer was isolated in one room, monitor pressed up against a window, and only a keyboard and mouse in the room with him.
Story aside, I'm intrigued by this. Was he allergic to PCB dust or something? Or is this some sort of joke or reference wooshing over my head...?
My take was it was either that, or a bullshit excuse to allow him to work from home. One the one hand, several other employees assured me his crazy home setup was legit. On the other hand, he would sometimes have to work a couple of days in our office during big events, and he seemed fine.
Yeah, and perhaps I'm a lobster pretending to type on a keyboard.
The guy was a fucking moron.
The user database was a flat file array stored, unencrypted, passwords and all, in users.js. I told him that was a huuuge security hole, and he said "What, you think hackers are just going to browser through our source code, looking for anything worth stealing?"
The entire store database- users, products, orders, etc, were stored in flat JS arrays, and all of them were downloaded to the user's browser so the store could run client-side, "because then our site can scale infinitely since we're only serving static files!". Only the credit card transactions were transmitted to our server. Thank god he knew better than to store credit card info, because our entire array of servers (that he was in charge of) were hijacked by russians and turned into warez FTP sites.
That doesn't even address the hard part. That link shows how to add tests for a nice pure function. The reason testing existing code is usually hard is that it's inserting records in a database or launching nukes or something, and we're not able to actually do those things in a unit test.
Of course not, at least not for the launching nukes. Inserting records in a database? I unit test that all the time. You just need a database on your development machine, rather than accessing the production database.
You need a database, a way to access it, and some kind of guaranteed initial state, and a way to tell your code to use that stuff instead of the production database. There's no guarantee that some arbitrary existing code base will provide a sane way of providing those things.
An arbitrary existing code base? Perhaps not. The portions I've been working on for any length of time? They will. They'll have unit tests, too.
Some kind of guaranteed initial state? Yeah, my tests will set that up every time they run, just like they'll set up everything else they need.
Now, if I'm working on Oracle-specific code, say, and I don't have an Oracle license for my desktop, I'm unable to do what I have so bravely stated that I will do. Also, if I'm thrown into a mess with no time to do anything but panic fixes, I don't have time. But never in my career has that been the case for long (maybe I'm fortunate...)
Great advice, and there are many advantages to doing so. However, my experience with that tends to demonstrate that programmers will nit pick at lesser details (function names, coding convention) and not really take the time to understand what the code on review wants to achieve. Of course, the latter is time consuming ... any thoughts on this?
The nit picking I refer to is not about arguing, once a non-standard piece of code is pointed out, the reviewee usually agrees to correct the issue. I refer to the fact that the reviewers will look at the leaves in the tree and not notice the burning inferno and smoke up ahead in the forest...
I'd argue the modern equivalent is the overuse of design pattern abstraction where every function has one or two lines in it. The symptom is the same: following the control flow of your program requires a maddening amount of jumping around in your editor, making it difficult to reason about.
There almost seems to be a cognitive dissonance with some people, where the terms "GOTOs considered harmful" and "spaghetti code" are thrown around but they see nothing wrong with hundreds of tiny functions that are only called from one place. It annoys me that the original point of those terms, that code is for humans, is lost.
With regards to the article's example, refactoring your code for unit tests is making life easier for the computer, not humans. And maybe that's why Bill is annoyed, being human and all.
And don't quote essays you didn't read.
edit: I removed a line but here was the gist of it: Unit tests are great but many languages don't support them well, especially compiled ones. We need to strive for a language that let's you isolate a module for testing without the type system or linker throwing a fit.