What's the worst piece of code you've ever seen in a production context?

[Individualised]

I'll start.

This is the code that determines what music to play on a level in the original Plants vs. Zombies game:

[Graf Zahl]

You call that one bad?

 

Unfortunately I cannot post my best examples due to confidentiality reasons but in 30 years of professional programming I have repeatedly encountered code where I seriously had to question its makers' sanity. If you want good open source examples, the Build engine got lots of good (or is that bad? :D) examples...

 

[Edward850]

Yeah @Individualised I think you might be punching a bit passed your pay grade on this one, what you posted is not even a good example of bad code. It's rather straight forward in its intent and flow, the type names make sense and it's not senselessly repetitive.

 

Also like Graf all my good examples are behind NDA so the thread asking for production code is a punt.

[Individualised]

3 minutes ago, Edward850 said:

Yeah @Individualised I think you might be punching a bit passed your pay grade on this one, what you posted is not even a good example of bad code. It's rather straight forward in its intent and flow, the type names make sense and it's not senselessly repetitive.

Given the context of the game and it's level progression I'd say it's needlessly complicated. There's also a few redundant checks in there, and those last 4 if statements are again pretty hilarious given the context of the game's level progression. It could be much simplified.

[dasho]

I think this one is more "maybe not optimal but who cares" territory.

[Edward850]

It could be but it doesn't have to be. One of the biggest dangers to production code done on a schedule is attempting to optimize something that has no reason to be optimized at all. Not only is that time and focus you could be spending elsewhere, you run the risk of actually introducing issues which your QA team then needs to waste time on reporting on something that you broke for no good reason.

[Individualised]

8 minutes ago, Edward850 said:

It could be but it doesn't have to be. One of the biggest dangers to production code done on a schedule is attempting to optimize something that has no reason to be optimized at all. Not only is that time and focus you could be spending elsewhere, you run the risk of actually introducing issues which your QA team then needs to waste time on reporting on something that you broke for no good reason.

Very true. In fact a late beta prototype of the game still has a lot of unused assets from alpha and pre-alpha builds so it's very possible they didn't have much time to clean stuff up. I assume Doom was the same too with things like it's broken status face code.

[Cammy]

You guys ever hear of Yandere Simulator

[Remilia Scarlet]

Worst?  Nah, and I wouldn't want to shit all over someone's code anyway.  But this was at least a headscratcher I encountered recently.

info->DdaFadeOutL[info->Channel] = (Sint32)((double)(PSGChn->ddaSample * PSGChn->outVolumeL) *
        ((1 + (1 >> 3) + (1 >> 4) + (1 >> 5) + (1 >> 7) + (1 >> 12) + (1 >> 14) + (1 >> 15)) * SAMPLE_FADE_DECLINE));

 

[Individualised]

4 minutes ago, Remilia Scarlet said:

Worst?  Nah, and I wouldn't want to shit all over someone's code anyway.  But this was at least a headscratcher I encountered recently.

info->DdaFadeOutL[info->Channel] = (Sint32)((double)(PSGChn->ddaSample * PSGChn->outVolumeL) *
        ((1 + (1 >> 3) + (1 >> 4) + (1 >> 5) + (1 >> 7) + (1 >> 12) + (1 >> 14) + (1 >> 15)) * SAMPLE_FADE_DECLINE));

 

I'm going to make a complete guess and assume this is for an emulator for the SN76489 sound chip?

[Remilia Scarlet]

1 minute ago, Individualised said:

I'm going to make a complete guess and assume this is for an emulator for the SN76489 sound chip?

The PSG in a HuC6280.

[Individualised]

Just now, Remilia Scarlet said:

The PSG in a HuC6820.

That's PC Engine if I'm correct? Guess I wasn't too far off...

[Mordeth]

I've seen peeps pushing code to production that did not even compile. And when prompted, did not understand why this was wrong.

[vyruss]

@Individualised have you ever shipped production code at all?

 

Like others I have some stuff I've seen that really makes the mind panic but I cannot disclose it.

[Individualised]

No, which is how I'm realising that this is a bit of a rich topic for me to make

