DaedTech

Stories about Software

By

What to do when Your Colleague Creates Spaghetti Code

Editorial Note: I originally wrote this post for the NDepend blog.  You can check out the original here, at their site.  While you’re there, take a look around and give NDepend a try.

I write for a number of different outfits and earn my living consulting around software and IT.  Because of the intersection of these three concerns — writing, offering advice professionally, and software — I field a lot of requests for advice on how to do the right technical thing without everyone around you shooting holes in it.  Consider an example.

“How can we get started with unit testing?”

Considered as a technical question alone, this invites a fairly obtuse response.  “First, write a unit test.  Second, run the unit test.”  Obviously, that’s not really what anyone is asking when they ask this question.

Instead, they want to know something orders of magnitude more complex.  “How can we overcome years of inertia, a nasty legacy codebase, Bill, who has been around for 40 years and hates everything new, and the fact that management doesn’t want to pay us to write ‘extra’ code… and then start unit testing?”  Oh, yeah, that.  Well, that’s complicated.

I frequently hear such apparently-innocuous-but-actually-complex questions about code quality.  “This idiot on my team is writing mountains of the most unmaintainable garbage imaginable — what should I do?”

spaghetti

Usually, this sort of question comes to me from people at client sites.  And, accordingly, I have to balance the answer that makes the most sense for the individual with the one that keeps the client’s best interests in mind.  The client pays my bill, and I have a charter to lower the cost of ownership and development of their code.

But when I’m in writer mode, and only the asker’s interests matter, I’l give a subtly different answer.  Today, I’d like to offer advice on what you should do in this situation.

A Note of Disclaimer

To be clear, the question “what should I do” does not imply that you and your organization move in lockstep when it comes to your interests.  Your organization may want the cleanest, most maintainable code imaginable, and, theoretically, it would demand that you fight the good fight.

But your organization might also throw “Angry Bill, the unit test hater” at you and tell you that he’s the senior developer, like it or not.  Suddenly, dying on the “clean code” hill becomes a less clearcut choice.

I thus offer the following disclaimer.  My advice on what you should do may not always drive the best outcome for your company.

Make a Data-Based Case

Having read my disclaimer, there you sit, convinced that Bill is slowly poisoning each codebase that he touches.  His vertical and horizontal spacing infuriates you, his public method semantics offend your sensibilities.  His code is just… ugly… to the point that you nearly detect a physical odor from it.  You might even think he’s doing it to spite you.

As much as you hate it, are you sure it’s bad?  Are you editorializing at all when you describe it as spaghetti?  I mean, how do you know?  Have you charted the entry and exit points and graphed out the dependencies?  Or do you just not like it because he does things much differently than you?

It’s not that I doubt you.  It’s that you can bet most people you make your case to will doubt you, especially Bill.  If you don’t arm yourself with some data about the code, you’ll look spiteful when you state your case.  And if you don’t back the data with supporting references that aid your case in declaring it a problem, you’ll do little better.

Do some research before going any further and make sure you can justify your accusations.

Ask Yourself, “Just Who Do I Think I Am?”

Ask this question because you can be sure someone else will.  Sure, you’ve built a bulletproof, case, leveraging static analysis, expert opinions, and industry trends.  That’s all fine and good.

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?

Seriously, this matters.  You need to consider your role.  If your team looks to you as technical leadership and a mentor, than going to a new team member and saying, “your methods are pretty complex — try to keep this thing called cyclomatic complexity under 3” presents no problem.  Everyone, newbie included, expects you to do this in your role.  But if you’re the newbie and the junior developer saying the same thing to the mentor, it’s going to get ugly.

I can hear your indignant protest that “roles shouldn’t matter when there’s an empirical demonstration of rightness!”  Honestly, I agree with you.  And I imagine HR will want to hear all about it at your exit interview.

Arguing from a position of relative inexperience exacts a political toll from you — even when you’re right.  Have these battles at your own peril, and check out the term pyrrhic victory to understand what happens even when you win one.

Offer Alternatives and Outcomes

Let’s assume that you’ve built a decent factual case and accurately sized up the organizational capital at your disposal.  You have enough of a case to proceed and enough credibility that people will listen to your case.

Don’t actually proceed until you can offer some kind of solution.  Simply walking over to Bill and saying, “I’ve been doing a lot of research and here’s a lengthy treatise on why your code is terrible,” will only demoralize him.  Instead, you need to prepare by having alternatives ready to offer.  “You have a tendency to write enormous methods, but you can actually take a lot of those local variables and extract classes with them as fields.”  Now, you’re offering a plan of action.

