Overload overload: excessive ambiguity in code is dangerous
I’m a newbie dev on a large company that works with data. Company implements it’s own flavor of Scrum. Halfway through my second sprint ever, I ran out of work. Team lead said to check out a module a mid level dev on my team had been working on and tell him if I found any refactoring opportunities.
The module took a JSON response, transformed it according to some rules, then output the resulting JSON. The module implemented some sort of strategy pattern, with transformation rules changing according to the format of the input file. Relatively simple stuff.
Except it wasn’t. The strategy pattern was a lie. The interface contract was there, sure, but the implementations were a mess.
The client classes of the strategies knew all about the different strategies, therefore breaking encapsulation and completely negating all benefits of the strategy pattern while keeping the bloat. But the implementation was the real headscratcher.
Let’s abstract what the strategy interface was trying to do and call it “transform”. The method, in this case, was called “doTransform” in the internface. This name is bad, but not bad enough to overcome my awkwardness and actually talk to someone about it.
The implementations, however, hid a nasty little secret.
Each one had 5 to 7 overloads of the same method. doTransform with any number of parameters. Some void, some returned. There were 5 implementations of the thing.
doTransform was defined around 30 times in the same project to do the same job.
I was confused at first. Surely there must be some recursive wizardry going on here. But IntelliJ’s IDEA said otherwise. Only one recursive use was found per strategy.
Now, I was humble. I tried to give him the benefit of the doubt. There must be a reason, a pattern for all of this madness. I struggled to understand the code, debugging was arduous. I tried and tried to avoid thinking what I was thinking. That I was going to tell my boss than a senior member of the team just shat out some spaghetti code and whoever reviewed it didn’t care. I was shaking. I like my new job. I have a history of being a smartass, and confronting two senior members on my first effective month on the job was something I didn’t yearn for.
Lady luck had my back though. Team lead had found a task for me. I closed the editor, deleted the local copy of the code and tried to clear my head. WTF was that?
I couldn’t understand a thing. I mean, I knew what the component’s purpose was, I knew all the tests passed and I knew the guy who wrote it had ten times more experience than I did. But this wasn’t the kind of frustration found in an unfamiliar pattern, language or business. This code was ambiguously written and lazily designed.
As Steve McConnell wrote: a software engineer’s primary technical directive is to manage complexity. This code required the reader to have in mind way too many things. It increased complexity by calling interdependent functions and putting them all at the same level of abstraction, and didn’t bother to call them differently. This was spaghetti code of a flavor I haven’t encountered yet.
Good code reads well, like a book. It’s clear and decisive. If you had to read a book where all the characters where called the same and they did pretty much the same thing, it’s usually a pretty confusing and unenjoyable book (unless it’s written by García Marquez). Code that reads well is easy to understand. Easily understood code is easy to modify, therefore easy to improve, therefore valuable.
Don’t call one overload of a method from another. Overloaded methods should stand at the same level of abstraction. If two method definitions share common functionality, you’re safer by extracting that functionality to a uniquely named method, or if the design allows it, a different class.
Originally published at pdelasotta.wordpress.com on April 29, 2017.