[Lucius Wooding]

With regards to the OP example it might have been written when the game was a lot different. It looks (to my cursory glance) like it was originally just going to be "regular" and "boss" music originally. I'm willing to bet the rest of the lines were added later as more expansions and content came about, simply following the existing pattern rather than refactoring it so it looks smaller. It doesn't take a lot of time to read because after the first 2 functions your eyes should glaze over. That means it's highly readable because it's a self similar list of functions; better than having fewer lines in many cases. If I were in charge of this project, and someone else added another function at the end but didn't properly follow the pattern and style of the first few lines, I'd consider it a failing on their part and probably change it to match.

 

Consistency matters. Even if it makes us follow it to dumb looking places at times. 

 

Also, in general, optimizing or refactoring code that only runs once per game is not that important, but managing the programmer's time is very important. Use it to get things working, then optimize the stuff that runs every frame or more. The latter is a lot more important to put actual thought into, in most cases. Refactoring can be a good thing but it can be done unnecessarily; in that case it no longer saves you time in the long run. If you're working on someone else's dime then you may not have the luxury of unlimited hours to make everything perfect.

 

You want to worry about spaghetti code mainly when it lacks good performance, isn't working properly, or needs to be altered or expanded often. Those are the parts of code that will be read and reread the most, often by multiple people. A lot of code is very basic stuff that's more of a set and forget policy. 

[aRottenKomquat]

Can't go into specifics, but I once found about 20,000 lines of C code from a previous employee, all in one code file. It had no comments, no meaningful variables names. Nobody understood it, everybody was afraid to touch it, and yet it broke almost any time anybody touched anything else in the codebase. I didn't even begin to understand the code but I knew what the desired effect was. I deleted it all and rewrote the feature from scratch, and the new version was about 250 lines which included whitespace and comments.

 

You'd think at some point someone would see their code and stop to say, "This is getting ugly. I must be taking the wrong approach." Whoever wrote that never thought that, and instead just kept growing it...like cancer...until it worked...-ish.

[Mr. Freeze]

[Chookum]

Currently I do data wrangling stuff for a living and while it isn't "production" production, live and people externally needed it yesterday sort of urgent, I am not proud of some of the stuff I've written. At least without the above parameters or urgency, I can iterate on what I've got until it works acceptably for my clients and is documented enough for me to forget about safely in case someone else picks it up and runs with it later on.

 

That said, several years back I had a web development job. My first red flag experience was seeing the most chaotic indentation for a live website's code ever. Things only got worse as I realised the 15+ year old, custom made website content management system that's had three iterations to turn out like Wordpress but soggy and worse, never had a single line of documentation written. Most of the time you just copied code from another website's source to make progress on tasks. Knowledge retention, transfer, and onboarding was awful inside those walls.

 

All the above however is less of a programming error and more of a managing programming error though, and I do not have time to go into the rest of what happened there.

[Ozcar]

Either build engine or tf2 has spaghetti code.

[Blzut3]

59 minutes ago, aRottenKomquat said:

I deleted it all and rewrote the feature from scratch, and the new version was about 250 lines which included whitespace and comments.

I have a similar story.  Former employee apparently didn't really understand the concept of version control systems, and so would create a whole new copy of the code to make a change and then commit the entire copy side by side into git.  When I got there it was very unclear what was the latest version, and it turned out there were multiple latest versions with some slight differences between them.  So I ultimately refactored the whole thing so it was all a common code base.

 