But go beyond that as well, and complete your case.  Once you’ve shown Bill the better way, make sure you can then prove (or something close) better results will follow.  Get creative.  Maybe pull someone over and ask them how long it takes them to understand the old method and the new, broken-up classes.  Whatever you have to do, help the “aha” moment along by offering benefit.

Humility, Diplomacy, and Leading By Example

You’ve built a case, stated it, and demonstrated it alongside value offered.  Don’t let that go to your head.

You don’t want to take the attitude that “my way is right and yours is wrong.”  Rather, you want to let the data do the talking and decouple you and Bill, as humans, from the code that you have written.  You’ve read a lot and learned from smart folks that better outcomes occur this way.  You’re not an expert, but you’ve had good results yourself, if he wants to take a look.  Here, look, you’ll show him if he’s interested.

Do your best to adopt something of a patient mentor stance, but without implying that one of you is better or more experienced… even if you clearly are better and more experienced.  The subject shouldn’t be you and Bill, but the relative merits of various techniques.  You’re just a couple of peers discussing them.

A Closing Philosophical Note

I touched on this in the beginning of the post, but I’d like to restate it here.  “How can I get Bill to stop writing Spaghetti code” is only superficially a technical problem.  In reality, it presents a human problem.

You can have the right of it without making any difference.  Or, you can browbeat Bill and entrench him further.  You can send him angry emails, undo his commits, “reject” his stuff at code review, and work against him.  That will demoralize him and torpedo everyone’s progress.

Simply put, you can’t alter Bill’s behavior without a whole lot of dealing with Bill.  So, whatever route you take, understand that your question is one of persuasion and not technical correctness.

 

21 Comments
Oldest
Newest Most Voted
Inline Feedbacks
View all comments
Michael Parker
Michael Parker
8 years ago

.. and of course there are 100 different types of Bill, organisation and relationship, and different actions are required in all of them! 🙂 For example.. – Bob who favours complex concise code because he has no problems understanding it and thinks neither should you (or thinks ‘it has no bugs so why do you want to read it’) – Bert who doesn’t really want to be there and doesn’t care about doing a good job and has family issues on his mind (and is getting 2 hours sleep a night) – Brad who always wants to use a different… Read more »

fignewtron
fignewtron
8 years ago

do…if(dataState1 == true) break; doSomeStuffHere; if(dataState2 == false) break;…while(false)

How to convince “Bill” after attempting to lead with compassion, showing a better way, more than once, that this code pattern is a thinly veiled goto statement and a pattern that should not keep going into a large code base? What if R# is promoting this spaghetti code?

Erik Dietrich
8 years ago
Reply to  fignewtron

This sounds like a fairly… personal situation.

There’s a lot to unpack here, and I could probably write posts about a few different things. That said, the first thing I have to ask is…

Really? R# recommends break statements? (I’m a CodeRush guy, myself)

enzi
enzi
8 years ago
Reply to  Erik Dietrich

I assume R# will offer an option to “reduce nesting” here i.e. `if (dataState1 == false) { doSomeStuffHere }` is changed to use a break so that `doSomeStuffHere` is not nested further.

I wouldn’t use the term “promote” or “recommend” though, it’s a quick action that you can use to speed up an otherwise tedious task, not a code quality tip. The decision of whether you should use it is the developer’s.

fignewtron
fignewtron
8 years ago
Reply to  enzi

Yes, it is my understanding that R# offered that do/while refactor action and the developer chose to use it. A lot. Which means a way of thinking about writing code that seems to need a lot of if statements and nesting. The idea is that this is not a good style for readability and therefore maintainability because what happens in practice is that it shows up in methods that are more than 300 lines of code. Which is another, different bad habit to try to help developers move away from. So, in this example it is R# suggesting to “Bill”… Read more »

adel
adel
8 years ago

Fire Bill is the best option imo.

Successful corps do not employ Bill. ( facebook, google,uber …)

bayram
bayram
8 years ago
Reply to  adel

successful corps don’t rely on the my way is better guy. As for this post i thought it would promote the unit test, xx test, test driven dev. hype which are like every other hype, they are SOMETIMES good but not in all cases

Erik Dietrich
8 years ago
Reply to  bayram

Personally, I’ll stump for TDD all day every day. But in the context of, “this works for me, so it could work for you.” I don’t presume to have the answer for everyone.

Erik Dietrich
8 years ago
Reply to  adel

I would beg to disagree. Companies of the size you mention (50K employees or whatever) can’t help but whiff on hiring and stagnate when it comes to talent. If you’re that big, you can’t ipso facto, hire only the best and brightest, no matter what the marketing brochure says.

I’d be shocked if there weren’t big piles of Spaghetti at Google and Facebook (Uber might be too new)

dave falkner
dave falkner
8 years ago

