Today we have a whole batch of category errors, picked out from the rash of submissions and a few that have been festering on the shelf. Just for fun, I threw in an ironic off-by-some meta-error. See if you can spot it.

Adam R. "I'm looking for hotel rooms for the 2026 Winter Olympics in Milan-Cortina. Most hotels haven't opened up reservations yet, except for ridiculously overprice hospitality packages. This search query found NaN facilities available, which equates to one very expensive apartment. I guess one is not a number now?"

 

Intrepid traveler BJH had a tough time at the Intercontinental. I almost feel sympathy. Almost. "I stare at nulls at home all the time so it made me feel comfortable to see them at the hotel when traveling. And what is that 'INTERCONTINENTAL W...' at the top? I may never know!"

 

Hoping to find out, BJ clicked through the mystery menu and discovered... this. But even worse, "There was no exit: Clicking Exit did nothing and neither did any of the buttons on the remote. Since I'd received more entertainment than usual from a hotel screen I just turned everything off."

 

Striking out for some streaming entertainment Dmitry NoLastName was silently stymied by this double-decker from Frontier.com.

 

No streaming needed for Barry M. who can get a full dose of fun from those legacy broadcast channels! Whatever did they do before null null undefined null? "Hey, want to watch TV tonight? NaN."

 

Hah! "That's MISTER Null, to you," declared an anonymous contributor.

 

And finally, another entirely different anonymous contributor clarified that there are apparently NaN cellphone towers in Switzerland. Personally, I'm intrigued by the existence of that one little crumb of English on an otherwise entirely German page.

 

