What to Do When Your Colleague Creates Spaghetti Code

> 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.

The modern equivalent is called 'ravioli code' http://c2.com/cgi/wiki?RavioliCode

"thousands of little classes everywhere and no one knows how to find the places where things really happen."

"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.

> 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).

>I'd argue the modern equivalent is the overuse of design pattern abstraction where every function has one or two lines in it

I've heard this described as Lasagna Layered code or Lasagna code. (Of course Copy Pasta is in our lexicon too, mmmmm)

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 :)

> 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...)

Interesting... Not knowing this background, I always assumed spaghetti code referred to the opposite in modern code: too many nested blocks.

I'm always super afraid of growing mounds of logic, and my refactoring usually abstracts a lot of that logic out.

If you use a good IDE like IDEA/Jetbrains, you can ease the pain of those "one-line functions" with "click to navigate" function/variable names

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.

What about the code for this python regular expression engine that is filled with goto statements? https://github.com/python/cpython/blob/master/Modules/sre_li...

Or the code for the OpenSSL handshake that interleaves preprocessing directives? https://github.com/openssl/openssl/blob/master/test/handshak...

Interesting point that I've never heard b4. What about the DRY concept?

ravioli code

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.

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.

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.

[1] http://www.greekmythology.com/Myths/Creatures/Medusa/medusa....

A middle course is to mock things that will support case B). It won't be perfect, but I think you have to try.

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.

> 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...?

Perhaps his system was better and you just didn't understand it and used a couple of buzzwords to get him fired.

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.

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?