Representative Line: Reduced to a Union
The code Clemens M supported worked just fine for ages. And then one day, it broke. It didn't break after a deployment, which implied some other sort of bug. So Clemens dug in, playing the game of "what specific data rows are breaking the UI, and why?"
One of the organizational elements of their system was the idea of "zones". I don't know the specifics of the application as a whole, but we can broadly describe it thus:
The application oversaw the making of widgets. Widgets could be assigned to one or more zones. A finished product requires a set of widgets. Thus, the finished product has a number of zones that's the union of all of the zones of its component widgets.
Which someone decided to handle this way:
zones.reduce((accumulator, currentValue) => accumulator = _.union(currentValue))So, we reduce across zones (which is an array of arrays, where the innermost arrays contain zone names, like zone-0, zone-1). In each step we union it with… nothing. The LoDash union function expects an array of arrays, and returns an array that's the union of all its inputs. This isn't how that function is meant to be used, but the behavior from this incorrect usage was that accumulator would end up holding the last element in zones. Which actually worked until recently, because until recently no one was splitting products across zones. When all the inputs were in the same zone, grabbing the last one was just fine.
The code had been like this for years. It was only just recently, as the company expanded, that it became problematic. The fix, at least, was easy- drop the reduce and just union correctly.
CodeSOD: Functionally, a Date
Dates are messy things, full of complicated edge cases and surprising ways for our assumptions to fail. They lack the pure mathematical beauty of other data types, like integers. But that absence doesn't mean we can't apply the beautiful, concise, and simple tools of functional programming to handling dates.
I mean, you or I could. J Banana's co-worker seems to struggle a bit with it.
/** * compare two dates, rounding them to the day */ private static int compareDates( LocalDateTime date1, LocalDateTime date2 ) { List<BiFunction<LocalDateTime,LocalDateTime,Integer>> criterias = Arrays.asList( (d1,d2) -> d1.getYear() - d2.getYear(), (d1,d2) -> d1.getMonthValue() - d2.getMonthValue(), (d1,d2) -> d1.getDayOfMonth() - d2.getDayOfMonth() ); return criterias.stream() .map( f -> f.apply(date1, date2) ) .filter( r -> r != 0 ) .findFirst() .orElse( 0 ); }This Java code creates a list containing three Java functions. Each function will take two dates and returns an integer. It then streams that list, applying each function in turn to a pair of dates. It then filters through the list of resulting integers for the first non-zero value, and failing that, returns just zero.
Why three functions? Well, because we have to check the year, the month, and the day. Obviously. The goal here is to return a negative value if date1 preceeds date2, zero if they're equal, and positive if date1 is later. And on this metric… it does work. That it works is what makes me hate it, honestly. This not only shouldn't work, but it should make the compiler so angry that the computer gets up and walks away until you've thought about what you've done.
Our submitter replaced all of this with a simple:
return date1.toLocalDate().compareTo( date2.toLocalDate() ); .comment { border; none; }Error'd: Free Birds
"These results are incomprensible," Brian wrote testily. "The developers at SkillCertPro must use math derived from an entirely different universe than ours. I can boast a world record number of answered questions in one hour and fifteen minutes somewhere."
"How I Reached Inbox -1," Maia titled her Tickity Tock. "Apparently I've read my messages so thoroughly that my email client (Mailspring) time traveled into the future and read a message before it was even sent."
... which taught Jason how to use Mailspring to order timely tunes. "Apparently, someone invented a time machine and is able to send us vinyls from the future..."
"Yes, we have no bananas," sang out Peter G. , rapping "... or email addresses or phone numbers, but we're going to block your post just the same (and this is better than the previous result of "Whoops something went wrong", because you'd never be able to tell something had gone wrong without that helpful message)."
Finally, our favorite cardsharp Adam R. might have unsharp eyes but sharp browser skills. "While reading an online bridge magazine, I tried to zoom out a bit but was dismayed to find I couldn't zoom out. Once it zooms in to NaN%, you're stuck there."
CodeSOD: The Getter Setter Getter
Today's Java snippet comes from Capybara James.
The first sign something was wrong was this:
private Map<String, String> getExtractedDataMap(PayloadDto payload) { return setExtractedDataToMap(payload); }Java conventions tell us that a get method retrieves a value, and a set method mutates the value. So a getter that calls a setter is… confusing. But neither of these are truly getters nor setters.
setExtractedDataToMap converts the PayloadDto to a Map<String, String>. getExtractedMap just calls that, which is just one extra layer of indirection that nobody needed, but whatever. At its core, this is just two badly named methods where there should be one.
But that distracts from the true WTF in here. Why on Earth are we converting an actual Java object to a Map<String,String>? That is a definite code smell, a sign that someone isn't entirely comfortable with object-oriented programming. You can't even say, "Well, maybe for serialization to JSON or something?" because Java has serializers that just do this transparently. And that's just the purpose of a DTO in the first place- to be a bucket that holds data for easy serialization.
We're left wondering what the point of all of this code is, and we're not alone. James writes:
I found this gem of a code snippet while trying to understand a workflow for data flow documentation purpose. I was not quite sure what the original developer was trying to achieve and at this point I just gave up
[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: Upsert Yours
Henrik H sends us a short snippet, for a relative value of short.
We've all seen this method before, but this is a particularly good version of it:
public class CustomerController { public void MyAction(Customer customer) { // snip 125 lines if (customer.someProperty) _customerService.UpsertSomething(customer.Id, customer.Code, customer.Name, customer.Address1, customer.Address2, customer.Zip, customer.City, customer.Country, null, null, null, null, null, null, null, null, null, null, null, null, null, null, false, false, null, null, null, null, null, null, null, null, null, null, null, null, false, false, false, false, true, false, null, null, null, false, true, false, true, true, 0, false, false, false, false, customer.TemplateId, false, false, false, false, false, string.Empty, true, false, false, false, false, false, false, false, false, true, false, false, true, false, false, MiscEnum.Standard, false, false, false, true, null, null, null); else _customerService.UpsertSomething(customer.Id, customer.Code, customer.Name, customer.Address1, customer.Address2, customer.Zip, customer.City, customer.Country, null, null, null, null, null, null, null, null, null, null, null, null, null, null, false, false, null, null, null, null, null, null, null, null, null, null, null, null, false, false, false, false, true, false, null, null, null, false, false, false, true, true, 0, false, false, false, false, customer.TemplateId, false, false, false, false, false, string.Empty, true, false, false, false, false, false, false, false, true, true, false, false, true, false, false, MiscEnum.Standard, false, false, false, true, null, null, null); // snip 52 lines } }Welcome to the world's most annoying "spot the difference" puzzle. I've added line breaks (as each UpsertSomething was all on one line in the original) to help you find it. Here's a hint: it's one of the boolean values. I'm sure that narrows it down for you. It means the original developed didn't need the if/else and instead could have simply passed customer.someProperty as a parameter.
Henrick writes:
While on a simple assignment to help a customer migrate from .NET Framework to .NET core, I encountered this code. The 3 lines are unfortunately pretty representative for the codebase
.comment { border: none; }Myopic Focus
Chops was a developer for Initrode. Early on a Monday, they were summoned to their manager Gary's office before the caffeine had even hit their brain.
Gary glowered up from his office chair as Chops entered. This wasn't looking good. "We need to talk about the latest commit for Taskmaster."
Taskmaster was a large application that'd been around for decades, far longer than Chops had been an employee. Thousands of internal and external customers relied upon it. Refinements over time had led to remarkable stability, its typical uptime now measured in years. However, just last week, their local installation had unexpectedly suffered a significant crash. Chops had been assigned to troubleshooting and repair.
"What's wrong?" Chops asked.
"Your latest commit decreased the number of unit tests!" Gary replied as if Chops had slashed the tires on his BMW.
Within Taskmaster, some objects that were periodically generated were given a unique ID from a pool. The pool was of limited size and required scanning to find a spare ID. Each time a value was needed, a search began where the last search ended. IDs returned to the pool as objects were destroyed would only be reused when the search wrapped back around to the start.
Chops had discovered a bug in the wrap-around logic that would inevitably produce a crash if Taskmaster ran long enough. They also found that if the number of objects created exceeded the size of the pool, this would trigger an infinite loop.
Rather than attempt to patch any of this, Chops had nuked the whole thing and replaced it with code that assigned each object a universally unique identifier (UUID) from a trusted library UUID generator within its constructor. Gone was the bad code, along with its associated unit tests.
Knowing they would probably only get in a handful of words, Chops wonderered how on earth to explain all this in a way that would appease their manager. "Well—"
"That number must NEVER go down!" Gary snapped.
"But—"
"This is non-negotiable! Roll it back and come up with something better!"
And so Chops had no choice but to remove their solution, put all the janky code back in place, and patch over it with kludge. Every comment left to future engineers contained a tone of apology.
Taskmaster became less stable. Time and expensive developer hours were wasted. Risk to internal and external customers increased. But Gary could rest assured, knowing that his favored metric never faltered on his watch.
CodeSOD: Pretty Little State Machine
State machines are a powerful way to organize code. They are, after all, one of the fundamental models of computation. That's pretty good. A well designed state machine can make a complicated problem clear, and easy to understand.
Chris, on the other hand, found this one.
static { sM.put(tk(NONE, NONE, invite), sp(PENDING, INVITED)); // t1 sM.put(tk(REJECTED, REJECTED, invite), sp(PENDING, INVITED)); // t2 sM.put(tk(PENDING, IGNORED, invite), sp(PENDING, INVITED)); // t3 sM.put(tk(PENDING, INVITED, cancel), sp(NONE, NONE)); // t4 sM.put(tk(PENDING, IGNORED, cancel), sp(NONE, NONE)); // t5 sM.put(tk(PENDING, BLOCKED, cancel), sp(NONE, BLOCKED)); // t6 sM.put(tk(INVITED, PENDING, accept), sp(ACCEPTED, ACCEPTED)); // t7 sM.put(tk(INVITED, PENDING, reject), sp(REJECTED, REJECTED)); // t8 sM.put(tk(INVITED, PENDING, ignore), sp(IGNORED, PENDING)); // t9 sM.put(tk(INVITED, PENDING, block), sp(BLOCKED, PENDING)); // t10 sM.put(tk(ACCEPTED, ACCEPTED, remove), sp(NONE, NONE)); // t11 sM.put(tk(REJECTED, REJECTED, remove), sp(NONE, NONE)); // t12 sM.put(tk(IGNORED, PENDING, remove), sp(NONE, NONE)); // t13 sM.put(tk(PENDING, IGNORED, remove), sp(NONE, NONE)); // t14 sM.put(tk(BLOCKED, PENDING, remove), sp(NONE, NONE)); // t15 sM.put(tk(PENDING, BLOCKED, remove), sp(NONE, BLOCKED)); // t16 sM.put(tk(NONE, BLOCKED, invite), sp(PENDING, BLOCKED)); // t17 sM.put(tk(IGNORED, PENDING, invite), sp(PENDING, INVITED)); // t19 sM.put(tk(INVITED, PENDING, invite), sp(ACCEPTED, ACCEPTED)); // t20 sM.put(tk(NONE, NONE, remove), sp(NONE, NONE)); // t21 sM.put(tk(NONE, BLOCKED, remove), sp(NONE, BLOCKED)); // t22 sM.put(tk(BLOCKED, NONE, remove), sp(NONE, NONE)); // t23 }Honestly, I only know this is a state machine because Chris told me. I could hazard a guess base on the variable name sM. The comments certainly don't help. Numbering lines isn't exactly what I want comments for. I don't know what tk or sp are actually doing.
So yes, this is an unreadable blob that I don't understand, which is always bad. But do you know what elevates this one step above that? If you note the third parameter to the tk function- invite, cancel, accept, etc? Those are constants. So are INVITED, PENDING, ACCEPTED.
While I am not fond of using the structure of a variable name to denote its role, "caps means const" is a very well accepted standard. A standard that they're using sometimes, but not all the time, and just looking at this makes me grind my teeth.
.comment { border: none; }Error'd: Superfluous U's
In today's Error'd episode, we flirt with European English to acknowledge the GDPR.
Modern Architect jeffphi shared an example of a hot software pattern from the early 21st. "As a bonus, these pickleball events appear to come with pickleball event listeners, too!"
Bob Loblaw highlighted that lawtech is typically SNAFU for reasons too complex to explore in this column, explaining: "It's unclear to me if Firefox 136.0 is later than Firefox undefined. Apparently not. This probably isn't as bad as the fact that the site listed in the logo for this technology organization leads to a misconfigured web server."
"It looks like I'm going to have to stay up all night to get best use of our solar panels," writes Stewart from the land of the midnight sun, which would appear to be... Australia? I guess it makes sense that since Oz has summer during winter, they must have high noon at 7 AM. Perfect sense.
Michael R. delivers from the near future. "Update on my parcel! I was not home and DHL will have dropped it off in 1h with the DHeLorean."
Finally,
Some Guy
wrote in with an ambiguous entry, wondering if it was suitable for inclusion.
"I'm not sure if this is Error'd material, since it is definitely
working as intended." It is indeed working as intended, but it is a matter of principle
that some intentions
are so egregious in and of themselves that we must consider them Error'doneous and
absolutely WTF-worthy. Is this an example? I think not, but let's let youse decide.
Mr. Guy explains:
"They chose a "toggle is active" color
closely resembling the "toggle is inactive" color on this
commonly used component for following cookie laws. Now
that's a dark pattern if I ever saw one." Perhaps this is an
accessibility fail, but the distinction between light and
dark grey is clearly visible to my comparatively unimpaired
colour vision. Which way does Hanlon's Razor cut here?
Coded Smorgasbord: Basically, a Smorgasbord
It's that time to take a look at a few short snippets.
Boolean values can hold true or false. But is that truly self documenting? I think we need clearer variable names for this. Certainly, the snippet Nonymous found thinks so:
boolean isTrue = false;Well, at least I'll know if it's true or not. I'm not sure what "it" is in this scenario, but I'm sure that's the least important part of all of this.
If you've worked in C#, you're aware that it offers both a string type, and a String type- they're the same thing. So Colin's co-worker isn't wrong for writing code this way, but they're also wrong for writing code this way.
writer.WriteLine(string.Empty); writer.WriteLine(String.Empty);Billie sends us this short bit of Java, which ensures that nulls are properly handled:
if (val == null) { return null; } return val;It's very important that, if val is null, we don't just return the contents of val, we should return null instead. Y'know, so no one is surprised by an unexpected null. Wait a second…
Finally, Jon finds this comment in the codebase. The code is elided, but I Jon has helpfully summarized it.
// Basically, … several thousand lines of dense code containing no further commentsHonestly, I'm not sure if that comment is a statement of surrender or just an ironic joke. Either way, I get it.
.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!
CodeSOD: Adding to the Argument
David G saw a pull request with a surprisingly long thread of comments on it. What was weirder was that the associated ticket was all about adding a single parameter to an existing method. What could be generating that much discussion?
How could adding an argument add to an argument?
registerFooWrapper: function(arg1, arg2, arg3, arg4, arg5, arg6, arg7) { bar.when('bar-event', function(context) { context.foo({ arg1: arg1, arg2: arg2, arg3: arg3, arg4: arg4, arg5: arg5, arg6: arg6, arg7: arg7, }); }); }This is the original version of the JavaScript function. The parameter names have been anonymized. That aside, this still isn't very good. Seven parameters is likely too many, and based on what I see in setting the context, there is an object type that holds them all, so maybe we should be passing the object around in the first place? Still, this isn't a WTF by any stretch, and since it's already deployed code, changing the interface significantly is a bad idea- maybe just adding a parameter is the right choice here. So what generated so much discussion?
This revision:
registerFooWrapper: function(arg1, arg2, arg3, arg4, arg5, arg6, arg7, notArg8) { if (notArg8 === true) { bar.when('bar-event', function(context) { context.foo({ arg1: arg1, arg2: arg2, arg3: arg3, arg4: arg4, arg5: arg5, arg6: arg6, arg7: arg7, arg8: !notArg8, }); }); } else { bar.when('bar-event', function(context) { context.foo({ arg1: arg1, arg2: arg2, arg3: arg3, arg4: arg4, arg5: arg5, arg6: arg6, arg7: arg7 }); }); } }Okay, so if notArg8 is true, we pass false to the context. If it's any other value, we don't past arg8 at all. I do not understand what I'm looking at here. If the goal is to ensure that arg8 is either true or not set, there are clearer ways to express that idea. But also, the goal of the ticket was not to do that- it was simply to add another parameter, which means you could drop the condition entirely and just add the parameter. context was already receiving arg8 as undefined, so it could clearly handle an undefined value.
David made some comments on the pull request, but the original developer just ended up going radio silent on it. One of the juniors on David's team approved it, for some reason, but nobody ever actually hit merge. Instead, a different developer simply made a version that took arg8 as a parameter, passed it down to context, and called it a day. It worked, the tests passed, and everyone was happy.
Well, except the original developer, but again, who knows what they were trying to do?
The Modern Job Hunt: Part 1
Ellis knew she needed a walk after she hurried off of Zoom at the end of the meeting to avoid sobbing in front of the group.
She'd just been attending a free online seminar regarding safe job hunting on the Internet. Having been searching since the end of January, Ellis had already picked up plenty of first-hand experience with the modern job market, one rejection at a time. She thought she'd attend the seminar just to see if there were any additional things she wasn't aware of. The seminar had gone well, good information presented in a clear and engaging way. But by the end of it, Ellis was feeling bleak. Goodness gracious, she'd already been slogging through months of this. Hundreds of job applications with nothing to show for it. All of the scams out there, all of the bad actors preying on people desperate for their and their loved ones' survival!
Ellis' childhood had been plagued with anxiety and depression. It was only as an adult that she'd learned any tricks for coping with them. These tricks had helped her avoid spiraling into full-on depression for the past several years. One such trick was to stop and notice whenever those first feelings hit. Recognize them, feel them, and then respond constructively.
First, a walk. Going out where there were trees and sunshine: Ellis considered this "garbage collection" for her brain. So she stepped out the front door and started down a tree-lined path near her house, holding on to that bleak feeling. She was well aware that if she didn't address it, it would take root and grow into hopelessness, self-loathing, fear of the future. It would paralyze her, leave her curled up on the couch doing nothing. And it would all happen without any words issuing from her inner voice. That was the most insidious thing. It happened way down deep in a place where there were no words at all.
Once she returned home, Ellis forced herself to sit down with a notebook and pencil and think very hard about what was bothering her. She wrote down each sentiment:
- This job search is a hopeless, unending slog!
- No one wants to hire me. There must be something wrong with me!
- This is the most brutal job search environment I've ever dealt with. There are new scams every day. Then add AI to every aspect until I want to vomit.
This was the first step of a reframing technique she'd just read about in the book Right Kind of Wrong by Amy Edmonson. With the words out, it was possible to look at each statement and determine whether it was rational or irrational, constructive or harmful. Each statement could be replaced with something better.
Ellis proceeded step by step through the list.
- Yes, this will end. Everything ends.
- There's nothing wrong with me. Most businesses are swamped with applications. There's a good chance mine aren't even being looked at before they're being auto-rejected. Remember the growth mindset you learned from Carol Dweck. Each application and interview is giving me experience and making me a better candidate.
- This job market is a novel context that changes every day. That means failure is not only inevitable, it's the only way forward.
Ellis realized that her job hunt was very much like a search algorithm trying to find a path through a maze. When the algorithm encountered a dead end, did it deserve blame? Was it an occasion for shame, embarrassment, and despair? Of course not. Simply backtrack and keep going with the knowledge gained.
Yes, there was truth to the fact that this was the toughest job market Ellis had ever experienced. Therefore, taking a note from Viktor Frankl, she spent a moment reimagining the struggle in a way that made it meaningful to her. Ellis began viewing her job hunt in this dangerous market, her gradual accumulation of survival information, as an act of resistance against it. She now hoped to write all about her experience once she was on the other side, in case her advice might help even one other person in her situation save time and frustration.
While unemployed, she also had the opportunity to employ the search algorithm against entirely new mazes. Could Ellis expand her freelance writing into a sustainable gig, for instance? That would mean exploring all the different ways to be a freelance writer, something Ellis was now curious and excited to explore.
Best of…: Classic WTF: We Are Not Meatbots!
Sales, as everyone knows, is the mortal enemy of Development.
Their goals are opposite, their people are opposite, their tactics are opposite. Even their credos - developers "Make a good product" but sales will "Do anything to get that money" - are at complete odds.
The company Jordan worked for made a pseudo-enterprise product responsible for everything e-commerce: contacts, inventory, website, shipping, payment...everything. His responsibility included the inventory package, overseeing the development team, designing APIs, integration testing, and coordination with the DBAs and sysadmins...you know, everything. One of his team members implemented a website CMS into the product, letting the website design team ignore the content and focus on making it look good.
Care to guess who was responsible for the site content? If you guessed the VP of Sales, congratulations! You win a noprize.
A couple months passed by without incident. Everything's peachy in fact...that is, until one fateful day when the forty-person stock-and-shipping department are clustered in the parking lot when Jordan shows up.
Jordan parked, crossed the asphalt, and asked one of the less threatening looking warehouse guys, "What's the problem?"
The reply was swift as the entire group unanimously shouted "YOUR F***ING WEBSITE!" Another worker added, "You guys in EYE TEE are so far removed from real life out here. We do REAL WORK, what you guys do from behind your desks?"
Jordan was dumbfounded. What brought this on? For a moment he considered defending his and his team's honor but decided it wouldn't accomplish much besides get his face rearranged and instead replied with a meek "Sure, just let me check into this..." before quickly diving into the nearest entry door.
It didn't take much long after for Jordan to ascertain that the issue wasn't that the website was down, but that the content of one page in particular , the "About Us" page, had upset the hardworking staff who accomplished what the company actually promised: stock and ship the products that they sold on their clients' websites.
After an hour of mediation, it was discovered that the VP of Sales, in a strikingly-insensitive-even-for-him moment, had referred to the warehouse staff as "meatbots." The lively folk who staffed the shipping and stocking departments naturally felt disrespected by being reduced to some stupid sci-fi cloning trope nomenclature. The VP's excuse was simply that he had drunk a couple of beers while he wrote the page text for the website. Oops!
Remarkably, the company (which Jordan left some time later for unrelated reasons) eventually caught up to the backlog of orders to go out. It took a complete warehouse staff replacement, but they did catch up. Naturally, the VP of Sales is still there, with an even more impressive title.
photo credit: RTD Photography via photopin cc
Error'd: Scamproof
Gordon S. is smarter than the machines. "I can only presume the "Fix Now with AI" button adds some mistakes in order to fix the lack of needed fixes."
"Sorry, repost with the link https://www.daybreaker.com/alive/," wrote Michael R.
And yet again from Michael R., following up with a package mistracker. "Poor DHL driver. I hope he will get a break within those 2 days. And why does the van look like he's driving away from me."
Morgan
airs some dirty laundry.
"After navigating this washing machine app on holiday and validating
my credit card against another app I am greeted by this less than
helpful message each time. So is OK okay? Or is the Error in error?
Washing machine worked though."
And finally, scamproof Stuart wondered "Maybe the filter saw the word "scam" and immediately filed it into the scam bucket. All scams include the word "scam" in them, right?"
Representative Line: Springs are Optional
Optional types are an attempt to patch the "billion dollar mistake". When you don't know if you have a value or not, you wrap it in an Optional, which ensures that there is a value (the Optional itself), thus avoiding null reference exceptions. Then you can query the Optional to see if there is a real value or not.
This is all fine and good, and can cut down on some bugs. Good implementations are loaded with convenience methods which make it easy to work on the optionals.
But then, you get code like Burgers found. Which just leaves us scratching our heads:
private static final Optional<Boolean> TRUE = Optional.of(Boolean.TRUE); private static final Optional<Boolean> FALSE = Optional.of(Boolean.FALSE);Look, any time you're making constants for TRUE or FALSE, something has gone wrong, and yes, I'm including pre-1999 versions of C in this. It's especially telling when you do it in a language that already has such constants, though- at its core- these lines are saying TRUE = TRUE. Yes, we're wrapping the whole thing in an Optional here, which potentially is useful, but if it is useful, something else has gone wrong.
Burgers works for a large insurance company, and writes this about the code:
I was trying to track down a certain piece of code in a Spring web API application when I noticed something curious. It looked like there was a chunk of code implementing an application-specific request filter in business logic, totally ignoring the filter functions offered by the framework itself and while it was not related to the task I was working on, I followed the filter apply call to its declaration. While I cannot supply the entire custom request filter implementation, take these two static declarations as a demonstration of how awful the rest of the class is.
Ah, of course- deep down, someone saw a perfectly functional wheel and said, "I could make one of those myself!" and these lines are representative of the result.
CodeSOD: The HTML Print Value
Matt was handed a pile of VB .Net code, and told, "This is yours now. I'm sorry."
As often happens, previous company leadership said, "Why should I pay top dollar for experienced software engineers when I can hire three kids out of college for the same price?" The experiment ended poorly, and the result was a pile of bad VB code, which Matt now owned.
Here's a little taste:
// SET IN SESSION AND REDIRECT TO PRINT PAGE Session["PrintValue"] = GenerateHTMLOfItem(); Response.Redirect("PrintItem.aspx", true);The function name here is accurate. GenerateHTMLOfItem takes an item ID, generates the HTML output we want to use to render the item, and stores it in a session variable. It then forces the browser to redirect to a different page, where that HTML can then be output.
You may note, of course, that GenerateHTMLOfItem doesn't actually take parameters. That's because the item ID got stored in the session variable elsewhere.
Of course, it's the redirect that gets all the attention here. This is a client side redirect, so we generate all the HTML, shove it into a session object, and then send a message to the web browser: "Go look over here". The browser sends a fresh HTTP request for the new page, at which point we render it for them.
The Microsoft documentation also has this to add about the use of Response.Redirect(String, Boolean), as well:
Calling Redirect(String) is equivalent to calling Redirect(String, Boolean) with the second parameter set to true. Redirect calls End which throws a ThreadAbortException exception upon completion. This exception has a detrimental effect on Web application performance. Therefore, we recommend that instead of this overload you use the HttpResponse.Redirect(String, Boolean) overload and pass false for the endResponse parameter, and then call the CompleteRequest method. For more information, see the End method.
I love it when I see the developers do a bonus wrong.
Matt had enough fires to put out that fixing this particular disaster wasn't highest on his priority list. For the time being, he could only add this comment:
// SET IN SESSION AND REDIRECT TO PRINT PAGE // FOR THE LOVE OF GOD, WHY?!? Session["PrintValue"] = GenerateHTMLOfItem(); Response.Redirect("PrintItem.aspx", true);Representative Line: Not What They Meant By Watching "AndOr"
Today's awfulness comes from Tim H, and while it's technically more than one line, it's so representative of the code, and so short that I'm going to call this a representative line. Before we get to the code, we need to talk a little history.
Tim's project is roughly three decades old. It's a C++ tool used for a variety of research projects, and this means that 90% of the people who have worked on it are PhD candidates in computer science programs. We all know the rule of CompSci PhDs and programming: they're terrible at it. It's like the old joke about the farmer who, when unable to find an engineer to build him a cow conveyer, asked a physicist. After months of work, the physicist introduced the result: "First, we assume a perfectly spherical cow in a vacuum…"
Now, this particularly function has been anonymized, but it's easy to understand what the intent was:
bool isFooOrBar() { return isFoo() && isBar(); }The obvious problem here is the mismatch between the function name and the actual function behavior- it promises an or operation, but does an and, which the astute reader may note are different things.
I think this offers another problem, though. Even if the function name were correct, given the brevity of the body, I'd argue that it actually makes the code less clear. Maybe it's just me, but isFoo() && isBar() is more clear in its intent than isFooAndBar(). There's a cognitive overhead to adding more symbols that would make me reluctant to add such a function.
There may be an argument about code-reuse, but it's worth noting- this function is only ever called in one place.
This particular function is not itself, all that new. Tim writes:
This was committed as new code in 2010 (i.e., not a refactor). I'm not sure if the author changed their mind in the middle of writing the function or just forgot which buttons on the keyboard to press.
More likely, Tim, is that they initially wrote it as an "or" operation and then discovered that they were wrong and it needed to be an "and". Despite the fact that the function was only called in one place, they opted to change the body without changing the name, because they didn't want to "track down all the places it's used". Besides, isn't the point of a function to encapsulate the behavior?
The C-Level Ticket
Everyone's got workplace woes. The clueless manager; the disruptive coworker; the cube walls that loom ever higher as the years pass, trapping whatever's left of your soul.
But sometimes, Satan really leaves his mark on a joint. I worked Tech Support there. This is my story. Who am I? Just call me Anonymous.
It starts at the top. A call came in from Lawrence Gibbs, the CEO himself, telling us that a conference room printer was, quote, "leaking." He didn't explain it, he just hung up. The boss ordered me out immediately, told me to step on it. I ignored the elevator, racing up the staircase floor after floor until I reached the dizzying summit of C-Town.
There's less oxygen up there, I'm sure of it. My lungs ached and my head spun as I struggled to catch my breath. The fancy tile and high ceilings made a workaday schmuck like me feel daunted, unwelcome. All the same, I gathered myself and pushed on, if only to learn what on earth "leaking" meant in relation to a printer.
I followed the signs on the wall to the specified conference room. In there, the thermostat had been kicked down into the negatives. The cold cut through every layer of mandated business attire, straight to bone. The scene was thick with milling bystanders who hugged themselves and traded the occasional nervous glance. Gibbs was nowhere to be found.
Remembering my duty, I summoned my nerve. "Tech Support. Where's the printer?" I asked.
Several pointing fingers showed me the way. The large printer/scanner was situated against the far wall, flanking an even more enormous conference table. Upon rounding the table, I was greeted with a grim sight: dozens of sheets of paper strewn about the floor like blood spatter. Everyone was keeping their distance; no one paid me any mind as I knelt to gather the pages. There were 30 in all. Each one was blank on one side, and sported some kind of large, blotchy ring on the other. Lord knew I drank enough java to recognize a coffee mug stain when I saw one, but these weren't actual stains. They were printouts of stains.
The printer was plugged in. No sign of foul play. As I knelt there, unseen and unheeded, I clutched the ruined papers to my chest. Someone had wasted a tree and a good bit of toner, and for what? How'd it go down? Surely Gibbs knew more than he'd let on. The thought of seeking him out, demanding answers, set my heart to pounding. It was no good, I knew. He'd play coy all day and hand me my pink slip if I pushed too hard. As much as I wanted the truth, I had a stack of unpaid bills at home almost as thick as the one in my arms. I had to come up with something else.
There had to be witnesses among the bystanders. I stood up and glanced among them, seeking out any who would return eye contact. There: a woman who looked every bit as polished as everyone else. But for once, I got the feeling that what lay beneath the facade wasn't rotten.
With my eyes, I pleaded for answers.
Not here, her gaze pleaded back.
I was getting somewhere, I just had to arrange for some privacy. I hurried around the table again and weaved through bystanders toward the exit, hoping to beat it out of that icebox unnoticed. When I reached the threshold, I spotted Gibbs charging up the corridor, smoldering with entitlement. "Where the hell is Tech Support?!"
I froze a good distance away from the oncoming executive, whose voice I recognized from a thousand corporate presentations. Instead of putting me to sleep this time, it jolted down my spine like lightning. I had to think fast, or I was gonna lose my lead, if not my life.
"I'm right here, sir!" I said. "Be right back! I, uh, just need to find a folder for these papers."
"I've got one in my office."
A woman's voice issued calmly only a few feet behind me. I spun around, and it was her, all right, her demeanor as cool as our surroundings. She nodded my way. "Follow me."
My spirits soared. At that moment, I would've followed her into hell. Turning around, I had the pleasure of seeing Gibbs stop short with a glare of contempt. Then he waved us out of his sight.
Once we were out in the corridor, she took the lead, guiding me through the halls as I marveled at my luck. Eventually, she used her key card on one of the massive oak doors, and in we went.
You could've fit my entire apartment into that office. The place was spotless. Mini-fridge, espresso machine, even couches: none of it looked used. There were a couple of cardboard boxes piled up near her desk, which sat in front of a massive floor-to-ceiling window admitting ample sunlight.
She motioned toward one of the couches, inviting me to sit. I shook my head in reply. I was dying for a cigarette by that point, but I didn't dare light up within this sanctuary. Not sure what to expect next, I played it cautious, hovering close to the exit. "Thanks for the help back there, ma'am."
"Don't mention it." She walked back to her desk, opened up a drawer, and pulled out a brand-new manila folder. Then she returned to conversational distance and proffered it my way. "You're from Tech Support?"
There was pure curiosity in her voice, no disparagement, which was encouraging. I accepted the folder and stuffed the ruined pages inside. "That's right, ma'am."
She shook her head. "Please call me Leila. I started a few weeks ago. I'm the new head of HR."
Human Resources. That acronym, which usually put me on edge, somehow failed to raise my hackles. I'd have to keep vigilant, of course, but so far she seemed surprisingly OK. "Welcome aboard, Leila. I wish we were meeting in better circumstances." Duty beckoned. I hefted the folder. "Printers don't just leak."
"No." Leila glanced askance, grave.
"Tell me what you saw."
"Well ..." She shrugged helplessly. "Whenever Mr. Gibbs gets excited during a meeting, he tends to lean against the printer and rest his coffee mug on top of it. Today, he must've hit the Scan button with his elbow. I saw the scanner go off. It was so bright ..." She trailed off with a pained glance downward.
"I know this is hard," I told her when the silence stretched too long. "Please, continue."
Leila summoned her mettle. "After he leaned on the controls, those pages spilled out of the printer. And then ... then somehow, I have no idea, I swear! Somehow, all those pages were also emailed to me, Mr. Gibbs' assistant, and the entire board of directors!"
The shock hit me first. My eyes went wide and my jaw fell. But then I reminded myself, I'd seen just as crazy and worse as the result of a cat jumping on a keyboard. A feline doesn't know any better. A top-level executive, on the other hand, should know better.
"Sounds to me like the printer's just fine," I spoke with conviction. "What we have here is a CEO who thinks it's OK to treat an expensive piece of office equipment like his own personal fainting couch."
"It's terrible!" Leila's gaze burned with purpose. "I promise, I'll do everything I possibly can to make sure something like this never happens again!"
I smiled a gallows smile. "Not sure what anyone can do to fix this joint, but the offer's appreciated. Thanks again for your help."
Now that I'd seen this glimpse of better things, I selfishly wanted to linger. But it was high time I got outta there. I didn't wanna make her late for some meeting or waste her time. I backed up toward the door on feet that were reluctant to move.
Leila watched me with a look of concern. "Mr. Gibbs was the one who called Tech Support. I can't close your ticket for you; you'll have to get him to do it. What are you going to do?"
She cared. That made leaving even harder. "I dunno yet. I'll think of something."
I turned around, opened the massive door, and put myself on the other side of it in a hurry, using wall signs to backtrack to the conference room. Would our paths ever cross again? Unlikely. Someone like her was sure to get fired, or quit out of frustration, or get corrupted over time.
It was too painful to think about, so I forced myself to focus on the folder of wasted pages in my arms instead. It felt like a mile-long rap sheet. I was dealing with an alleged leader who went so far as to blame the material world around him rather than accept personal responsibility. I'd have to appeal to one or more of the things he actually cared about: himself, his bottom line, his sense of power.
By the time I returned to the conference room to face the CEO, I knew what to tell him. "You're right, sir, there's something very wrong with this printer. We're gonna take it out here and give it a thorough work-up."
That was how I was able to get the printer out of that conference room for good. Once it underwent "inspection" and "testing," it received a new home in a previously unused closet. Whenever Gibbs got to jawing in future meetings, all he could do was lean against the wall. Ticket closed.
Gibbs remained at the top, doing accursed things that trickled down to the roots of his accursed company. But at least from then on, every onboarding slideshow included a photo of one of the coffee ring printouts, with the title Respect the Equipment.
Thanks, Leila. I can live with that.
[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: 8 Days a Week
"What word can spell with the letters housucops?" asks Mark R. "Sometimes AI hallucinations can be hard to find. Other times, they just kind of stand out..."
"Do I need more disks?" wonders Gordon "I'm replacing a machine which has only 2 GB of HDD. New one has 2 TB, but that may not be enough. Unless Thunar is lying." It's being replaced by an LLM.
"Greenmobility UX is a nightmare" complains an anonymous reader. "Just like last week's submission, do you want to cancel? Cancel or Leave?" This is not quite as bad as last week's.
Cinephile jeffphi rated this film two thumbs down. "This was a very boring preview, cannot recommend."
Malingering Manuel H. muses "Who doesn't like long weekends? Sometimes, one Sunday per week is just not enough, so just put a second one right after the first." I don't want to wait until Oktober for a second Sunday; hope we get one søøn.
A Countable
Once upon a time, when the Web was young, if you wanted to be a cool kid, you absolutely needed two things on your website: a guestbook for people to sign, and a hit counter showing how many people had visited your Geocities page hosting your Star Trek fan fiction.
These days, we don't see them as often, but companies still like to track the information, especially when it comes to counting downloads. So when Justin started on a new team and saw a download count in their analytics, he didn't think much of it at all. Nor did he think much about it when he saw the download count displayed on the download page.
Another thing that Justin didn't think much about was big piles of commits getting merged in overnight, at least not at first. But each morning, Justin needed to pull in a long litany of changes from a user named "MrStinky". For the first few weeks, Justin was too preoccupied with getting his feet under him, so he didn't think about it too much.
But eventually, he couldn't ignore what he saw in the git logs.
docs: update download count to 51741 docs: update download count to 51740 docs: update download count to 51738And each commit was exactly what the name implied, a diff like:
- 51740 + 51741Each time a user clicked the download link, a ping was sent to their analytics system. Throughout the day, the bot "MrStinky" would query the analytics tool, and create new commits that updated the counter. Overnight, it would bundle those commits into a merge request, approve the request, merge the changes, and then redeploy what was at the tip of main.
"But, WHY?" Justin asked his peers.
One of them just shrugged. "It seemed like the easiest and fastest way at the time?"
"I wanted to wire Mr Stinky up to our content management system's database, but just never got around to it. And this works fine," said another.
Much like the rest of the team, Justin found that there were bigger issues to tackle.
[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!
CodeSOD: Copy of a Copy of a
Jessica recently started at a company still using Windows Forms.
Well, that was a short article. Oh, you want more WTF than that? Sure, we can do that.
As you might imagine, a company that's still using Windows Forms isn't going to upgrade any time soon; they've been using an API that's been in maintenance mode for a decade, clearly they're happy with it.
But they're not too happy- Jessica was asked to track down a badly performing report. This of course meant wading through a thicket of spaghetti code, pointless singletons, and the general sloppiness that is the code base. Some of the code was written using Entity Framework for database access, much of it is not.
While it wasn't the report that Jessica was sent to debug, this method caught her eye:
private Dictionary<long, decimal> GetReportDiscounts(ReportCriteria criteria) { Dictionary<long, decimal> rows = new Dictionary<long, decimal>(); string query = @"select ii.IID, SUM(CASE WHEN ii.AdjustedTotal IS NULL THEN (ii.UnitPrice * ii.Units) ELSE ii.AdjustedTotal END) as 'Costs' from ii where ItemType = 3 group by ii.IID "; string connectionString = string.Empty; using (DataContext db = DataContextFactory.GetInstance<DataContext>()) { connectionString = db.Database.Connection.ConnectionString; } using (SqlConnection connection = new SqlConnection(connectionString)) { using (SqlCommand command = new SqlCommand(query, connection)) { command.Parameters.AddWithValue("@DateStart", criteria.Period.Value.Min.Value.Date); command.Parameters.AddWithValue("@DateEnd", criteria.Period.Value.Max.Value.Date.AddDays(1)); command.Connection.Open(); using (SqlDataReader reader = command.ExecuteReader()) { while (reader.Read()) { decimal discount = (decimal)reader["Costs"]; long IID = (long)reader["IID"]; if (rows.ContainsKey(IID)) { rows[IID] += discount; } else { rows.Add(IID, discount); } } } } } return rows; }This code constructs a query, opens a connection, runs the query, and iterates across the results, building a dictionary as its result set. The first thing which leaps out is that, in code, they're doing a summary (iterating across the results and grouping by IID), which is also what they did in the query.
It's also notable that the table they're querying is called ii, which is not a result of anonymization, and actually what they called it. Then there's the fact that they set parameters on the query, for DateStart and DateEnd, but the query doesn't use those. And then there's that magic number 3 in the query, which is its own set of questions.
Then, right beneath that method was one called GetReportTotals. I won't share it, because it's identical to what's above, with one difference:
string query = @" select ii.IID, SUM(CASE WHEN ii.AdjustedTotal IS NULL THEN (ii.UnitPrice * ii.Units) ELSE ii.AdjustedTotal END) as 'Costs' from ii where itemtype = 0 group by iid ";The magic number is now zero.
So, clearly we're in the world of copy/paste programming, but this raises the question: which came first, the 0 or the 3? The answer is neither. GetCancelledInvoices came first.
private List<ReportDataRow> GetCancelledInvoices(ReportCriteria criteria, Dictionary<long, string> dictOfInfo) { List<ReportDataRow> rows = new List<ReportDataRow>(); string fCriteriaName = "All"; string query = @"select A long query that could easily be done in EF, or at worst a stored procedure or view. Does actually use the associated parameters"; string connectionString = string.Empty; using (DataContext db = DataContextFactory.GetInstance<DataContext>()) { connectionString = db.Database.Connection.ConnectionString; } using (SqlConnection connection = new SqlConnection(connectionString)) { using (SqlCommand command = new SqlCommand(query, connection)) { command.Parameters.AddWithValue("@DateStart", criteria.Period.Value.Min.Value.Date); command.Parameters.AddWithValue("@DateEnd", criteria.Period.Value.Max.Value.Date.AddDays(1)); command.Connection.Open(); using (SqlDataReader reader = command.ExecuteReader()) { while (reader.Read()) { long ID = (long)reader["ID"]; decimal costs = (decimal)reader["Costs"]; string mNumber = (string)reader["MNumber"]; string mName = (string)reader["MName"]; DateTime idate = (DateTime)reader["IDate"]; DateTime lastUpdatedOn = (DateTime)reader["LastUpdatedOn"]; string iNumber = reader["INumber"] is DBNull ? string.Empty : (string)reader["INumber"]; long fId = (long)reader["FID"]; string empName = (string)reader["EmpName"]; string empNumber = reader["EmpNumber"] is DBNull ? string.Empty : (string)reader["empNumber"]; long mId = (long)reader["MID"]; string cName = dictOfInfo[matterId]; if (criteria.EmployeeID.HasValue && fId != criteria.EmployeeID.Value) { continue; } rows.Add(new ReportDataRow() { CName = cName, IID = ID, Costs = costs * -1, //Cancelled i - minus PC TimedValue = 0, MNumber = mNumber, MName = mName, BillDate = lastUpdatedOn, BillNumber = iNumber + "A", FID = fId, EmployeeName = empName, EmployeeNumber = empNumber }); } } } } return rows; }This is the original version of the method. We can infer this because it actually uses the parameters of DateStart and DateEnd. Everything else just copy/pasted this method and stripped out bits until it worked. There are more children of this method, each an ugly baby of its own, but all alike in their ugliness.
It's also worth noting, the original version is doing filtering after getting data from the database, instead of putting that criteria in the WHERE clause.
As for Jessica's poor performing report, it wasn't one of these methods. It was, however, another variation on "run a query, then filter, sort, and summarize in C#". By simply rewriting it as a SQL query in a stored procedure that leveraged indexes, performance improved significantly.