[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.

Jenny had been perfectly happy working on a series of projects for her company, before someone said, "Hey, we need you to build a desktop GUI for an existing API."

The request wasn't the problem, per se. The API, on the other hand, absolutely was.

The application Jenny was working on represented a billing contract for materials consumed at a factory. Essentially, the factory built a bunch of individual parts, and then assembled them into a finished product. They only counted the finished product, but needed to itemize the billing down to not only the raw materials that went into the finished product, the intermediate parts, but also the toilet paper put in the bathrooms. All the costs of operating the factory were derived from the units shipped out.

This meant that the contract itself was a fairly complicated tree structure. Jenny's application was meant to both visualize and allow users to edit that tree structure to update the billing contract in sane, and predictable ways, so that it could be reviewed and approved and when the costs of toilet paper went up, those costs could be accurately passed on to the customer.

Now, all the contract management was already implemented and lived library that itself called back into a database. Jenny just needed to wire it up to a desktop UI. Part of the requirements were that line items in the tree needed to have a special icon displayed next to them under two conditions: if one of their ancestors in the tree had been changed since the last released contract, or if they child was marked as "inherit from parent".

The wrapper library wasn't documented, so Jenny asked the obvious question: "What's the interface for this?"

The library team replied with this:

IModelInheritFromParent : INotifyPropertyChanged { bool InheritFromParent {get; set;} }

"That covers the inheritance field," Jenny said, "but that doesn't tell me if the ancestor has been modified."

"Oh, don't worry," the devs replied, "there's an extension method for that."

public bool GetChangedIndicator(this IModelTypeA);

Extension methods in C# are just a way to use syntactic sugar to "add" methods to a class: IModelTypeA does not have a GetChangedIndicator method, but because of the this keyword, it's an extension method and we can now invoke aInstance.GetChangedIndicator(). It's how many built-in .Net APIs work, but like most forms of syntactic sugar, while it can be good, it usually makes code harder to understand, harder to test, and harder to debug.

But Jenny's main complaint was this: "You can't raise an event or something? I'm going to need to poll?"

"Yes, you're going to need to poll."

Jenny didn't like the idea of polling the (slow) database, so at first, she tried to run the polling in a background thread so it wouldn't block the UI. Unfortunately for her, the library was very much not threadsafe, so that blew up. She ended up needing to poll on the main UI thread, which meant the application would frequently stall while users were working. She did her best to minimize it, but it was impossible to eliminate.

But worse than that, each contract item may implement one of four interfaces, which meant there were four versions of the extension method:

public bool GetChangedIndicator(this IModelTypeA); public bool GetChangedIndicator(this IModelTypeB); public bool GetChangedIndicator(this IModelTypeC); public bool GetChangedIndicator(this IModelTypeD);

To "properly" perform the check, Jenny would have to check which casts were valid for a given item, cast it, and then invoke GetChangedIndicator. It's worth noting that had they just used regular inheritance instead of extension methods, this wouldn't have been necessary at all. Using the "fun" syntactic sugar made the code more complicated for no benefit.

This left Jenny with another question: "What if an item implements more than one of these interfaces? What if the extension methods disagree on if the item is changed?"

"Good question," the team responsible for the library replied. "That should almost never happen."

Jenny quit not long after this.

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

Once upon a time, Ryan's company didn't use a modern logging framework to alert admins when services failed. No, they used everyone's favorite communications format, circa 2005: email. Can't reach the database? Send an email. Unhandled exception? Send an email. Handled exception? Better send an email, just in case. Sometimes they go to admins, sometimes they just go to an inbox used for logging.

Let's look at how that worked.

public void SendEMail(String receivers, String subject, String body) { try { System.Net.Mail.SmtpClient clnt = new System.Net.Mail.SmtpClient(ConfigurationManager.AppSettings["SmtpServer"]); clnt.Send(new System.Net.Mail.MailMessage( ConfigurationManager.AppSettings["Sender"], ConfigurationManager.AppSettings["Receivers"], subject, body)); } catch (Exception ex) { SendEMail( ConfigurationManager.AppSettings["ErrorLogAddress"], "An error has occurred while sending an email", ex.Message + "\n" + ex.StackTrace); } }

They use the Dot Net SmtpClient class to connect to an SMTP server and send emails based on the configuration. So far so good, but what happens when we can't send an email because the email server is down? We'll get an exception, and what do we do with it?

The same thing we do with every other exception: send an email.

Ryan writes:

Strangely enough, I've never heard of the service crashing or hanging. We must have a very good mail server!

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

Alex had the misfortune to work on the kind of application which has forms with gigantic piles of fields, stuffed haphazardly into objects. A single form could easily have fifty or sixty fields for the user to interact with.

That leads to C# code like this:

private static String getPrefix(AV_Suchfilter filter) { String pr = String.Empty; try { int maxLength = 0; if (filter.Angebots_id != null) { maxLength = getmaxLength(maxLength, AV_MessagesTexte.Reportliste_sf_angebotsID.Length); } if (filter.InternesKennzeichen != null) { if (filter.InternesKennzeichen.Trim() != String.Empty) { maxLength = getmaxLength(maxLength, AV_MessagesTexte.Reportliste_sf_internesKennzeichen.Length); } } if (filter.Angebotsverantwortlicher_guid != null) { maxLength = getmaxLength(maxLength, AV_MessagesTexte.Reportliste_sf_angebotsverantwortlicher.Length); } // Do this another 50 times.... // and then .... int counter = 0; while (counter < maxLength) { pr += " "; counter++; } } catch (Exception error) { ErrorForm frm = new ErrorForm(error); frm.ShowDialog(); } return pr; }

The "Do this another 50 times" is doing a lot of heavy lifting in here. What really infuriates me about it, though, which we can see here, is that not all of the fields we're looking at are parameters to this function. And because the function here is static, they're not instance members either. I assume AV_MessagesTexte is basically a global of text labels, which isn't a bad way to manage such a thing, but functions should still take those globals as parameters so you can test them.

I'm kidding, of course. This function has never been tested.

Aside from a gigantic pile of string length comparisons, what does this function actually do? Well, it returns a new string which is a number of spaces exactly equal to the length of the longest string. And the way we build that output string is not only through string concatenation, but the use of a while loop where a for loop makes more sense.

Also, just… why? Why do we need a spaces-only-string the length of another string? Even if we're trying to do some sort of text layout, that seems like a bad way to do whatever it is we're doing, and also if that's the case, why is it called getPrefix? WHY IS OUR PREFIX A STRING OF SPACES THE LENGTH OF OUR FIELD? HOW IS THAT A PREFIX?

I feel like I'm going mad.

But the real star of this horrible mess, in my opinion, is the exception handling. Get an exception? Show the user a form! There's no attempt to decide if or how we could recover from this error, we just annoy the user with it.

Which isn't just unique to this function. Notice the getmaxLength function? It's really a max and it looks like this:

private static int getmaxLength(int old, int current) { int result = old; try { if (current > old) { result = current; } } catch (Exception error) { ErrorForm frm = new ErrorForm(error); frm.ShowDialog(); } return result; }

What's especially delightful here is that this function couldn't possibly throw an exception. And you know what that tells me? This try/catch/form pattern is just their default error handling. They spam this everywhere, in every function, and the tech lead or architect pats themselves on the back for ensuring that the application "never crashes!" all the while annoying the users with messages they can't do anything about.

.comment { border: none; } [Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

Fresh out of university, Remco accepted a job that allowed him to relocate to a different country. While entering the workforce for the first time, he was also adjusting to a new home and culture, which is probably why the red flags didn't look quite so red.

The trouble had actually begun during his interview. While being questioned about his own abilities, Remco learned about Conglomcorp's healthy financial position, backed by a large list of clients. Everything seemed perfect, but Remco had a bad gut feeling he could neither explain nor shake off. Being young and desperate for a job, he ignored his misgivings and accepted the position. He hadn't yet learned how scarily accurate intuition often proves to be.

The second red flag was run up the mast at orientation. While teaching him about the company's history, one of the senior managers proudly mentioned that Conglomcorp had recently fired 50% of their workforce, and were still doing great. This left Remco feeling more concerned than impressed, but he couldn't reverse course now.

Flag number three waved during onboarding, as Remco began to learn about the Java application he would be helping to develop. He'd been sitting at the cubicle of Lars, a senior developer, watching over his shoulder as Lars familiarized him with the application's UI.

"Garbage Collection." Using his mouse, Lars circled a button in the interface labeled just that. "We added this to solve a bug some users were experiencing. Now we just tell everyone that if they notice any weird behavior in the application, they should click this button."

Remco frowned. "What happens in the code when you click that?"

"It calls System.gc()."

But that wasn't even guaranteed to run! The Java virtual machine handled its own garbage collection. And in no universe did you want to put a worse-than-useless button in your UI and manipulate clients into thinking it did something. But Remco didn't feel confident enough to speak his mind. He kept silent and soldiered on.

When Remco was granted access to the codebase, it got worse. The whole thing was a pile of spaghetti full of similar design brillance that mostly worked well enough to satisfy clients, although there was a host of bugs in the bug tracker, some of which had been rotting there for over 7 years. Remco had been given the unenviable task of fixing the oldest ones.

Remco slogged through another few months. Eventually, he was tasked with implementing a new feature that was supposed to be similar to existing features already in the application. He checked these other features to see how they were coded, intending to follow the same pattern. As it turned out, each and every one of them had been implemented in a different, weird way. The wheel had been reinvented over and over, and none of the implementations looked like anything he ought to be imitating.

Flummoxed, Remco approached Lars' cubicle and explained his findings. "How should I proceed?" he finally asked.

Lars shrugged, and looked up from a running instance of the application. "I don't know." Lars turned back to his screen and pushed "Garbage Collect".

Fairly soon after that enlightening experience, Remco moved on. Conglomcorp is still going, though whether they've retained their garbage collection button is anyone's guess.

[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.

I'm not entirely sure I understand the first item today, but maybe you can help. I pulled a couple of older items from the backlog to round out this timely theme.

Rudi A. reported this Errord, chortling "Time flies when you're having fun, but it goes back when you're walking along the IJ river!" Is the point here that the walking time is quoted as 77 minutes total, but the overall travel time is less than that? I must say I don't recommend swimming the Ij in March, Rudi.

 

I had to go back quite a while for this submission from faithful reader Adam R., who chimed "I found a new type of datetime handling failure in this timestamp of 12:8 PM when checking my past payments at my medical provider." I hope he's still with us.

 

Literary critic Jay commented "Going back in time to be able to update your work after it gets published but before everyone else in your same space time fabric gets to see your mistakes, that's privilege." This kind of error is usually an artifact of Daylight Saving Time, but it's a day too late.

 

Lucky Luke H. can take his time with this deal. "The board is proud to approve a 20% discount for the next 8 millenia," he crowed.

 

At nearly the other end of the entire modern era, Carlos found himself with a nostalgic device. "Excel crashed. When it came back, it did so showing this update banner." Some programmer confused "restore state" with the English Restoration. Not that state, bub.

 

[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.

I remember in some intro-level compsci class learning that credit card numbers were checksummed, and writing basic functions to validate those checksums as an exercize. I was young and was still using my "starter" credit card with a whopping limit of $500, so that was all news to me.

Alex's company had a problem processing credit cards: they rejected a lot of credit cards as being invalid. The checksum code seemed to be working fine, so what could the problem be? Well, the problem became more obvious when someone's card worked one day, and stopped working the very next day, and they just so happened to be the first and last day of the month.

protected function validateExpirationCcDate($i_year, $i_month) { return (((int)strftime('%y') <= $i_year) && ((int)strftime ('%m') <= $i_month))? true : false; }

This function is horrible; because it uses strftime (instead of taking the comparison date and time as a parameter) it's not unit-testable. We're (ab)using casts to convert strings into integers so we can do our comparison. We're using a ternary to return a boolean value instead of just returning the result of the boolean expression.

But of course, that's all the amuse bouche: the main course is the complete misunderstanding of basic logic. A credit card is valid if the expiration year is less than or equal to the current year and the month is less than or equal to the current month. As this article goes live in March, 2025, this code would allow credit cards from April, 2026, as it should. But it would reject any cards with an expiration of February, 2028.

Per Alex, "This is a credit card date validation that has been in use for ages."

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

We previously discussed some whitespacing choices in a C++ codebase. Tim promised that there were more WTFs lurking in there, and has delivered one.

Let's start with this class constructor:

QBatch_arithExpr::QBatch_arithExpr(QBatch_unOp, const QBatch_snippet &, const QBatch_snippet &);

You'll notice that this takes a parameter of type QBatch_unOp. What is that type? Well, it's an enumerated type describing the kind of operation this arithExpr represents. That is to say, they're not using real inheritance, but instead switching on the QBatch_unOp value to decide which code branch to execute- hand-made, home-grown artisanal inheritance. And while there are legitimate reasons to avoid inheritance, this is a clear case of "is-a" relationships, and it would allow compile-time checking of how you combine your types.

Tim also points out the use of the "repugnant west const", which is maybe a strong way to word it, but definitely using only "east const" makes it a lot easier to understand what the const operator does. It's worth noting that in this example, the second parameters is a const reference (not a reference to a const value).

Now, they are using inheritance, just not in that specific case:

class QBatch_paramExpr : public QBatch_snippet {...};

There's nothing particularly wrong with this, but we're going to use this parameter expression in a moment.

QBatch_arithExpr* Foo(QBatch_snippet *expr) { // snip QBatch_arithExpr *derefExpr = new QBatch_arithExpr(enum_tag1, *(new QBatch_paramExpr(paramId))); assert(derefExpr); return new QBatch_arithExpr(enum_tag2, *expr, *derefExpr); }

Honestly, in C++ code, seeing a pile of "*" operators and raw pointers is a sign that something's gone wrong, and this is no exception.

Let's start with calling the QBatch_arithExpr constructor- we pass it *(new QBatch_paramExpr(paramId)), which is a multilayered "oof". First, the new operator will heap allocate and construct an object, and return a pointer to that object. We then dereference that pointer, and pass the value as a reference to the constructor. This is an automatic memory leak; because we never trap the pointer, we never have the opportunity to release that memory. Remember kids, in C/C++ you need clear ownership semantics and someone needs to be responsible for deallocating all of the allocated memory- every new needs a delete, in this case.

Now, new QBatch_arithExpr(...) will also return a pointer, which we put in derefExpr. We then assert on that pointer, confirming that it isn't null. Which… it can't be. A constructor may fail and throw an exception, but you'll never get a null (now, I'm sure a sufficiently motivated programmer can mix nothrow and -fno-exceptions to get constructors to return null, but that's not happening here, and shouldn't happen anywhere).

Then we dereference that pointer and pass it to QBatch_arithExpr- creating another memory leak. Two memory leaks in three lines of code, where one line is an assert, is fairly impressive.

Elsewhere in the code, shared_pointer objects are used, wit their names aliased to readable types, aka QBatch_arithExpr::Ptr, and if that pattern were followed here, the memory leaks would go away.

As Tim puts it: "Some folks never quite escaped their Java background," and in this case, I think it shows. Objects are allocated with new, but never deleted, as if there's some magical garbage collector which is going to find the unused objects and free them.

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

Marco found this wreck, left behind by a former co-worker:

$("#image_sample").html('<i><br /><br /><br /><br /><br /><br /><br /><br /><br /><br /><br />No image selected, select an image to see how it looks in the banner!</i>');

This code uses the JQuery library to find an element in the web page with the ID "image_sample", and then replaces its contents with this hard-coded blob of HTML.

I really appreciate the use of self-closing, XHTML style BR tags, which was a fad between 2000 and 2002, but never truly caught on, and was basically forgotten by the time HTML5 dropped. But this developer insisted that self-closing tags were the "correct" way to write HTML.

Pity they didn't put any thought in the "correct" way to add blank space to page beyond line breaks. Or the correct way to populate the DOM that isn't accessing the inner HTML of an element.

At least this was a former co-worker.

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

As oft stated, the "right" way to validate emails is to do a bare minimum sanity check on format, and then send a verification message to the email address the user supplied; it's the only way to ensure that what they gave you isn't just syntactically valid, but is actually usable.

But even that simple approach leaves places to go wrong. Take a look at this code, from Lana.

public function getEmailValidationErrors($data): array { $errors = []; if (isset($data["email"]) && !empty($data["email"])) { if (!str_contains($data["email"], "@")) { $error["email"] = "FORM.CONTACT_DETAILS.ERRORS.NO_AT"; } if (!str_contains($data["email"], ".")) { $error["email"] = "FORM.CONTACT_DETAILS.ERRORS.NO_DOT"; } if (strrpos($data["email"], "@") > strrpos($data["email"], ".")) { $error["email"] = "FORM.CONTACT_DETAILS.ERRORS.NO_TLD"; } } if (isset($data["email1"]) && !empty($data["email1"])) { if (!str_contains($data["email1"], "@")) { $error["email1"] = "FORM.CONTACT_DETAILS.ERRORS.NO_AT"; } if (!str_contains($data["email1"], ".")) { $error["email1"] = "FORM.CONTACT_DETAILS.ERRORS.NO_DOT"; } if (strrpos($data["email1"], "@") > strrpos($data["email1"], ".")) { $error["email1"] = "FORM.CONTACT_DETAILS.ERRORS.NO_TLD"; } } if (isset($data["email2"]) && !empty($data["email2"])) { if (!str_contains($data["email2"], "@")) { $error["email2"] = "FORM.CONTACT_DETAILS.ERRORS.NO_AT"; } if (!str_contains($data["email2"], ".")) { $error["email2"] = "FORM.CONTACT_DETAILS.ERRORS.NO_DOT"; } if (strrpos($data["email2"], "@") > strrpos($data["email2"], ".")) { $error["email2"] = "FORM.CONTACT_DETAILS.ERRORS.NO_TLD"; } } if (isset($data["email3"]) && !empty($data["email3"])) { if (!str_contains($data["email3"], "@")) { $error["email3"] = "FORM.CONTACT_DETAILS.ERRORS.NO_AT"; } if (!str_contains($data["email3"], ".")) { $error["email3"] = "FORM.CONTACT_DETAILS.ERRORS.NO_DOT"; } if (strrpos($data["email3"], "@") > strrpos($data["email3"], ".")) { $error["email3"] = "FORM.CONTACT_DETAILS.ERRORS.NO_TLD"; } } return $errors; }

Let's start with the obvious problem: repetition. This function doesn't validate simply one email, but four, by copy/pasting the same logic multiple times. Lana didn't supply the repeated blocks, just noted that they existed, so let's not pick on the bad names: "email1", etc.- that's just my placeholder. I assume it's different contact types for a customer, or similar.

Now, the other problems range from trivial to comical. First, the PHP function empty returns true if the variable has a zero/falsy value or is not set, which means it implies an isset, making the first branch redundant. That's trivial.

The way the checks get logged into the $error array, they can overwrite each other, meaning if you forget the "@" and the ".", it'll only complain about the ".", but if you forget the ".", it'll complain about not having a valid TLD (the "NO_DOT" error will never be output). That's silly.

Finally, the $errors array is the return value, but the $error array is where we store our errors, meaning this function doesn't return anything in the first place. And that means that it's a email validation function which doesn't do anything at all, which honestly- probably for the best.

[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.

It's only a day away!

Punctual Robert F. never procrastinates. But I think now would be a good time for a change. He worries that "I better do something quick, before my 31,295 year deadline arrives."

 

Stewart suffers so, saying "Whilst failing to check in for a flight home on the TUI app (one of the largest European travel companies), their Harry Potter invisibility cloak slipped. Perhaps I'll just have to stay on holiday?" You have my permission, just tell the boss I said so.

 

Diligent Dan H. is in no danger of being replaced. Says Dan, "My coworker was having problems getting regular expressions to work in a PowerShell script. She asked Bing's Copilot for help - and was it ever helpful!"

 

PSU alum (I'm guessing) Justin W. was overwhelmed in Happy Valley. "I was just trying to find out when the game started. This is too much date math for my brain to figure out."

 

Finally, bug-loving Pieter caught this classic. "They really started with a blank slate for the newest update. I'm giving them a solid %f for the effort."

 

[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.

Markus does QA, and this means writing automated tests which wrap around the code written by developers. Mostly this is a "black box" situation, where Markus doesn't look at the code, and instead goes by the interface and the requirements. Sometimes, though, he does look at the code, and wishes he hadn't.

Today's snippet comes from a program which is meant to generate PDF files and then, optionally, email them. There are a few methods we're going to look at, because they invested a surprising amount of code into doing this the wrong way.

protected override void Execute() { int sendMail = this.VerifyParameterValue(ParamSendMail); if (sendMail == -1) return; if (sendMail == 1) mail = true; this.TraceOutput(Properties.Resources.textGetCustomerForAccountStatement); IList<CustomerModel> customers = AccountStatement.GetCustomersForAccountStatement(); if (customers.Count == 0) return; StreamWriter streamWriter = null; if (mail) streamWriter = AccountStatement.CreateAccountStatementLogFile(); CreateAccountStatementDocumentEngine engine = new CreateAccountStatementDocumentEngine(); foreach (CustomerModel customer in customers) { this.TraceOutput(Properties.Resources.textCustomerAccountStatementBegin + customer.DisplayName.ToString()); // Generate the PDF, optionally send an email with the document attached engine.Execute(customer, mail); if (mail) { AccountStatement.WriteToLogFile(customer, streamWriter); this.TraceOutput(Properties.Resources.textLogWriting); } } engine.Dispose(); if (streamWriter != null) streamWriter.Close(); }

Now, this might sound unfair, but right off the bat I'm going to complain about separation of concerns. This function both generates output and emails it (optionally), while handling all of the stream management. Honestly, I think if the developer were simply forced to go back and make this a set of small, cohesive methods, most of the WTFs would vanish. But there's more to say here.

Specifically, let's look at the first few lines, where we VerifyParameterValue. Note that this function clearly returns -1 when it fails, which is a very C-programmer-forced-to-do-OO idiom. But let's look at that method.

private int VerifyParameterValue(string name) { string stringValue = this.GetParam(name, string.Empty); bool isValid = this.VerifyByParameterFormat(name, stringValue); if (!isValid) return -1; int value = -1; try { value = Convert.ToInt32(stringValue); } catch (Exception e) { this.TraceOutput(string.Concat("\n\n\n", e.Message, "\n\n\n")); return -1; } return value; }

We'll come back to the VerifyByParameterFormat but otherwise, this is basically a wrapper around Convert.ToInt32, and could easily be replaced with Int32.TryParse.

Bonus points for spamming the log output with loads of newlines.

Okay, but what is the VerifyByParameterFormat doing?

private bool VerifyByParameterFormat(string name, string value) { string parameter = string.Empty; string message = string.Empty; if (value.Length != 1) { parameter = Properties.Resources.textSendMail; message = string.Format(Properties.Resources.textSendMailNotValid, value); this.TraceOutput(string.Concat("\n\n\n", message, "\n\n\n")); return false; } string numbers = "0123456789"; char[] characters = value.ToCharArray(); for (byte i = 0; i < characters.Length; i++) { if (numbers.IndexOf(characters[i]) < 0) { parameter = Properties.Resources.textSendMail; message = Properties.Resources.textSendMailNotValid; this.TraceOutput(string.Concat("\n\n\n", message, "\n\n\n")); return false; } } return true; }

Oh, it just goes character by character to verify whether or not this is made up of only digits. Which, by the way, means the CLI argument needs to be an integer, and only when that integer is 1 do we send emails. It's a boolean, but worse.

Let's assume, however, that passing numbers is required by the specification. Still, Markus has thoughts:

Handling this command line argument might seem obvious enough. I'd probably do something along the lines of "if (arg == "1") { sendMail = true } else if (arg != "0") { tell the user they're an idiot }. Of course, I'm not a professional programmer, so my solution is way too simple as the attached piece of code will show you.

There are better ways to do it, Markus, but as you've shown us, there are definitely worse ways.

.comment { border: none; } [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!

Today, we look at a simple bit of bad code. The badness is not that they're using Oracle, though that's always bad. But it's how they're writing this PL/SQL stored function:

FUNCTION CONVERT_STRING_TO_DATE --Public (p_date_string IN Varchar2, p_date_format IN Varchar2 DEFAULT c_date_format) Return Date AS BEGIN If p_date_string Is Null Then Return Null; Else Return To_Date(p_date_string, p_date_format); End If; END; -- FUNCTION CONVERT_STRING_DATE

This code is a wrapper around the to_date function. The to_date function takes a string and a format and returns that format as a date.

This wrapper adds two things, and the first is a null check. If the input string is null, just return null. Except that's exactly how to_date behaves anyway.

The second is that it sets the default format to c_date_format. This, actually, isn't a terrible thing. If you check the docs on the function, you'll see that if you don't supply a format, it defaults to whatever is set in your internationalization settings, and Oracle recommends that you don't rely on that.

On the flip side, this code is used as part of queries, not on processing input, which means that they're storing dates as strings, and relying on the application layer to send them properly formatted strings. So while their to_date wrapper isn't a terrible thing, storing dates as strings definitely is a terrible thing.

[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!

The end of the quarter was approaching, and dark clouds were gathering in the C-suite. While they were trying to be tight lipped about it, the scuttlebutt was flowing freely. Initech had missed major sales targets, and not just by a few percentage points, but by an order of magnitude.

Heads were going to roll.

Except there was a problem: the master report that had kicked off this tizzy didn't seem to align with the department specific reports. For the C-suite, it was that report that was the document of record; they had been using it for years, and had great confidence in it. But something was wrong.

Enter Jeff. Jeff had been hired to migrate their reports to a new system, and while this particular report had not yet been migrated, Jeff at least had familiarity, and was capable of answering the question: "what was going on?" Were the sales really that far off, and was everyone going to lose their jobs? Or could it possibly be that this ancient and well used report might be wrong?

The core of the query was basically a series of subqueries. Each subquery followed this basic pattern:

SELECT SUM(complex_subquery_A) as subtotal FROM complex_subquery_B

None of this was particularly readable, mind you, and it took some digging just to get the shape of the individual queries understood. But none of the individual queries were the problem; it was the way they got stitched together:

SELECT SUM(subtotal) FROM (SELECT SUM(complex_subquery_A) as subtotal FROM complex_subquery_B UNION SELECT SUM(complex_subquery_C) as subtotal FROM complex_subquery_D UNION SELECT SUM(complex_subquery_E) as subtotal FROM complex_subquery_F);

The full query was filled with a longer chain of unions, but it was easy to understand what went wrong, and demonstrate it to management.

The UNION operator does a set union- which means if there are any duplicate values, only one gets included in the output. So if "Department A" and "Department C" both have $1M in sales for the quarter, the total will just be $1M- not the expected $2M.

The correct version of the query would use UNION ALL, which preserves duplicates.

What stunned Jeff was that this report was old enough to be basically an antique, and this was the kind of business that would burn an entire forest down to find out why a single invoice was off by $0.15. It was sheer luck that this hadn't caused an explosion before- or maybe in the past it had, and someone had just written it off as a "minor glitch"?

Unfortunately for Jeff, because the report was so important it required a huge number of approvals before the "UNION ALL" change could be deployed, which meant he was called upon to manually run a "test" version of the report containing the fix every time a C-suite executive wanted one, until the end of the following quarter, when he could finally integrate the fix.

[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.

There are things which are true. Regular expressions frequently perform badly. They're hard to read. Email addresses are not actually regular languages, and thus can't truly be validated (in all they're many possible forms) by a pure regex.

These are true. It's also true that a simple regex can get you most of the way there.

Lucas found this in their codebase, for validating emails.

function echeck(str) { var at="@"; var dot="."; var lat=str.indexOf(at); var lstr=str.length; var ldot=str.indexOf(dot); if (str.indexOf(at)==-1){ alert("You must include an accurate email address for a response."); return false; } if (str.indexOf(at)==-1 || str.indexOf(at)==0 || str.indexOf(at)==lstr){ alert("You must include an accurate email address for a response."); return false; } if (str.indexOf(dot)==-1 || str.indexOf(dot)==0 || str.indexOf(dot)==lstr){ alert("You must include an accurate email address for a response."); return false; } if (str.indexOf(at,(lat+1))!=-1){ alert("You must include an accurate email address for a response."); return false; } if (str.substring(lat-1,lat)==dot || str.substring(lat+1,lat+2)==dot){ alert("You must include an accurate email address for a response."); return false; } if (str.indexOf(dot,(lat+2))==-1){ alert("You must include an accurate email address for a response."); return false; } if (str.indexOf(" ")!=-1){ alert("You must include an accurate email address for a response."); return false; } return true; }

It checks that the string contains an "@", and the "@" is not at the beginning or end of the string. Then it does the same check for a ".". Then it checks that there isn't a second "@". Then it checks that there are at least two non-"@" characters before the ".". Then it checks that there's at least one "." after the "@". Then it checks that there are no spaces.

Like a regex, I don't think this covers the entire space of valid and invalid email addresses, but that's just because the email address spec is complicated. It likely qualifies as "good enough", on that front. But it's the most awkward way to express that series of tests, especially since they create variables which might be useful, but never use them, thus calling str.indexOf many, many times. The awkwardness becomes more obvious with the way it outputs the same error message in multiple branches. Outputs them using alert I might add, which is the kind of choice that should send someone to the Special Hell™.

[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!

The title of this week's column is making me hungry.

To start off our WTFreitag, Reinier B. complains "I did not specify my gender since it's completely irrelevant when ordering a skateboard for my daughter. That does not mean it is correct to address me as Dear Not specified." I wonder (sincerely) if there is a common German-language personal letter greeting for "Dear somebody of unknown gender". I don't think there is one for English. "To Whom It May Concern" is probably the best we can do.

 

"A coworker shared this phishing email he got," reported Adam R. "Even the scammers have trouble with their email templating variable names too." (It's there at the end).

 

Mathematically-minded Marcel V. thinks these numbers don't figure. "The update process of my Overwatch game left me wondering. I am quite certain my computer does not have 18 Exabytes of harddisk space to reclaim. However, if bytes were meant, then how is it trying to reclaim the last .65 bytes??"

 

Big Spender Jeff W. humblebrags "If you have to ask, you can't afford it."

 

Finishing up for the week, Bruce R. signs off with "This dialog from CNN seems a little overdone."

 

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

Kirill writes:

I've worked in this small company for a year, and on a daily basis I've come across things that make my eyes sink back into their sockets in fear, but mostly I've been too busy fixing them to post anything. It being my last day however, here's a classic

We'll take this one in parts. First, every element of the UI the user can navigate to is marked with an enum, defined thus:

enum UiItem { SectionA, SectionB, SectionC,...SectionG }

These names are not anonymized, so already I hate it. But it's the next enum that starts my skin crawling:

enum SecurityUiItem { SectionA = UiItem.SectionA, SectionB = UiItem.SectionB, ... SectionG = UiItem.SectionG }

A SecurityUiItem is a different type, but the values are identical to UiItem.

These enums are used when trying to evaluate role-based permissions for access, and that code looks like this:

if ((currentAccess.ContainsKey(SecurityUiItem.SectionA) && currentAccess[SecurityUiItem.SectionA] != AccessLevel.NoAccess)) return UiItem.SectionA; else if (!currentAccess.ContainsKey(SecurityUiItem.SectionB) || (currentAccess.ContainsKey(SecurityUiItem.SectionB) && currentAccess[SecurityUiItem.SectionB] != AccessLevel.NoAccess)) return UiItem.SectionB; else if (!currentAccess.ContainsKey(SecurityUiItem.SectionC) || (currentAccess.ContainsKey(SecurityUiItem.SectionC) && currentAccess[SecurityUiItem.SectionC] != AccessLevel.NoAccess)) return UiItem.SectionC; ..... else if (!currentAccess.ContainsKey(SecurityUiItem.SectionG) || (currentAccess.ContainsKey(SecurityUiItem.SectionG) && currentAccess[SecurityUiItem.SectionG] != AccessLevel.NoAccess)) return UiItem.SectionG; else return UiItem.Unknown;

Honestly, I don't hate the idea of having one data type representing the actual UI objects and a separate data type which represents permissions, and having a function which can map between these two things. But this is a perfect example of a good idea executed poorly.

I also have to wonder about the fall-through pattern. If I have access to SectionA, I only seem to get SectionA out of this function. Are these permissions hierarchical? I have no idea, but I suspect there's a WTF underpinning this whole thing.

Congratulations on Kirill's last day.

[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.

The CEO of Delia's company retired. They were an old hand in the industry, the kind of person who worked their way up and had an engineering background, and while the staff loved them, the shareholders were less than pleased, because the company was profitable, but not obscenely so. So the next CEO was a McKinsey-approved MBA who had one mission: cut costs.

Out went the senior devs, and much of the managers. Anyone who was product or customer focused followed quickly behind. What remained were a few managers handpicked by the new CEO and a slew of junior engineers- and Pierre.

Pierre was a contractor who followed the new CEO around from company to company. Pierre was there to ensure that nobody wasted any time on engineering that didn't directly impact features. Tests? Wastes of time. Module boundaries? Just slow you down. Move fast and break things, and don't worry about fixing anything because that's your successors' problem.

So let's take a look at how Pierre wrote code. This block of PHP code was simply copy/pasted everywhere it needed to be used, across multiple applications.

foreach($alphaCheck as $checkField) { if($paramsArray[$checkField['field']]) { if($paramsArray[$checkField['field']]) { $invalidChars = false; $checkValue = trim(strtoupper($paramsArray[$checkField['field']])); $allowedChars = explode('|',"A|B|C|D|E|F|G|H|I|J|K|L|M|N|O|P|Q|R|S|T|U|V|W|X|Y|Z|-| |.|'"); for($i=0; $i<strlen($checkValue); $i++) { if(!in_array($checkValue[$i],$allowedChars)) { $invalidChars = true; } } if($invalidChars) { $errorMsgs[] = $checkField['name'] . ' contains invalid characters'; } } } }

This isn't the worst approach to this problem I've seen in PHP, but the fact that this is just a copy/pasted blob, and worse- the $allowedChars may vary a bit in each place it's copy/pasted is what makes it terrible.

Don't worry. The new CEO only stayed for 18 months, got a huge bonus thanks to all the cost-cutting, and then left, taking Pierre along to the next company.

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

Python's "batteries included" approach means that a lot of common tasks have high-level convenience functions for them. For example, if you want to read all the lines from a file into an array (list, in Python), you could do something like:

with open(filename) as f: lines = f.readlines()

Easy peasy. Of course, because it's so easy, there are other options.

For example, you can just convert the file directly to a list: lines = list(f). Or you can iterate across the file directly, e.g.:

with open(filename) as f: for line in f: # do stuff

Of course, that's fine for plain old text files. But we frequently use text files which are structured in some fashion, like a CSV file. No worries, though, as Python has a csv library built in, which makes it easy to handle these files too; especially useful because "writing a CSV parser yourself" is one of those tasks that sounds easy until you hit the first edge case, and then you realize you've made a terrible mistake.

Now, it's important to note that CSV usually is expressed as a "comma separated values" file, but the initialism is actually "character separated values". And, as Sally's co-worker realized, newlines are characters, and thus every text file is technically a CSV file.

foo = list(csv.reader(someFile, delimiter="\n")) .comment { border: none; } [Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

Most of us, when generating a UUID, will reach for a library to do it. Even a UUIDv4, which is just a random number, presents challenges: doing randomness correctly is hard, and certain bits within the UUID are reserved for metadata about what kind of UUID we're generating.

But Gretchen's co-worker didn't reach for a library. What they did reach for was… regular expressions?

function uuidv4() { return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, function (c) { var r = (Math.random() * 16) | 0, v = c == "x" ? r : (r & 0x3) | 0x8; return v.toString(16); }); }

At a glance, this appears to be a riff on common answers on Stack Overflow. I won't pick on this code for not using crypto.randomUUID, the browser function for doing this, as that function only started showing up in browsers in 2021. But using a format string and filling it with random data instead of generating your 128-bits as a Uint8Buffer is less forgivable.

This solution to generating UUIDs makes a common mistake: confusing the representation of the data with the reality of the data. A UUID is 128-bits of numerical data, with a few bits reserved for identification (annoyingly, how many bits are reserved depends on which format we're talking about). We render it as the dash-separated-hex-string, but it is not a dash-separated-hex-string.

In the end, this code does work. Awkwardly and inefficiently and with a high probability of collisions due to bad randomness, but it works. I just hate it.

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