The Daily WTF

For months, everything at Yusuf's company was fine. Then, suddenly, he comes in to the office to learn that overnight the log exploded with thousands of panic messages. No software changes had been pushed, no major configurations had happened- just a reboot. What had gone wrong?

This particular function was invoked as part of the application startup:

func (a *App) setupDocDBClient(ctx context.Context) error { docdbClient, err := docdb.NewClient( ctx, a.config.MongoConfig.URI, a.config.MongoConfig.Database, a.config.MongoConfig.EnableTLS, ) if err != nil { return nil } a.DocDBClient = docdbClient return nil }

This is Go, which passes errors as part of the return. You can see an example where docdb.NewClient returns a client and an err object. At one point in the history of this function, it did the same thing- if connecting to the database failed, it returned an error.

But a few months earlier, an engineer changed it to swallow the error- if an error occurred, it would return nil.

As an organization, they did code reviews. Multiple people looked at this and signed off- or, more likely, multiple people clicked a button to say they'd looked at it, but hadn't.

Most of the time, there weren't any connection issues. But sometimes there were. One reboot had a flaky moment with connecting, and the error was ignored. Later on in execution, downstream modules started failing, which eventually lead to a log full of panic level messages.

The change was part of a commit tagged merely: "Refactoring". Something got factored, good and hard, all right.

[Advertisement] Picking up NuGet is easy. Getting good at it takes time. Download our guide to learn the best practice of NuGet for the Enterprise.

low-case jeffphi found "Yep, all kinds of technical errors."

 

Michael R. reports an off by 900 error.

 

"It is often said that news slows down in August," notes Stewart , wondering if "perhaps The Times have just given up? Or perhaps one of the biggest media companies just doesn't care about their paying subscribers?"

 

"Zero is a dangerous idea!" exclaims Ernie in Berkeley .

 

Daniel D. found one of my unfavorites, calling it "Another classic case of cancel dialog. This time featuring KDE Partition Manager."

 

