CodeSOD: Join Us in this Query
Today's anonymous submitter worked for a "large, US-based, e-commerce company." This particular company was, some time back, looking to save money, and like so many companies do, that meant hiring offshore contractors.
Now, I want to stress, there's certainly nothing magical about national borders which turns software engineers into incompetents. The reality is simply that contractors never have their client's best interests at heart; they only want to be good enough to complete their contract. This gets multiplied by the contracting firm's desire to maximize their profits by keeping their contractors as booked as possible. And it gets further multiplied by the remoteness and siloing of the interaction, especially across timezones. Often, the customer sends out requirements, and three months later gets a finished feature, with no more contact than that- and it never goes well.
All that said, let's look at some SQL Server code. It's long, so we'll take it in chunks.
-- =============================================================================== -- Author : Ignacius Ignoramus -- Create date: 04-12-2020 -- Description: SP of Getting Discrepancy of Allocation Reconciliation Snapshot -- ===============================================================================That the comment reinforces that this is an "SP", aka stored procedure, is already not my favorite thing to see. The description is certainly made up of words, and I think I get the gist.
ALTER PROCEDURE [dbo].[Discrepency] ( @startDate DATETIME, @endDate DATETIME ) AS BEGINNothing really to see here; it's easy to see that we're going to run a query for a date range. That's fine and common.
DECLARE @tblReturn TABLE ( intOrderItemId INT )Hmm. T-SQL lets you define table variables, which are exactly what they sound like. It's a local variable in this procedure, that acts like a table. You can insert/update/delete/query it. The vague name is a little sketch, and the fact that it holds only one field also makes me go "hmmm", but this isn't bad.
DECLARE @tblReturn1 TABLE ( intOrderItemId INT )Uh oh.
DECLARE @tblReturn2 TABLE ( intOrderItemId INT )Oh no.
DECLARE @tblReturn3 TABLE ( intOrderItemId INT )Oh no no no.
DECLARE @tblReturn4 TABLE ( intOrderItemId INT )This doesn't bode well.
So they've declared five variables called tblReturn, that all hold the same data structure.
What happens next? This next block is gonna be long.
INSERT INTO @tblReturn --(intOrderItemId) VALUES (@_ordersToBeAllocated) /* OrderItemsPlaced */ select intOrderItemId from CompanyDatabase..Orders o inner join CompanyDatabase..OrderItems oi on oi.intOrderId = o.intOrderId where o.dtmTimeStamp between @startDate and @endDate AND intOrderItemId Not In ( /* _itemsOnBackorder */ select intOrderItemId from CompanyDatabase..OrderItems oi inner join CompanyDatabase..Orders o on o.intOrderId = oi.intOrderId where o.dtmTimeStamp between @startDate and @endDate and oi.strstatus='backordered' ) AND intOrderItemId Not In ( /* _itemsOnHold */ select intOrderItemId from CompanyDatabase..OrderItems oi inner join CompanyDatabase..Orders o on o.intOrderId = oi.intOrderId where o.dtmTimeStamp between @startDate and @endDate and o.strstatus='ONHOLD' and oi.strStatus <> 'BACKORDERED' ) AND intOrderItemId Not In ( /* _itemsOnReview */ select intOrderItemId from CompanyDatabase..OrderItems oi inner join CompanyDatabase..Orders o on o.intOrderId = oi.intOrderId where o.dtmTimeStamp between @startDate and @endDate and o.strstatus='REVIEW' and oi.strStatus <> 'BACKORDERED' ) AND intOrderItemId Not In ( /*_itemsOnPending*/ select intOrderItemId from CompanyDatabase..OrderItems oi inner join CompanyDatabase..Orders o on o.intOrderId = oi.intOrderId where o.dtmTimeStamp between @startDate and @endDate and o.strstatus='PENDING' and oi.strStatus <> 'BACKORDERED' ) AND intOrderItemId Not In ( /*_itemsCancelled */ select intOrderItemId from CompanyDatabase..OrderItems oi inner join CompanyDatabase..Orders o on o.intOrderId = oi.intOrderId where o.dtmTimeStamp between @startDate and @endDate and oi.strstatus='CANCELLED' )We insert into @tblReturn the result of a query, and this query relies heavily on using a big pile of subqueries to decide if a record should be included in the output- but these subqueries all query the same tables as the root query. I'm fairly certain this could be a simple join with a pretty readable where clause, but I'm also not going to sit here and rewrite it right now, we've got a lot more query to look at.
INSERT INTO @tblReturn1 /* _backOrderItemsReleased */ select intOrderItemId from CompanyDatabase..OrderItems oi inner join CompanyDatabase..orders o on o.intorderid = oi.intorderid where oi.intOrderItemid in ( select intRecordID from CompanyDatabase..StatusChangeLog where strRecordType = 'OrderItem' and strOldStatus in ('BACKORDERED') and strNewStatus in ('NEW', 'RECYCLED') and dtmTimeStamp between @startDate and @endDate ) and o.dtmTimeStamp < @startDate UNION ( /*_pendingHoldItemsReleased*/ select intOrderItemId from CompanyDatabase..OrderItems oi inner join CompanyDatabase..orders o on o.intorderid = oi.intorderid where oi.intOrderID in ( select intRecordID from CompanyDatabase..StatusChangeLog where strRecordType = 'Order' and strOldStatus in ('REVIEW', 'ONHOLD', 'PENDING') and strNewStatus in ('NEW', 'PROCESSING') and dtmTimeStamp between @startDate and @endDate ) and o.dtmTimeStamp < @startDate ) UNION /* _reallocationsowingtonostock */ ( select oi.intOrderItemID from CompanyDatabase.dbo.StatusChangeLog inner join CompanyDatabase.dbo.OrderItems oi on oi.intOrderItemID = CompanyDatabase.dbo.StatusChangeLog.intRecordID inner join CompanyDatabase.dbo.Orders o on o.intOrderId = oi.intOrderId where strOldStatus = 'RECYCLED' and strNewStatus = 'ALLOCATED' and CompanyDatabase.dbo.StatusChangeLog.dtmTimestamp > @endDate and strRecordType = 'OrderItem' and intRecordId in ( select intRecordId from CompanyDatabase.dbo.StatusChangeLog where strOldStatus = 'ALLOCATED' and strNewStatus = 'RECYCLED' and strRecordType = 'OrderItem' and CompanyDatabase.dbo.StatusChangeLog.dtmTimestamp between @startDate and @endDate ) )Okay, just some unions with more subquery filtering. More of the same. It's the next one that makes this special.
INSERT INTO @tblReturn2 SELECT intOrderItemId FROM @tblReturn UNION SELECT intOrderItemId FROM @tblReturn1Ah, here's the stuff. This is just bonkers. If the goal is to combine the results of these queries into a single table, you could just insert into one table the whole time.
But we know that there are 5 of these tables, so why are we only going through the first two to combine them at this point?
INSERT INTO @tblReturn3 /* _factoryAllocation*/ select oi.intOrderItemId from CompanyDatabase..Shipments s inner join CompanyDatabase..ShipmentItems si on si.intShipmentID = s.intShipmentID inner join Common.CompanyDatabase.Stores stores on stores.intStoreID = s.intLocationID inner join CompanyDatabase..OrderItems oi on oi.intOrderItemId = si.intOrderItemId inner join CompanyDatabase..Orders o on o.intOrderId = s.intOrderId where s.dtmTimestamp >= @endDate and stores.strLocationType = 'FACTORY' UNION ( /*_storeAllocations*/ select oi.intOrderItemId from CompanyDatabase..Shipments s inner join CompanyDatabase..ShipmentItems si on si.intShipmentID = s.intShipmentID inner join Common.CompanyDatabase.Stores stores on stores.intStoreID = s.intLocationID inner join CompanyDatabase..OrderItems oi on oi.intOrderItemId = si.intOrderItemId inner join CompanyDatabase..Orders o on o.intOrderId = s.intOrderId where s.dtmTimestamp >= @endDate and stores.strLocationType <> 'FACTORY' ) UNION ( /* _ordersWithAllocationProblems */ select oi.intOrderItemId from CompanyDatabase.dbo.StatusChangeLog inner join CompanyDatabase.dbo.OrderItems oi on oi.intOrderItemID = CompanyDatabase.dbo.StatusChangeLog.intRecordID inner join CompanyDatabase.dbo.Orders o on o.intOrderId = oi.intOrderId where strRecordType = 'orderitem' and strNewStatus = 'PROBLEM' and strOldStatus = 'NEW' and CompanyDatabase.dbo.StatusChangeLog.dtmTimestamp > @endDate and o.dtmTimestamp < @endDate )Okay, @tblReturn3 is more of the same. Nothing more to really add.
INSERT INTO @tblReturn4 SELECT intOrderItemId FROM @tblReturn2 WHERE intOrderItemId NOT IN(SELECT intOrderItemId FROM @tblReturn3 )Ooh, but here we see something a bit different- we're taking the set difference between @tblReturn2 and @tblReturn3. This would almost make sense if there weren't already set operations in T-SQL which would handle all of this.
Which brings us, finally, to the last query in the whole thing:
SELECT o.intOrderId ,oi.intOrderItemId ,o.dtmDate ,oi.strDescription ,o.strFirstName + o.strLastName AS 'Name' ,o.strEmail ,o.strBillingCountry ,o.strShippingCountry FROM CompanyDatabase.dbo.OrderItems oi INNER JOIN CompanyDatabase.dbo.Orders o on o.intOrderId = oi.intOrderId WHERE oi.intOrderItemId IN (SELECT intOrderItemId FROM @tblReturn4) ENDAt the end of all this, I've determined a few things.
First, the developer responsible didn't understand table variables. Second,they definitely didn't understand joins. Third, they had no sense of the overall workflow of this query and just sorta fumbled through until they got results that the client said were okay.
And somehow, this pile of trash made it through a code review by internal architects and got deployed to production, where it promptly became the worst performing query in their application. Correction: the worst performing query thus far.
CodeSOD: A Ruby Encrusted Footgun
Many years ago, JP joined a Ruby project. This was in the heyday of Ruby, when every startup on Earth was using it, and if you weren't building your app on Rails, were you even building an app?
Now, Ruby offers a lot of flexibility. One might argue that it offers too much flexibility, especially insofar as it permits "monkey patching": you can always add new methods to an existing class, if you want. Regardless of the technical details, JP and the team saw that massive flexibility and said, "Yes, we should use that. All of it!"
As these stories usually go, that was fine- for awhile. Then one day, a test started failing because a class name wasn't defined. That was already odd, but what was even odder is that when they searched through the code, that class name wasn't actually used anywhere. So yes, there was definitely no class with that name, but also, there was no line of code that was trying to instantiate that class. So where was the problem?
def controller_class(name) "#{settings.app_name.camelize}::Controllers".constantize.const_get("#{name.to_s.camelize}") end def model_class(name) "#{settings.app_name.camelize}".constantize.const_get("#{name.to_s.camelize}") end def resource_class(name) "#{settings.app_name.camelize}Client".constantize.const_get("#{name.to_s.camelize}") endIt happened because they were dynamically constructing the class names from a settings field. And not just in this handful of lines- this pattern occurred all over the codebase. There were other places where it referenced a different settings field, and they just hadn't encountered the bug yet, but knew that it was only a matter of time before changing a settings file was going to break more functionality in the application.
They wisely rewrote these sections to not reference the settings, and dubbed the pattern the "Caramelize Pattern". They added that to their coding standards as a thing to avoid, and learned a valuable lesson about how languages provide footguns.
Since today's April Fool's Day, consider the prank the fact that everyone learned their lesson and corrected their mistakes. I suppose that has to happen at least sometimes.
CodeSOD: Nobody's BFF
Legacy systems are hard to change, and even harder to eliminate. You can't simply do nothing though; as technology and user expectations change, you need to find ways to modernize and adapt the legacy system.
That's what happened to Alicia's team. They had a gigantic, spaghetti-coded, monolithic application that was well past drinking age and had a front-end to match. Someone decided that they couldn't touch the complex business logic, but what they could do was replace the frontend code by creating an adapter service; the front end would call into this adapter, and the adapter would execute the appropriate methods in the backend.
Some clever coder named this "Backend for Frontend" or "BFF".
It was not anyone's BFF. For starters, this system didn't actually allow you to just connect a UI to the backend. No, that'd be too easy. This system was actually a UI generator.
The way this works is that you feed it a schema file, written in JSON. This file specifies what input elements you want, some hints for layout, what validation you want the UI to perform, and even what CSS classes you want. Then you compile this as part of a gigantic .NET application, and deploy it, and then you can see your new UI.
No one likes using it. No one is happy that it exists. Everyone wishes that they could just write frontends like normal people, and not use this awkward schema language.
All that is to say, when Alicia's co-worker stood up shortly before lunch, said, "I'm taking off the rest of the day, BFF has broken me," it wasn't particularly shocking to hear- or even the first time that'd happened.
Alicia, not heeding the warning inherent in that statement, immediately tracked down that dev's last work, and tried to understand what had been so painful.
"minValue": 1900, "maxValue": 99,This, of course, had to be a bug. Didn't it? How could the maxValue be lower than the minValue?
Let's look at the surrounding context.
{ "type": "eventValueBetweenValuesValidator", "eventType": "CalendarYear", "minValue": 1900, "maxValue": 99, "isCalendarBasedMaxValue": true, "message": "CalendarYear must be between {% raw %}{{minValue}}{% endraw %} and {% raw %}{{maxValue}}{% endraw %}." }I think this should make it perfectly clear what's happening. Oh, it doesn't? Look at the isCalendarBasedMaxValue field. It's true. There, that should explain everything. No, it doesn't? You're just more confused?
The isCalendarBasedMaxValue says that the maxValue field should not be treated as a literal value, but instead, is the number of years in the future relative to the current year which are considered valid. This schema definition says "accept all years between 1900 and 2124 (at the time of this writing)." Next year, that top value goes up to 2125. Then 2126. And so on.
As features go, it's not a terrible feature. But the implementation of the feature is incredibly counter-intuitive. At the end of the day, this is just bad naming: (ab)using min/max to do something that isn't really a min/max validation is the big issue here.
Alicia writes:
I couldn't come up with something more counterintuitive if I tried.
Oh, don't sell yourself short, Alicia. I'm sure you could write something far, far worse if you tried. The key thing here is that clearly, nobody tried- they just sorta let things happen and definitely didn't think too hard about it.
[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.Error'd: Here Comes the Sun
We got an unusual rash of submissions at Error'd this week. Here are five reasonably good ones chosen not exactly at random. For those few (everyone) who didn't catch the off-by-one from last week's batch, there's the clue.
"Gotta CAPTCHA 'Em All," puns Alex G. "So do I select them all?" he wondered. I think the correct answer is null.
"What does a null eat?" wondered B.J.H , "and is one null invited or five?". The first question is easily answered. NaaN, of course. Probably garlic. I would expect B.J. to already know the eating habits of a long-standing companion, so I am guessing that the whole family is not meant to tag along. Stick with just the one.
Planespotter Rick R. caught this one at the airport. "Watching my daughter's flight from New York and got surprised by Boeing's new supersonic 737 having already arrived in DFW," he observed. I'm not quite sure what went wrong. It's not the most obvious time zone mistake I can imagine, but I'm pretty sure the cure is the same: all times displayed in any context that is not purely restricted to a single location (and short time frame) should explicitly include the relevant timezone.
Rob H. figures "From my day job's MECM Software Center. It appears that autocorrect has miscalculated, because the internet cannot be calculated." The internet is -1.
Ending this week on a note of hope, global warrior Stewart may have just saved the planet. "Climate change is solved. We just need to replicate the 19 March performance of my new solar panels." Or perhaps I miscalculated.
A Bracing Way to Start the Day
Barry rolled into work at 8:30AM to see the project manager waiting at the door, wringing her hands and sweating. She paced a bit while Barry badged in, and then immediately explained the issue:
Today was a major release of their new features. This wasn't just a mere software change; the new release was tied to major changes to a new product line- actual widgets rolling off an assembly line right now. And those changes didn't work.
"I thought we tested this," Barry said.
"We did! And Stu called in sick today!"
Stu was the senior developer on the project, who had written most of the new code.
"I talked to him for a few minutes, and he's convinced it's a data issue. Something in the metadata or something?"
"I'll take a look," Barry said.
He skipped grabbing a coffee from the carafe and dove straight in.
Prior to the recent project, the code had looked something like this:
if (IsProduct1(_productId)) _programId = 1; elseif (IsProduct2(_productId)) _programId = 2; elseif (IsProduct3(_productId)) _programId = 3;Part of the project, however, was about changing the workflow for "Product 3". So Stu had written this code:
if (IsProduct1(_productId)) _programId = 1; else if (IsProduct2(_productId)) _programId = 2; else if (IsProduct3(_productId)) _programId = 3; DoSomethingProductId3Specific1(); DoSomethingProductId3Specific2(); DoSomethingProductId3Specific3();Since this is C# and not Python, it took Barry all of 5 seconds to spot this and figure out what the problem was and fix it:
if (IsProduct1(_productId)) { _programId = 1; } else if (IsProduct2(_productId)) { _programId = 2; } else if (IsProduct3(_productId)) { _programId = 3; DoSomethingProductId3Specific1(); DoSomethingProductId3Specific2(); DoSomethingProductId3Specific3(); }This brings us to about 8:32. Now, given the problems, Barry wasn't about to just push this change- in addition to running pipeline tests (and writing tests that Stu clearly hadn't), he pinged the head of QA to get a tester on this fix ASAP. Everyone worked quickly, and that meant by 9:30 the fix was considered good and ready to be merged in and pushed to production. Sometime in there, while waiting for a pipeline to complete, Barry managed to grab a cup of coffee to wake himself up.
While Barry was busy with that, Stu had decided that he wasn't feeling that sick after all, and had rolled into the office around 9:00. Which meant that just as Barry was about to push the button to run the release pipeline, an "URGENT" email came in from Stu.
"Hey, everybody, I fixed that bug. Can we get this released ASAP?"
Barry went ahead and released the version that he'd already tested, but out of morbid curiosity, went and checked Stu's fix.
if (IsProduct1(_productId)) _programId = 1; else if (IsProduct2(_productId)) _programId = 2; else if (IsProduct3(_productId)) { _programId = 3; } if (IsProduct3(_productId)) { DoSomethingProductId3Specific1(); DoSomethingProductId3Specific2(); DoSomethingProductId3Specific3(); }At least this version would have worked, though I'm not sure Stu fully understands what "{}"s mean in C#. Or in most programming languages, if we're being honest.
With Barry's work, the launch went off just a few minutes later than the scheduled time. Since the launch was successful, at the next company "all hands", the leadership team made sure to congratulate the people instrumental in making it happen: that is to say, the lead developer of the project, Stu.
[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.Representative Line: Time for Identification
If you need a unique ID, UUIDs provide a variety of options. It's worth noting that variants 1, 2, and 7 all incorporate a timestamp into the UUID. In the case of variant 7, this has the benefit of making the UUID sortable, which can be convenient in many cases (v1/v2 incorporate a MAC address which means that they're sortable if generated with the same NIC).
I bring this up because Dave inherited some code written by a "guru". Said guru was working before UUIDv7 was a standard, but also didn't have any problems that required sortable UUIDs, and thus had no real reason to use timestamp based UUIDs. They just needed some random identifier and, despite using C#, didn't use the UUID functions built in to the framework. No, they instead did this:
string uniqueID = String.Format("{0:d9}", (DateTime.UtcNow.Ticks / 10) % 1000000000);A Tick is 100 nanoseconds. We divide that by ten, mod by a billion, and then call that our unique identifier.
This is, as you might guess, not unique. First there's the possibility of timestamp collisions: generating two of these too close together in time would collide. Second, the math is just complete nonsense. We divide Ticks by ten (converting hundreds of nanoseconds into thousands of nanoseconds), then we mod by a billion. So every thousand seconds we loop and have a risk of collision again?
Maybe, maybe, these are short-lived IDs and a thousand seconds is plenty of time. But even if that's true, none of this is a good way to do that.
I suppose the saving grace is they use UtcNow and not Now, thus avoiding situations where collisions also happen because of time zones?
Representative Line: Tern Down a Date
Today's anonymous submitter has managed to find a way to do date formatting wrong that I don't think I've seen yet. That's always remarkable. Like most such bad code, it checks string lengths and then adds a leading zero, if needed. It's not surprising, but again, it's all in the details:
// convert date string to yyyy/MM/DD return dtmValue.Year + "-" + ((dtmValue.Month.ToString().Length == 1)? ("0" + dtmValue.Month.ToString()): dtmValue.Month.ToString()) + "-" + ((dtmValue.Day.ToString().Length == 1)? ("0" + dtmValue.Day.ToString()): dtmValue.Day.ToString());This is only one line, but it has it all, doesn't it. First, we've got good ol' Hungarian notation, which conveys no useful information here. We've got a comment which tells us the code outputs /es, but the code actually outputs -. We've got ternaries that are definitely not helping readability here, plus repeated calls to ToString() instead of maybe just storing the result in a variable.
And, for the record, dtmValue.ToString("yyyy-MM-dd") would have done the correct thing.
.comment { border: none; }Representative Line: The Rounding Error
At one point, someone noticed that some financial transactions weren't summing up correctly in the C# application Nancy supported. It didn't require Superman or a Peter Gibbons to figure out why: someone was using floating points for handling dollar amounts.
That kicked off a big refactoring project to replace the usage of double types with decimal types. Everything seemed to go well, at least until there was a network hiccup and the application couldn't connect to the database. Let's see if you can figure out what happened:
MessageBox.Show("Please decimal check the connection details. Also check firewall settings (port 1433) and network connectivity.");What a clbuttic mistake.
Error'd: NaN is the Loneliest Number
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.
Over Extended Methods
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.
CodeSOD: Reliability Test
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!
CodeSOD: Spaced Out Prefix
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; }Too Many Red Flags
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, they had all been implemented in a different, weird way. The wheel had been reinvented over and over, each time by someone who'd never even heard of a circle. 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.Error'd: No Time Like the Present
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.
CodeSOD: Don't Date Me
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. According to this code, 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."
CodeSOD: Expressing a Leak
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.
Representative Line: Broken Up With
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.
CodeSOD: Where is the Validation At?
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.
Error'd: Tomorrow
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."
CodeSOD: An Argument With QA
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 ConfidenceYour journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!