The worst pieces of code I’ve ever seen
Today, I’m going to show you the worst pieces of code I’ve ever seen. Some devilries that should never be produced! Unless you want to be hated by your colleagues and your users. We’ll see that with a few good practices, it’s easy to avoid them.
Devilry
There’s a difference between code that needs to be improved and what I call a devilry.
No matter the language, a devilry is a dirty piece of code that endangers the stability and maintainability of the project. And I can tell you that I’ve seen a lot of devilry.
When it piles up, your project quickly becomes like the basement of hell. And if you’re the one who’s making a mess everywhere, the lead tech in the kitchen is going to start looking at you differently.
Ambiguity and inconsistency
A long time ago, in a galaxy far, far away, I arrived one morning and was jumped on as if it was the end of the world.
A huge bug in prod. All the system tickets in production return “null” for no reason. Chaos everywhere. Everyone was running around like headless chickens.
I sprint to my station and first reflex, I look at Kibana. No logs.No nothing. Fuck, this is not a good start.
So I decide to retrace the path of creation of the ticket.
To do so, I have to go into the depths of internals libraries that have been around since the Jurassic era. At the end of my archaeological work, I arrive in one of the responsible files.
And i saw this
// use id and expire to get ticket
async function get_ticket(i, expire) {
return CheckisNotExp(expire).then(async function() {
var t = await GetTicketModel(i)
if (t) {
return t
} else {
logger.error(JSON.stringify(t))
return null
}
}).catch(async function(e) {
logger.error(JSON.stringify(e))
return null
})
}
Is there really an “i” variable ? Where are we now? That’s an id, right? Integer or UUID? And what exactly is expire? Is it a date or a timestamp? Why is there camelCase, PascalCase and snake_case? Async notation with promises and async notation again? If something fails we return null? DEVILRY!
At this moment, half the company Skype me every 5 minutes to get an ETA for the fix.
So first, git blame, someone need to know what’s going on here.
I quickly realized that it’s not one person who is responsible, but three. Two people have left the company a long time ago, the third hasn’t arrived yet this morning. Classic scenario.
According to Git, these three people have touched this file at very distant times. Hence the inconsistency, the different styles, the different ECMAScript versions and the different ways of handling promises.
I take this opportunity to suggest you the best conference on promises I’ve ever seen.
I was in the room at this conference! I know, nobody care.
Anyway, in this piece of code, everything is ambiguous, everything is inconsistent. It’s a perfect example of what you should avoid at all costs. Needless to say, the code review didn’t go through that.
Well now, we have to rewrite this quickly.
I’m changing the names of the variables and functions. No ambiguity allowed. Async / Await everywhere and in the same way.
I’m also making sure I don’t hide any errors under the mat with a null return. These errors must break the function if something goes wrong. Exceptions should be handled by the layer above.
// hotfix
// @todo rewrite the ticket module entirely
async function getTicket(ticketUuid, ticketExpirationTimestamp) {
await validTicketExpiration(ticketUuid, ticketExpirationTimestamp)
const ticket = await getTicketByUuid(ticketUuid)
return ticket
}
The really good solution would be to rewrite part of the module. The ticket validation logic is bad here. But it’s not urgent.
The urgency is to spot and fix the error.
And right after updating the code, the real error starts to show its face. A configuration change made the day before had changed the ticket creation behavior. Returning to the previous configuration immediately fixed the problem.
The following week, the module in question was completely rewritten.
Spaghetti Bolognese
A long time ago, in a galaxy far, far away, I was working on a product with a clean, square code.
Like any good product, everything was optimized inside. Features made with as little code as possible. A strong attention to readability. A strict follow up of all good practices ensured by a code review managed by clean code nazis.
SOLID, DRY, KISS, YAGNI and every other acronym you can think of.
It was so clean, you could have eaten off the floor.
Even so, a very specific part of the product would break intermittently.
During a sprint, I eventually managed to negotiate time to investigate the case.
Very quickly, I realize that the problem is not the product. Errors have only one thing in common: a dependency. An internal dependency solved via an internal artifactory.
It is managed by another team and – very surprisingly – the code is not freely available. You have to ask a permission to see it. So I ask for access to understand what’s going on.
I then receive a slack message or I’m asked why I want access.
“-Hi! Why do you need access on this repository ?”
“-What do you mean why? Are you aware that I work here? Hold on, I’m on my way.”
After a surprise armlock on the person, I end up having access to the project.
I see a single file in it. 300 Ko in size. 300 Ko of text, it’s huge. It hasn’t been touched for several years. The name of the person who last touched it is completely unknown to me.
The worst devilry.
The biggest spaghetti code I’ve ever seen in my life. For the sake of length, and to preserve your sanity, I’m not putting everything in. Just a very short excerpt of what I saw inside.
Careful, it stings the eyes.
// Thousands of lines of spaghetti codes
if (global.Builder)
{
module.exports.buildPgs = function(pgs, options, limitNodes = 0)
{
var config = options.config || {};
var builded = [];
pgs.each(function(pg)
{
var supported = pg.prop('tagName') == "INPUT"
&& pr.attr['name'] == "file"
&& options.pgs.rel.active == ""
&& global.FileReader;
if (!supported || !pg.f || pg.f.length == 0)
return;
for (var i = 0; i < pg.f.length; i++)
{
builded.push({
file: pg.f[i],
instanceConfig: _.extend({}, config)
});
if (isFunction(options.before))
{
var returned = options.before(pg.f.path);
if (typeof returned === 'object' && global.status.in_progress)
{
if (returned.action == "skip")
{
var needsSkip = (typeof global.status.in_progress === 'boolean' && global.status.in_progress)
|| _.hasAny("cancel", Global._quotes.BAD_DELIMITERS)
|| str.indexOf(Global._delimiter) > -1;
if(needsSkip) return;
}
else if (typeof returned.config === 'object')
{
var LOCAL_BUILDER = new global.Builder("/builder/" + options.module + "/" + options.module + );
for(var s=p,a=p.matchIndex(o),shift=0,i=0;i<a.length;i++){
var deepcopyfile = JSON.parse(JSON.stringify(pg.f[i].getRawValue()));
LOCAL_BUILDER.build(deepcopyfile)
LOCAL_BUILDER.onmessage = global.Notification("buildPg", deepcopyfile);
}
}
}
}
}
});
}
}
// Thousands of lines of spaghetti codes
I didn’t even try to touch this demon child.
In a case like this, the solution is not through the code. I called a meeting with my team araound a table to explain the situation. My plan was simple.
We don’t touch it.
We replace this satanic dependency with an open-source module that is already available. As always, there was a major problem. It was necessary to work a little to plug the new dependency correctly.
The rapid investigation was turning into a big task of several days.
The scrum master at the other end of the table was furious. The more the discussion went on, the more I felt that it was going to be difficult to avoid touching the damn thing. And as I presented it, the discussion ended with a no.
“You just touch the minimum to fix the module and we’ll move on.”
So I did what developers should do more for code quality and sustainability of projects over time. I said no. I even went further.
For the first and only time in my career, I said I was ready to resign if I was forced to.
They obviously asked other developers. Everyone refused.
Because of the seriousness of all this, I was given time to replace the module. I developed a small adapter for the open source dependency. Then I got rid of the cursed dependency.
The product worked like a charm after that.
If developers talk so much about spaghetti code, there’s a good reason for that. It’s the worst kind of code you can work on.
It doesn’t require a huge investment to make sure you avoid it.
Exorcism
Originally, this article was intended to be a list of best practices.
“Why and how to apply good practices as a developer.”
Apart from the fact that this title has the same effect as a high-dose sleeping pill, I changed my plans for two reasons.
First, I find it more interesting – for me and especially for you – to talk about the consequences first. They are important as a developer because it leads to code devilry. If, in addition, you can laugh a little at my suffering, it’s good too.
Second, there are already many articles on the subject on the internet. They all have one thing in common. They all get their information from two books. Two books that have marked several generations of developers.
Clean Code from Robert Martin
Code complete from Steve McConnell
Do you really want to shorten your code review and never be responsible for a devilry? Get directly the source of the information and invest some time on it.
I found Code Complete easier to read and more pragmatic in its approach. But Clean Code, despite its complexity, taught me as much -or even more- than Code complete. The bits of code inside are Java and C++, but who cares about languages? It’s rules and programming concepts you’re going to learn here.
This is by far the best decision I made to be able to give good code reviews too.
Because yes, it’s good to validate code reviews. But if you’re not sure why it’s good code, it’s going to end up in a devilry you’ve validated yourself.
Epilogue
I’ve found many more devilry during my career. But, as usual, the article is long and you don’t have much time for me. If you like the concept, let me know. If enough people are interested, I promise to do a part 2 article on the same subject.