Fail? Until next time. [Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

Andreas found this in a rather large, rather ugly production code base.

private static void LogView(object o) { try { ArrayList al = (ArrayList)o; int pageId = (int)al[0]; int userId = (int)al[1]; // ... snipped: Executing a stored procedure that stores the values in the database } catch (Exception) { } }

This function accepts an object of any type, except no, it doesn't, it expect that object to be an ArrayList. It then assumes the array list will then store values in a specific order. Note that they're not using a generic ArrayList here, nor could they- it (potentially) needs to hold a mix of types.

What they've done here is replace a parameter list with an ArrayList, giving up compile time type checking for surprising runtime exceptions. And why?

"Well," the culprit explained when Andreas asked about this, "the underlying database may change. And then the function would need to take different parameters. But that could break existing code, so this allows us to add parameters without ever having to change existing code."

"Have you heard of optional arguments?" Andreas asked.

"No, all of our arguments are required. We'll just default the ones that the caller doesn't supply."

And yes, this particular pattern shows up all through the code base. It's "more flexible this way."

.comment { border: none; } [Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.

Ronan works with a vibe coder- an LLM addicted developer. This is a type of developer that's showing up with increasing frequency. Their common features include: not reading the code the AI generated, not testing the code the AI generated, not understanding the context of the code or how it integrates into the broader program, and absolutely not bothering to follow the company coding standards.

Here's an example of the kind of Python code they were "writing":

if isinstance(o, Test): if o.requirement is None: logger.error(f"Invalid 'requirement' in Test: {o.key}") try: raise ValueError("Missing requirement in Test object.") except ValueError: pass if o.title is None: logger.error(f"Invalid 'title' in Test: {o.key}") try: raise ValueError("Missing title in Test object.") except ValueError: pass

An isinstance check is already a red flag. Even without proper type annotations and type checking (though you should use them) any sort of sane coding is going to avoid situations where your method isn't sure what input it's getting. isinstance isn't a WTF, but it's a hint at something lurking off screen. (Yes, sometimes you do need it, this may be one of those times, but I doubt it.)

In this case, if the Test object is missing certain fields, we want to log errors about it. That part, honestly, is all fine. There are potentially better ways to express this idea, but the idea is fine.

No, the obvious turd in the punchbowl here is the exception handling. This is pure LLM, in that it's a statistically probable result of telling the LLM "raise an error if the requirement field is missing". The resulting code, however, raises an exception, immediately catches it, and then does nothing with it.

I'd almost think it's a pre-canned snippet that's meant to be filled in, but no- there's no reason a snippet would throw and catch the same error.

Now, in Ronan's case, this has a happy ending: after a few weeks of some pretty miserable collaboration, the new developer got fired. None of "their" code ever got merged in. But they've already got a few thousand AI generated resumes out to new positions…

[Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.

JavaScript is frequently surprising in terms of what functions it does not support. For example, while it has a Math.round function, that only rounds to the nearest integer, not an arbitrary precision. That's no big deal, of course, as if you wanted to round to, say, four decimal places, you could write something like: Math.floor(n * 10000) / 10000.

But in the absence of a built-in function to handle that means that many developers choose to reinvent the wheel. Ryan found this one.

function stripExtraNumbers(num) { //check if the number's already okay //assume a whole number is valid var n2 = num.toString(); if(n2.indexOf(".") == -1) { return num; } //if it has numbers after the decimal point, //limit the number of digits after the decimal point to 4 //we use parseFloat if strings are passed into the method if(typeof num == "string"){ num = parseFloat(num).toFixed(4); } else { num = num.toFixed(4); } //strip any extra zeros return parseFloat(num.toString().replace(/0*$/,"")); }

We start by turning the number into a string and checking for a decimal point. If it doesn't have one, we've already rounded off, return the input. Now, we don't trust our input, so if the input was already a string, we'll parse it into a number. Once we know it's a number, we can call toFixed, which returns a string rounded off to the correct number of decimal points.

This is all very dumb. Just dumb. But it's the last line which gets really dumb.

toFixed returns a padded string, e.g. (10).toFixed(4) returns "10.0000". But this function doesn't want those trailing zeros, so they convert our string num into a string, then use a regex to replace all of the trailing zeros, and then parse it back into a float.

Which, of course, when storing the number as a number, we don't really care about trailing zeros. That's a formatting choice when we output it.

I'm always impressed by a code sample where every single line is wrong. It's like a little treat. In this case, it even gets me a sense of how it evolved from little snippets of misunderstood code. The regex to remove trailing zeros in some other place in this developer's experience led to degenerate cases where they had output like 10., so they also knew they needed to have the check at the top to see if the input had a fractional part. Which the only way they knew to do that was by looking for a . in a string (have fun internationalizing that!). They also clearly don't have a good grasp on types, so it makes sense that they have the extra string check, just to be on the safe side (though it's worth noting that parseFloat is perfectly happy to run on a value that's already a float).

This all could be a one-liner, or maybe two if you really need to verify your types. Yet here we are, with a delightfully wrong way to do everything.

.comment { border: none; } [Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.

We've discussed singleton abuse as an antipattern many times on this site, but folks keep trying to find new ways to implement them badly. And Olivia's co-worker certainly found one.

We start with a C++ utility class with a bunch of functions in it:

//utilities.h class CUtilities { public CUtilities(); void doSomething(); void doSomeOtherThing(); }; extern CUtilities* g_Utility;

So yes, if you're making a pile of utility methods, or if you want a singleton object, the keyword you're looking for is static. We'll set that aside. This class declares a class, and then also declares that there will be a pointer to the class, somewhere.

We don't have to look far.

//utilities.cpp CUtilities* g_Utility = nullptr; CUtilities::CUtilities() { g_Utility = this; } // all my do-whatever functions here

This defines the global pointer variable, and then also writes the constructor of the utility class so that it initializes the global pointer to itself.

It's worth noting, at this point, that this is not a singleton, because this does nothing to prevent multiple instances from being created. What it does guarantee is that for each new instance, we overwrite g_Utility without disposing of what was already in there, which is a nice memory leak.

But where, or where, does the constructor get called?

//startup.h class CUtilityInit { private: CUtilities m_Instance; }; //startup.cpp CUtilityInit *utils = new CUtilityInit();

I don't hate a program that starts with an initialization step that clearly instantiates all the key objects. There's just one little problem here that we'll come back to in just a moment, but let's look at the end result.

Anywhere that needs the utilities now can do this:

#include "utilities.h" //in the code g_Utility->doSomething();

There's just one key problem: back in the startup.h, we have a private member called CUtilities m_Instance which is never referenced anywhere else in the code. This means when people, like Olivia, are trawling through the codebase looking for linter errors they can fix, they may see an "unused member" and decide to remove it. Which is what Olivia did.

The result compiles just fine, but explodes at runtime since g_Utility was never initialized.

The fix was simple: just don't try and make this a singleton, since it isn't one anyway. At startup, she just populated g_Utility with an instance, and threw away all the weird code around populating it through construction.

Singletons are, as a general rule, bad. Badly implemented singletons themselves easily turn into landmines waiting for unwary developers. Stop being clever and don't try and apply a design pattern for the sake of saying you used a design pattern.

.comment { border: none; } [Advertisement] Picking up NuGet is easy. Getting good at it takes time. Download our guide to learn the best practice of NuGet for the Enterprise.

It is said (allegedly by the Scots) that confession heals the soul. But does it need to be strictly voluntary? The folks toiling away over at CodeSOD conscientiously change the names to protect the innocent but this side of the house is committed to curing the tortured souls of webdevs. Whether they like it or not. Sadly Sam's submission has been blinded, so the black black soul of xxxxxxxxxxte.com remains unfortunately undesmirched, but three others should be experiencing the sweet sweet healing light of day right about now. I sure hope they appreciate it.

More monkey business this week from Reinier B. who is hoping to check in on some distant cousins. "I'll make sure to accept email from {email address}, otherwise I won't be able to visit {zoo name}."

 

Alex A. is "trying to pay customs duty." It definitely can be.

 

"I know it's hard to recruit good developers," commiserates Sam B. , "but it's like they're not even trying." They sure are, as above.

 

Peter G. bemoans "Apparently this network power thingamajig, found on Aliexpress, is pain itself if the brand name on it is to be believed." Cicero wept.

 

Jan B. takes the perfecta, hitting not only this week's theme of flubstitutions but also a bounty of bodged null references to boot. "This is one of the hardest choices I've ever had in my life. I'm not sure if I'd prefer null or null as my location.detection.message.CZ." Go in peace.

 

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.

"Code should be clear and explain what it does, comments should explain why it does that." This aphorism is a decent enough guideline, though like any guidance short enough to fit on a bumper sticker, it can easily be overapplied or misapplied.

Today, we're going to look at a comment Salagir wrote. This comment does explain what the code does, can't hope to explain why, and instead serves as a cautionary tale. We're going to take the comment in sections, because it's that long.

This is about a stored procedure in MariaDB. Think of Salagir as our Virgil, a guide showing us around the circles of hell. The first circle? A warning that the dead code will remain in the code base:

/************************** Dead code, but don't delete! What follows if the history of a terrible, terrible code. I keep it for future generations. Read it in a cold evening in front of the fireplace.

My default stance is "just delete bad, dead code". But it does mean we get this story out of it, so for now I'll allow it.

**** XXX **** This is the story of the stored procedure for getext_fields. **** XXX **** Gets the english and asked language for the field, returns what it finds: it's the translation you want. Called like this: " SELECT getext('$table.$field', $key, '$lang') as $label " The function is only *in the database you work on right now*.

Okay, this seems like a pretty simple function. But why does this say "the function is only in the database you work on right now"? That's concerning.

***** About syntax!! The code below can NOT be used by copy and paste in SQL admin (like phpmyadmin), due to the multiple-query that needs DELIMITER set. The code that works in phpmyadmin is this: DELIMITER $$ DROP FUNCTION IF EXISTS getext$$ CREATE FUNCTION (...same...) LIMIT 1; RETURN `txt_out`; END$$ However, DELIMITER breaks the code when executed from PHP.

Am I drowning in the river Styx? Why would I be copy/pasting SQL code into PhpMyAdmin from my PHP code? Is… is this a thing people were doing? Or was it going the opposite way, and people were writing delimited statements and hoping to execute them as a single query? I'm not surprised that didn't work.

***** About configuration!!! IMPORTANT: If you have two MySQL servers bind in Replication mode in order to be able to execute this code, you (or your admin) should set: SET GLOBAL log_bin_trust_function_creators = 1; Without that, adding of this function will fail (without any error).

I don't know the depths of MariaDB, so I can't comment on if this is a WTF. What leaps out to me though, is that this likely needs to be in a higher-level form of documentation, since this is a high-level configuration flag. Having it live here is a bit buried. But, this is dead code, so it's fine, I suppose.

***** About indexes!!!! The primary key was not used as index in the first version of this function. No key was used. Because the code you see here is modified for it's execution. And `field`=my_field becomes `field`= NAME_CONST('my_field',_ascii'[value]' COLLATE 'ascii_bin') And if the type of my_field in the function parameter wasn't the exact same as the type of `text`, no index is used! At first, I didn't specify the charset, and it became `field`= NAME_CONST('my_field',_utf8'[value]' COLLATE 'utf8_unicode_ci') Because utf8 is my default, and no index was used, the table `getext_fields` was read entirely each time! Be careful of your types and charsets... Also...

Because the code you see here is modified for its execution. What? NAME_CONST is meant to create synthetic columns not pulled from tables, e.g. SELECT NAME_CONST("foo", "bar") would create a result set with one column ("foo"), with one row ("bar"). I guess this is fine as part of a join- but the idea that the code written in the function gets modified before execution is a skin-peelingly bad idea. And if the query is rewritten before being sent to the database, I bet that makes debugging hard.

***** About trying to debug!!!!! To see what the query becomes, there is *no simple way*. I literally looped on a SHOW PROCESSLIST to see it! Bonus: if you created the function with mysql user "root" and use it with user "SomeName", it works. But if you do the show processlist with "SomeName", you won't see it!!

Ah, yes, of course. I love running queries against the database without knowing what they are, and having to use diagnostic tools in the database to hope to understand what I'm doing.

***** The final straw!!!!!! When we migrated to MariaDB, when calling this a lot, we had sometimes the procedure call stucked, and UNKILLABLE even on reboot. To fix it, we had to ENTIRELY DESTROY THE DATABASE AND CREATE IT BACK FROM THE SLAVE. Several times in the same month!!!

This is the 9th circle of hell, reserved for traitors and people who mix tabs and spaces in the same file. Unkillable even on reboot? How do you even do that? I have a hunch about the database trying to retain consistency even after failures, but what the hell are they doing inside this function creation statement that can break the database that hard? The good news(?) is the comment(!) contains some of the code that was used:

**** XXX **** The creation actual code, was: **** XXX **** // What DB are we in? $PGf = $Phoenix['Getext']['fields']; $db = $PGf['sql_database']? : ( $PGf['sql_connection'][3]? : ( $sql->query2cell("SELECT DATABASE()") ) ); $func = $sql->query2assoc("SHOW FUNCTION STATUS WHERE `name`='getext' AND `db`='".$sql->e($db)."'"); if ( !count($func) ) { $sql->query(<<<MYSQL CREATE FUNCTION {$sql->gt_db}getext(my_field VARCHAR(255) charset {$ascii}, my_id INT(10) UNSIGNED, my_lang VARCHAR(6) charset {$ascii}) RETURNS TEXT DETERMINISTIC BEGIN DECLARE `txt_out` TEXT; SELECT `text` INTO `txt_out` FROM {$sql->gt_db}`getext_fields` WHERE `field`=my_field AND `id`=my_id AND `lang` IN ('en',my_lang) AND `text`!='' ORDER BY IF(`lang`=my_lang, 0, 1) LIMIT 1; RETURN `txt_out`; END; MYSQL ); ... }

I hate doing string munging to generate SQL statements, but I especially hate it when the very name of the object created is dynamic. The actual query doesn't look too unreasonable, but everything about how we got here is terrifying.

**** XXX **** Today, this is not used anymore, because... **** XXX **** Because a simple sub-query perfectly works! And no maria-db bug. Thus, in the function selects() The code: //example: getext('character.name', `character_id`, 'fr') as name $sels[] = $this->sql_fields->gt_db."getext('$table.$field', $key, '$lang') as `$label`"; Is now: $sels[] = "(SELECT `text` FROM {$this->sql_fields->gt_db}`getext_fields` WHERE `field`='$table.$field' AND `lang` IN ('en', '$lang') AND `id`=$key AND `text`!='' ORDER BY IF(`lang`='$lang', 0, 1) LIMIT 1) as `$label`"; Less nice to look at, but no procedure, all the previous problems GONE! **** XXX The end. */

Of course a simple subquery (or heck, probably a join!) could handle this. Linking data across two tables is what databases are extremely good at. I agree that, at the call site, this is less readable, but there are plenty of ways one could clean this up to make it more readable. Heck, with this, it looks a heck of a lot like you could have written a much simpler function.

Salagir did not provide the entirety of the code, just this comment. The comment remains in the code, as a warning sign. That said, it's a bit verbose. I think a simple "Abandon all hope, ye who enter here," would have covered it.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

While I frequently have complaints about over-reliance on Object Relational Mapping tools, they do offer key benefits. For example, mapping each relation in the database to a type in your programming language at least guarantees a bit of type safety in your code. Or, you could be like Nick L's predecessor, and write VB code like this.

For i As Integer = 0 To SQLDataset.Tables(0).Rows.Count - 1 Try 'Handles DBNull Select Case SQLDataset.Tables(0).Rows(i).Item(0) Case "Bently" 'Probes Probes_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim) Case "Keyphasor" Keyphasor_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim) Case "Transmitter" Transmitter_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim) Case "Tachometer" Tachometer_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim.ToUpper.ToString.Trim) Case "Dial Therm" DialThermometer_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim) Case "DPS" DPS_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim) Case "Pump Bracket" PumpBracket_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim) Case "Accelerometer" Accelerometer_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim) Case "Velometer" Velometer_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim) End Select Catch 'MessageBox.Show(text:="Error during SetModelNums().", _ ' caption:="Error", _ ' buttons:=MessageBoxButtons.OK, _ ' icon:=MessageBoxIcon.Error) End Try Next