Any tips on how to sniff out organizations early that are too Bill-like to ever be able to make enough headway, say, during the interview phase?

RvLeshrac
RvLeshrac
8 years ago
Reply to  dave falkner

Unless they’re willing to show their code, not really.

You can probably see some tells (e.g. that one guy who always asks an incredibly stupid interview question about, say, “what’s your favourite function”), but that’s no guarantee; maybe you’re just interviewing with less-capable people because everyone else is busy doing actual work.

You could opt for some reverse-interviewing if there’s a whiteboard involved, by seeing if you can get away with terrible code, but that’s sadly just as likely to get you NOT hired at a decent company as sniff out a bad one.

Erik Dietrich
8 years ago
Reply to  dave falkner

I’m going to note this as a topic to address with a blog post in more detail. Short story is that I try to ask questions that would elicit telltale responses. “How do you manage dependencies in your codebase to keep things manageable?” “When do you feel it’s appropriate to use global variables?” Questions along those lines. They can yield false negatives, but you’ll tend to clue in to five alarm red flags this way.

Robert Blair
Robert Blair
8 years ago
Reply to  Erik Dietrich

Interview war story, from around ten years ago: The interviewer was in his twenties, I was much older. The job was mostly coding to SQL Server. He gave me three problems to code SQL queries for. Two were no-brainers. The third one was a bit tricky. I decided to resolve it in one query by using a couple of subselects. I figured the whiteboard I was coding on didn’t really care about performance. The interviewer got agitated. “That is not the correct answer” says he. I saw he had an answer sheet. Apparently I was supposed to query into a… Read more »

dave falkner
dave falkner
8 years ago
Reply to  Erik Dietrich

It’s totally fine with me if the false negative rate of a strategy is extremely high, as long as the false positive rate is kept very low. 🙂 I’ve tried once actually bringing pre-printed code samples to an interview, one with an object hierarchy to represent various animal types, and a separate code sample with an AnimalType enum and switch statement, then asked the interviewing developers which variety their team generally preferred, what the implications were as the system grew and more types and operations were added, etc. etc. I’ve also learned to ask what the team’s reaction would be… Read more »

Robert Blair
Robert Blair
8 years ago
Reply to  dave falkner

Well Dave, my organisation is a “Bill-like” one. And I am afraid that I am the Bill. Yes, I started programming in Cobol on mainframes. Only people from that era really understand the importance of dual rubber bands. The main product of my organisation is a very specialist Call Center system, about 250K LOC. There is also an Express version, rather less code. Keep in mind the industry I am working for is secretive and volatile. Also, “Call Center” is a bit misleading. It is really an ERP with call center focus. I designed the thing originally, and it was… Read more »

Greg Hall
8 years ago

I generally agree, but sometimes you may not have much of a solution to offer beyond a rewrite, or you may have not been at the company for very long. I had a case like that, I had been in the biz for 2 years and had just joined the company 3 months previously. The code was bloody terrible, to the point that the testers did a full 40 hour workup for every little change, as they did not trust developers any farther than they could throw them. I wrestled a bit with it, and decided to come up with… Read more »

Erik Dietrich
8 years ago
Reply to  Greg Hall

That’s a brave (and ethically admirable) tack to take. Do you have a sense of how the responsible developers reacted to your case to management? Or, did they know about it? Management having blank stares in response to this isn’t necessarily surprising to me (I have to make cases like this a lot these days), but I’d be surprised if you stating that opinion didn’t make for at least some contentiousness around the water cooler.

Greg Hall
8 years ago
Reply to  Erik Dietrich

Believe it or not, there really wasn’t any conversation about it, I never heard so much as a whisper it again from anyone. I did state my case in a rather matter of fact way, without any blame – for all I knew the system got that way before half the other devs even started. I can only guess the other devs were in silent agreement 🙂

Erik Dietrich
8 years ago
Reply to  Greg Hall

That makes sense. Usually, when I’m consulting and have to call out problems like that, I employ various ways to remove blame from the situation. One of the best is the (honest) declaration that, “look, I’m here to make a prognosis, but I didn’t have to make the calls that you made faced with the pressures and constraints that you faced. I don’t know that I’d have done a lot better — I’m just here to talk about current reality.” One thing I’ve found in my travels is that people rarely make purely lazy or stupid decisions, contrary to common… Read more »

H.M. Müller
H.M. Müller
7 years ago

And … back off and say you are wrong when you are wrong! – at times,theremight be good reasons for queer constructs. Having accepted that others are right will make them listen to you next time!

Erik Dietrich
7 years ago
Reply to  H.M. Müller

That’s a great point. Freely admitting when you’ve gotten things wrong definitely bolsters your credibility over the long term.