It gets worse though, it turned out none of those were the latest versions.  The actual latest versions were only on the production servers.  We discovered this was the case when we deployed the refactored code and stuff broke.  In the end I technically deleted something like 97%-99% (can't remember exactly) of the code just with how much duplicated code there was.  For some reason about 40,000 lines of code deleted comes to mind, but honestly don't remember the exact figures.

[Kyka]

I have nothing to add except this meme.

 

[sandwedge]

Reminds of some old star citizen videos that showed some code in the background and horrified people.  Then they stopped doing those types of videos.  I don't know anything about programming but it seems to be a pretty common opinion that 9 nested "if" statements is a pretty bad idea lol.

 

[indigotyrian]

49 minutes ago, sandwedge said:

Reminds of some old star citizen videos that showed some code in the background and horrified people.  Then they stopped doing those types of videos.  I don't know anything about programming but it seems to be a pretty common opinion that 9 nested "if" statements is a pretty bad idea lol.

 

 

years ago someone made a video making fun of yanderedev's code and a lot of people became armchair software development experts overnight, you can tell because its usually people getting pissy at nested if statements like this

[Lucius Wooding]

This is why you should never show your code in a promo video. 1% of people who can read code will nitpick it with no context, and the other 99% who know nothing about code will join in and dunk on you forever. It certainly doesn't help when your project is a ponzi scheme to begin with.

[Mr. Freeze]

In fairness any reason to clown on Star Citizen is a good one, no matter how small. 

[KubaloBlackMT]

"Huh huh, oops. I accidentally walked into the Nerdy Corner. I better skedaddle before the girls see me hangin' round youse guys, huh huh."

[Edward850]

2 hours ago, sandwedge said:

Reminds of some old star citizen videos that showed some code in the background and horrified people.  Then they stopped doing those types of videos.  I don't know anything about programming but it seems to be a pretty common opinion that 9 nested "if" statements is a pretty bad idea lol.

 

This is not 9 nested if statements. This is a lambda callback iterator (well what looks like the word iterator, the image is too made-for-ants to be certain) doing 6 ifs of which are doing null pointer condition checks (a fairly reliable way of checking for pointer validation when doing sequential casts), followed by another lambda callback assignment for some kind of object query with another 3 ifs on screen.

 

There's actually nothing obviously wrong with this. The layout isn't the greatest but lambdas are easier to inline for their captures than define externally so this may just be structural necessity. The ifs themselves could probably be improved by using a single if and short circuiting, but given I can't see the other side of that function this may just well have another purpose we can't see, and it's not like short circuiting would make the code any faster anyway.

 

There is one weird detail in that there's a redundant if for a value that was checked before a capture already, but given this is obviously b roll footage for an in development game, I can't really even comment on that because who damn knows why it ended up like that for the exact period of time it was recorded. There's frequent periods of time where our own in development code will just have if(true) simply to help isolate something, or a redundant check because two sections of code were merged.

This critically fails the "production" aspect of the thread because this is not production code.

Edited May 14, 2023 by Edward850

[Graf Zahl]

2 hours ago, sandwedge said:

Reminds of some old star citizen videos that showed some code in the background and horrified people.  Then they stopped doing those types of videos.  I don't know anything about programming but it seems to be a pretty common opinion that 9 nested "if" statements is a pretty bad idea lol.

 

 

Some people have really strange ideas about 'bad' code.

While not the prettiest thing around, that stuff is at least readable (if the image was large enough to read its content, that is...)

Things will really turn nasty if code is so convoluted that you cannot make any sense of it anymore.

 

One example I recently encountered was some server-side code for a website that checked button presses by comparing the label's text. It didn't use any table, it didn't centralize the code in any way, it just had literal string checks scattered througout its entire code base. And half the buttons have orthographic mistakes, sometimes even the same text having different mistakes.

And since program flow is close to impenetrable, nobody knows which button may reach which part, there's nothing that can be done here. If a text is changed, random stuff may break. Migrating the checks to separate properties also wouldn't work because button texts can change by reading stuff from some data files. The whole system is a house of cards where if you take away one card you risk a catastrophic chain reaction.

So in the end, the entire website cannot be proofread, it cannot be localized and truth be told, it cannot even be edited anymore without risking some major breakage. It will have to be reprogrammed in its entirety, but so far everybody its owner tried to hire walked away in horror before starting all the work on it.

 

The true horror is not having one poorly thought out function, but having an entire system designed in a way where change will become impossible.

 

[sandwedge]

1 hour ago, Edward850 said:

This critically fails the "production" aspect of the thread because this is not production code.

 

Star citizen will technically never have production code.  Except it basically all is because they are treating it like a released live service game, and probably are going to use that legally when it all implodes.