So, for starters, they're using the ADO .Net DataSet object. This is specifically meant to be a disconnected, in-memory model of the database. The idea is that you might run a set of queries, store the results in a DataSet, and interact with the data entirely in memory after that point. The resulting DataSet will model all the tables and constraints you've pulled in (or allow you to define your own in memory).

One of the things that the DataSet tracks is the names of tables. So, the fact that they go and access .Table(0) is a nuisance- they could have used the name of the table. And while that might have been awfully verbose, there's nothing stopping them from doing DataTable products = SQLDataSet.Tables("Products").

None of this is what caught Nick's attention, though. You see, the DataTable in the DataSet will do its best to map database fields to .NET types. So it's the chain of calls at the end of most every field that caught Nick's eye:

SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim

ToUpper works because the field in the database is a string field. Also, it returns a string, so there's no need to ToString it before trimming. Of course, it's the Tachometer entry that brings this to its natural absurdity:

Tachometer_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim.ToUpper.ToString.Trim)

All of this is wrapped up in an exception handler, not because of the risk of an error connecting to the database (the DataSet is disconnected after all), but because of the risk of null values, as the comment helpfully states.

We can see that once, this exception handler displayed a message box, but that has since been commented out, presumably because there are a lot of nulls and the number of message boxes the users had to click through were cumbersome. Now, the exception handler doesn't actually check what kind of exception we get, and just assumes the only thing that could happen was a null value. But that's not true- someone changed one of the tables to add a column to the front, which meant Item(1) was no longer grabbing the field the code expects, breaking the population of the Pump Bracket combo box. There was no indication that this had happened beyond users asking, "Why are there no pump brackets anymore?"

[Advertisement] Plan Your .NET 9 Migration with Confidence
Your journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!

Michael has the "fun" task of converting old, mainframe-driven reports into something more modern. This means reading through reams of Intelligent Query code.

Like most of these projects, no one has a precise functional definition of what it's supposed to do. The goal is to replace the system with one that behaves exactly the same, but is more "modern". This means their test cases are "run the two systems in parallel and compare the outputs; if they match, the upgrade is good."

After converting one report, the results did not match. Michael dug in, tracing through the code. The name of the report contained the word "Annual". One of the key variables which drove original the report was named TODAYS-365 (yes, you can put dashes in variables in IQ). Michael verified that the upgraded report was pulling exactly one year's worth of data. Tracing through the original report, Michael found this:

# DIVIDE ISBLCS BY ISB-COST-UOM GIVING ISB-COST-EACH. MULTIPLY ISB-STK-QOH TIMES ISB-COST-EACH GIVING ISB-ON-HAND-COST. # SUBTRACT TODAYS-DATE MINUS 426 GIVING TODAYS-365. # SEARCH FOR ITMMAN = 'USA' AND ITMNMB <> '112-*'

This snippet comes from a report which contains many hundreds of lines of code. So it's very easy to understand how someone could miss the important part of the code. Specifically, it's this line: SUBTRACT TODAYS-DATE MINUS 426 GIVING TODAYS-365..

Subtract 426 from today's date, and store the result in a variable called TODAYS-365. This report isn't for the past year, but for the past year and about two months.

It's impossible to know exactly why, but at a guess, originally the report needed to grab a year. Then, at some point, the requirement changed, probably based on some nonsense around fiscal years or something similar. The least invasive way to make that change was to just change the calculation, leaving the variable name (and the report name) incorrect and misleading. And there it say, working perfectly fine, until poor Michael came along, trying to understand the code.

The fix was easy, but the repeated pattern of oddly name, unclear variables was not. Remember, the hard part about working on old mainframes isn't learning COBOL or IQ or JCL or whatever antique languages they use; I'd argue those languages are in many cases easier to learn (if harder to use) than modern languages. The hard part is the generations of legacy kruft that's accumulated in them. It's grandma's attic, and granny was a pack rat.

[Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.

User inputs are frequently incorrect, which is why we validate them. So, for example, if the user is allowed to enter an "asset ID" to perform some operation on it, we should verify that the asset ID exists before actually doing the operation.

Someone working with Capybara James almost got there. Almost.

private boolean isAssetIdMatching(String requestedAssetId, String databaseAssetId) { return (requestedAssetId + "").equals(databaseAssetId + ""); }

This Java code checks if the requestedAssetId, provided by the user, matches a databaseAssetId, fetched from the database. I don't fully understand how we get to this particular function. How is the databaseAssetId fetched? If the fetch were successful, how could it not match? I fear they may do this in a loop across all of the asset IDs in the database until they find a match, but I don't know that for sure, but the naming conventions hint at a WTF.

The weird thing here, though, is the choice to concatenate an empty string to every value. There's no logical reason to do this. It certainly won't change the equality check. I strongly suspect that the goal here was to protect against null values, but it doesn't work that way in Java. If the string variables are null, this will just throw an exception when you try and concatenate.

I strongly suspect the developer was more confident in JavaScript, where this pattern "works".

I don't understand why or how this function got here. I'm not the only one. James writes:

No clue what the original developers were intending with this. It sure was a shocker when we inherited a ton of code like this.

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.

If monkeys aren't your bag, Manuel H. is down to clown. "If anyone wants to know the address of that circus - it's written right there. Too bad that it's only useful if you happen to be in the same local subnet..." Or on the same block.

 

"Where's my pension?" paniced Stewart , explaining "I logged on to check my Aegon pension to banish the Monday blues and my mood only worsened!"

 

After last week's episode, BJH is keeping a weather eye out for freak freezes. "The Weather.com app has something for everyone. Simple forecasts almost anyone can understand, and technical jargon for the geeks."

 

"It costs too much to keep a salmon on the road," complains Yitzchok Nolastname . I agree this result looks fishy.

 

"At Vanity Fair, brevity is the soul of wit," notes jeffphi . Always has been.

 

[Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.

In my career, several times I've ended up being the pet programmer for a team of engineers and CNC operators, which frequently meant helping them do automation in their CAD tools. At its peak complexity, it resulted in a (mostly unsuccessful) attempt to build a lens/optics simulator in RhinoCAD.

Which brings us to the code Nick L sends us. It sounds like Nick's in a similar position: engineers write VB.Net code to control their CAD tool, and then Nick tries desperately to get them to follow some sort of decent coding practice. The result is code like:

'Looping Through S_Parts that have to be inital created For Each Item As Object In RootPart.S_PartsToCreate If Item.objNamDe IsNot String.Empty Then If Item.objNamEn IsNot String.Empty Then If Item.artCat IsNot String.Empty Then If Item.prodFam IsNot String.Empty Then If Item.prodGrp IsNot String.Empty Then 'Checking if the Mandatory Properties are in the partfamilies and not empty If Item.Properties.ContainsKey("From_sDesign") Then ' I omitted 134 lines of logic that really should be their own function Else MsgBox("Property From_SDesign is missing or empty.", MsgBoxStyle.DefaultButton2, "Information RS2TC") Exit Sub End If Else MsgBox("Property prodGrp is missing or empty.", MsgBoxStyle.DefaultButton2, "Information RS2TC") Exit Sub End If Else MsgBox("Property prodFam is missing or empty.", MsgBoxStyle.DefaultButton2, "Information RS2TC") Exit Sub End If Else MsgBox("Property artCat is missing or empty.", MsgBoxStyle.DefaultButton2, "Information RS2TC") Exit Sub End If Else MsgBox("objNamEn is missing or empty.", MsgBoxStyle.DefaultButton2, "Information RS2TC") Exit Sub End If Else MsgBox("objNamDe is missing or empty.", MsgBoxStyle.DefaultButton2, "Information RS2TC") Exit Sub End If Next

All of their code is stored in a single file called Custom.vb, and it is not stored in source control. Yes, people overwrite each other's code all the time, and it causes endless problems.

Nick writes:

I really wish we'd stop letting engineers code without supervision. Someone should at least tell them about early returns.

.comment { border: none; } [Advertisement] Picking up NuGet is easy. Getting good at it takes time. Download our guide to learn the best practice of NuGet for the Enterprise.

Carlos G found some C++ that caused him psychic harm, and wanted to know how it ended up that way. So he combed through the history. Let's retrace the path with him.

Here was the original code:

void parseExpiryDate (const char* expiryDate) { // expiryDate is in "YYMM" format int year, month; sscanf(expiryDate, "%2d%2d", &year, &month); //... }

This code takes a string containing an expiry date, and parses it out. The sscanf function is given a format string describing two, two digit integers, and it stores those values into the year and month variables.

But oops! The expiry date is actually in a MMYY format. How on earth could we possibly fix this? It can't be as simple as just swapping the year and month variables in the sscanf call, can it? (It is.) No, it couldn't be that easy. (It is.) I can't imagine how we would solve this problem. (Just swap them!)

void parseExpiryDate(const char* expiryDate) { // expiryDate is in "YYMM" format but, in some part of the code, it is formatted to "MMYY" int year, month; char correctFormat[5]; correctFormat[0] = expiryDate[2]; correctFormat[1] = expiryDate[3]; correctFormat[2] = expiryDate[0]; correctFormat[3] = expiryDate[1]; correctFormat[4] = '\0'; sscanf(correctFormat, "%2d%2d", &year, &month); //... }

There we go! That was easy! We just go, character by character, and shift the order around and copy it to a new string, so that we format it in YYMM.

The comment here is a wonderful attempt at CYA. By the time this function is called, the input is in MMYY, so that's the relevant piece of information to have in the comment. But the developer really truly believed that YYMM was the original input, and thus shifts blame for the original version of this function to "some part of the code" which is shifting the format around on them, thus justifying… this trainwreck.

Carlos replaced it with:

void parseExpiryDate (const char* expiryDate) { // expiryDate is in "MMYY" format int month, year; sscanf(expiryDate, "%2d%2d", &month, &year); //... } .comment { border: none; } [Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.

To ensure that several services could only be invoked by trusted parties, someone at Ricardo P's employer had the brilliant idea of requiring a token along with each request. Before servicing a request, they added this check:

private bool IsValidToken(string? token) { if (string.Equals("xxxxxxxx-xxxxxx+xxxxxxx+xxxxxx-xxxxxx-xxxxxx+xxxxx", token)) return true; return false; }

The token is anonymized here, but it's hard-coded into the code, because checking security tokens into source control, and having tokens that never expire has never caused anyone any trouble.

Which, in the company's defense, they did want the token to expire. The problem there is that they wanted to be able to roll out the new token to all of their services over time, which meant the system had to be able to support both the old and new token for a period of time. And you know exactly how they handled that.

private bool IsValidToken(string? token) { if (string.Equals("xxxxxxxx-xxxxxx+xxxxxxx+xxxxxx-xxxxxx-xxxxxx+xxxxx", token)) return true; else if (string.Equals("yyyyyyy-yyyyyy+yyyyy+yyyyy-yyyyy-yyyyy+yyyy", token)) return true; return false; }

For a change, I'm more mad about this insecurity than the if(cond) return true pattern, but boy, I hate that pattern.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

The Standard Template Library for C++ is… interesting. A generic set of data structures and algorithms was a pretty potent idea. In practice, early implementations left a lot to be desired. Because the STL is a core part of C++ at this point, and widely used, it also means that it's slow to change, and each change needs to go through a long approval process.

Which is why the STL didn't have a std::map::containsfunction until the C++20 standard. There were other options. For example, one could usestd::map::count, to count how many times a key appear. Or you could use std::map::findto search for a key. One argument against adding astd::map::containsfunction is thatstd::map::count basically does the same job and has the same performance.

None of this stopped people from adding their own. Which brings us to Gaetan's submission. Absent a std::map::contains method, someone wrote a whole slew of fieldExists methods, where field is one of many possible keys they might expect in the map.

bool DataManager::thingyExists (string name) { THINGY* l_pTHINGY = (*m_pTHINGY)[name]; if(l_pTHINGY == NULL) { m_pTHINGY->erase(name); return false; } else { return true; } return false; }

I've head of upsert operations- an update and insert as the same operation, but this is the first exert- an existence check and an insert in the same operation.

"thingy" here is anonymization. The DataManager contained several of these methods, which did the same thing, but checked a different member variable. Other classes, similar to DataManager had their own implementations. In truth, the original developer did a lot of "it's a class, but everything inside of it is stored in a map, that's more flexible!"

In any case, this code starts by using the [] accessor on a member variable m_pTHINGY. This operator returns a reference to what's stored at that key, or if the key doesn't exist inserts a default-constructed instance of whatever the map contains.

What the map contains, in this case, is a pointer to a THINGY, so the default construction of a pointer would be null- and that's what they check. If the value is null, then we erase the key we just inserted and return false. Otherwise, we return true. Otherotherwise, we return false.

As a fun bonus, if someone intentionally stored a null in the map, this will think the key doesn't exist and as a side effect, remove it.

Gaetan writes:

What bugs me most is the final, useless return.

I'll be honest, what bugs me most is the Hungarian notation on local variables. But I'm long established as a Hungarian notation hater.

This code at least works, which compared to some bad C++, puts it on a pretty high level of quality. And it even has some upshots, according to Gaetan:

On the bright side: I have obtained easy performance boosts by performing that kind of cleanup lately in that particular codebase.

[Advertisement] Picking up NuGet is easy. Getting good at it takes time. Download our guide to learn the best practice of NuGet for the Enterprise.

Or cold. It's getting hot and cold. But on average... no. It's absolutely unbelievable.

"There's been a physics breakthrough!" Mate exclaimed. "Looking at meteoblue, I should probably reconsider that hike on Monday." Yes, you should blow it off, but you won't need to.

 

An anonymous fryfan frets "The yellow arches app (at least in the UK) is a buggy mess, and I'm amazed it works at all when it does. Whilst I've heard of null, it would appear that they have another version of null, called ullnullf! Comments sent to their technical team over the years, including those with good reproduceable bugs, tend to go unanswered, unfortunately."

 

Llarry A. whipped out his wallet but baffled "I tried to pay in cash, but I wasn't sure how much."

 

"Github goes gonzo!" groused Gwenn Le Bihan. "Seems like Github's LLM model broke containment and error'd all over the website layout. crawling out of its grouped button." Gross.

 

Peter G. gripes "The text in the image really says it all." He just needs to rate his experience above 7 in order to enable the submit button.

 

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

Mads introduces today's code sample with this line: " this was before they used git to track changes".

Note, this is not to say that they were using SVN, or Mercurial, or even Visual Source Safe. They were not using anything. How do I know?

/** * Converts HTML to PDF using HTMLDOC. * * @param printlogEntry ** @param inBytes * html. * @param outPDF * pdf. * @throws IOException * when error. * @throws ParseException */ public void fromHtmlToPdfOld(PrintlogEntry printlogEntry, byte[] inBytes, final OutputStream outPDF) throws IOException, ParseException {...} /** * Converts HTML to PDF using HTMLDOC. * * @param printlogEntry ** @param inBytes * html. * @param outPDF * pdf. * @throws IOException * when error. * @throws ParseException */ public void fromHtmlToPdfNew(PrintlogEntry printlogEntry, byte[] inBytes, final OutputStream outPDF) throws IOException, ParseException {...}

Originally, the function was just called fromHtmlToPdf. Instead of updating the implementation, or using it as a wrapper to call the correct implementation, they renamed it to Old, added one named New, then let the compiler tell them where they needed to update the code to use the new implementation.

Mads adds: "And this is just one example in this code. This far, I have found 5 of these."

.comment { border: none; } [Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.

I am on record as not particularly loving JSON as a serialization format. It's fine, and I'm certainly not going to die on any hills over it, but I think that as we stripped down the complexity of XML we threw away too much.

On the flip side, the simplicity means that it's harder to use it wrong. It's absent many footguns.

Well, one might think. But then Hootentoot ran into a problem. You see, an internal partner needed to send them a JSON document which contains a JSON document. Now, one might say, "isn't any JSON object a valid sub-document? Can't you just nest JSON inside of JSON all day? What could go wrong here?"

"value":"[{\"value\":\"1245\",\"begin_datum\":\"2025-05-19\",\"eind_datum\":null},{\"value\":\"1204\",\"begin_datum\":\"2025-05-19\",\"eind_datum\":\"2025-05-19\"}]",

This. This could go wrong. They embedded JSON inside of JSON… as a string.

Hootentoot references the hottest memes of a decade and a half ago to describe this Xzibit:

Yo dawg, i heard you like JSON, so i've put some JSON in your JSON

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

"This keeps giving me a primary key violation!" complained one of Nancy's co-workers. "Screw it, I'm dropping the primary key constraint!"

That was a terrifying thing to hear someone say out loud. Nancy decided to take a look at the table before anyone did anything they'd regret.

CREATE TYPE record_enum AS ENUM('parts'); CREATE TABLE IF NOT EXISTS parts ( part_uuid VARCHAR(40) NOT NULL, record record_enum NOT NULL, ... ... ... PRIMARY KEY (part_uuid, record) );

This table has a composite primary key. The first is a UUID, and the second is an enum with only one option in it- the name of the table. The latter column seems, well, useless, and certainly isn't going to make the primary key any more unique. But the UUID column should be unique. Universally unique, even.

Nancy writes:

Was the UUID not unique enough, or perhaps it was too unique?! They weren't able to explain why they had designed the table this way.

Nor were they able to explain why they kept violating the primary key constraint. It kept happening to them, for some reason until eventually it stopped happening, also for some reason.

[Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.