The Daily WTF
Error'd: What Goes Around
No obvious pattern fell out of last week's submissions for Error'd, but I did especially like Caleb Su's example.
Michael R. , apparently still job hunting, reports "I have signed up to outlier.ai to make some $$$ on the side. No instructions necessary."
Peter G. repeats a recurring theme of lost packages, saying "(Insert obligatory snark about Americans and geography. No, New Zealand isn't located in Washington DC)." A very odd coincidence, since neither the lat/long nor the zip code are particularly interesting.
"The Past Is Mutable," declares Caleb Su , explaining "In the race to compete with Gmail feature scheduling emails to send in the *future*, Outlook now lets you send emails in the past! Clearly, someone at Microsoft deserves a Nobel Prize for defying the basic laws of unidirectional time." That's thinking different.
Explorer xOneca explains this snapshot: "Was going to watch a Youtube video in DuckDuckGo, and while diagnosing why it wasn't playing I found this. It seems that youtube-nocookie.com actually *sets* cookies..?"
Morgan either found or made a funny. But it is a funny. "Now when I think about it I do like Option 3 more…" I rate this question a 👎
[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.
CodeSOD: Join Our Naming
As a general rule, if you're using an RDBMS and can solve your problem using SQL, you should solve your problem using SQL. It's how we avoid doing joins or sorts in our application code, which is always a good thing.
But this is a general rule. And Jasmine sends us one where solving the problem as a query was a bad idea.
ALTER FUNCTION [dbo].[GetName](@EntityID int) RETURNS varchar(200) AS BEGIN declare @Name varchar(200) select @Name = case E.EntityType when 'Application' then A.ApplicationName when 'Automation' then 'Automated Process' when 'Group' then G.GroupName when 'Organization' then O.OrgName when 'Person' then P.FirstName + ' ' + P.LastName when 'Resource' then R.ResourceName when 'Batch' then B.BatchComment end from Entities E left join AP_Applications A on E.EntityID = A.EntityID left join CN_Groups G on E.EntityID = G.EntityID left join CN_Organizations O on E.EntityID = O.EntityID left join CN_People P on E.EntityID = P.EntityID left join Resources R on E.EntityID = R.EntityID left join AR_PaymentBatches B on E.EntityID = B.EntityID where E.EntityID = @EntityID return @Name ENDThe purpose of this function is to look up the name of an entity. Depending on the kind of entity we're talking about, we have to pull that name from a different table. This is a very common pattern in database normalization- a database equivalent of inheritance. All the common fields to all entities get stored in an Entities table, while specific classes of entity (like "Applications") get their own table which joins back to the Entities table.
On the surface, this code doesn't even really look like a WTF. By the book, this is really how you'd write this kind of function- if we were going by the book.
But the problem was that these tables were frequently very large, and even with indexes on the EntityID fields, it simply performed horribly. And since "showing the name of the thing you're looking at" was a common query, that performance hit added up.
The fix was easy- write out seven unique functions- one for each entity type- and then re-write this function to use an IF statement to decide which one to execute. The code was simpler to understand and read, and performed much faster.
In the end, perhaps not really a WTF, or perhaps the root WTF is some of the architectural decisions which allow this to exist (why a function for getting the name, and the name alone, which means we execute this query independently and not part of a more meaningful join?). But I think it's an interesting example of how "this is the right way to do it" can lead to some unusual outcomes.
[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!CodeSOD: Querieous Strings
When processing HTTP requests, you frequently need to check the parameters which were sent along with that request. Those parameters are generally passed as stringly-typed key/value pairs. None of this is news to anyone.
What is news, however, is how Brodey's co-worker indexed the key/value pairs.
For i As Integer = 0 To (Request.Params().Count - 1) If (parameters.GetKey(i).ToString() <> "Lang") Then If (parameters.GetKey(i).Equals("ID")) OrElse (parameters.GetKey(i).Equals("new")) OrElse _ (parameters.GetKey(i).Equals("open")) OrElse (parameters.GetKey(i).Equals("FID")) _ OrElse (parameters.GetKey(i).Equals("enabled")) OrElse (parameters.GetKey(i).Equals("my")) OrElse _ (parameters.GetKey(i).Equals("msgType")) OrElse (parameters.GetKey(i).Equals("Type")) _ OrElse (parameters.GetKey(i).Equals("EID")) OrElse (parameters.GetKey(i).Equals("Title")) OrElse _ (parameters.GetKey(i).Equals("ERROR")) Then URLParams &= "&" & parameters.GetKey(i).ToString() URLParams &= "=" & parameters(i).ToString() End If End If NextThe goal of this code is to take a certain set of keys and construct a URLParams string which represents those key/values as an HTTP query string. The first thing to get out of the way: .NET has a QueryString type that handles the construction of the query string for you (including escaping), so that you don't need to do any string concatenation.
But the real WTF is everything surrounding that. We opt to iterate across every key- not just the ones we care about- and use the GetKey(i) function to check each individual key in an extensive chain of OrElse statements.
The obvious and simpler approach would have been to iterate across an array of the keys I care about- ID, new, FID, enabled, my, msgType, Type, EID, Title, ERROR- and simply check if they were in the Request.
I suppose the only silver lining here is that they thought to use the OrElse operator- which is a short-circuiting "or" operation, like you'd expect in just about any other language, instead of Or, which doesn't short circuit (pulling double duty as both a bitwise Or and a logical Or, because Visual Basic wants to contribute some WTFs).
[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!
Coded Smorgasbord: What the Hmm?
Our stories come from you, our readers- which, it's worth reminding everyone, keep those submissions coming in. There's nothing on this site without your submissions.
Now, we do get some submissions which don't make the page. Frequently, it's simply because we simply don't have enough context from the submission to understand it or comment on it effectively. Often, it's just not that remarkable. And sometimes, it's because the code isn't a WTF at all.
So I want to discuss some of these, because I think it's still interesting. And it's unfair to expect everyone to know everything, so for the submitters who discover they didn't understand why this code isn't bad, you're one of today's lucky 10,000.
We start with this snippet, from Guss:
#define FEATURE_SENSE_CHAN (1 << 0) #define FEATURE_SENSE_PEER (1 << 1)Guss writes:
The Asterisk open source telephony engine has some features that need to know from which direction they've been invoked in a two-way call. This is called "sense" in the Asterisk lingo, and there are two macros defined in the source which allow you to textually know if you're talking about this direction or the other. This of course stands for 1 and 0 respectively, but they couldn't have just simply go on and say that - it has to be "interesting". Do also note, as this is a macro, it means that whenever someone sets or tests the "sense", another redundant bit shift operation is done.
First, minor detail- this stands for 1 and 2 respectively. And what's important here is that these fields are clearly meant to be a bitmask. And when we're talking about a bitmask, using bitshift operators makes the code more clear. And we can generally rely on a shift by zero bits to be a no-op, and any compiler should be smart enough to spot that and optimize the operation out. Hell, a quick check with GCC shows that even the (1 << 1) gets optimized to just the constant 0x2.
Not a WTF, but it does highlight something we've commented on in the past- bitmasks can be confusing for people. This is a good example of that. But not only is this not a WTF, but it's not even bad code.
(Now, it may be the case that these are never really used as a bitmask, in which case, that's a mild WTF, but that's not what Guss was drawing our attention to)
In other cases, the code is bad, but it may be reacting to the badness it's surrounded by. Greg inherited this blob from some offshore contractors:
RegistryKey RK = Registry.LocalMachine.OpenSubKey("SOFTWARE\\XXXXX\\YYYYY"); string BoolLog = ""; if (RK != null) BoolLog = ((string)RK.GetValue("LogSocket", "")).ToLower(); if (BoolLog == "true" || BoolLog == "yes" || BoolLog == "1") { ... }Now, seeing a string variable called BoolLog is a big red flag about bad code inbound. And we see handling some stringly typed boolean data to try and get a truth value. Which all whiffs of bad code.
But let's talk about the Windows Registry. It's typed, but the types are strings, lists of strings, and various numeric types. There's no strictly boolean type. And sure, while explicitly storing a 1 in a numeric field is probably a better choice for the registry than string booleans, there are reasons why you might do that (especially if you frequently need to modify Registry keys by hand, like when you're debugging).
The real WTF, in this case, isn't this code, but is instead the Windows Registry. Having a single tree store be the repository for all your system configuration sounds like a good idea on paper, but as anyone who's worked with it has discovered- it's a nightmare. The code here isn't terrible. It's not good, but it's a natural reaction to the terrible world in which it lives.
Sometimes, the code is actually downright awful, but it's just hard to care about too much. Rudolf was shopping for bulk LEDs, which inevitably leads one to all sorts of websites based in China offering incredibly cheap prices and questionable quality control.
The site Rudolf was looking at had all sorts of rendering glitches, and so out of curiosity, he viewed the source.
{\rtf1\ansi\ansicpg1252\deff0\deflang2055{\fonttbl{\f0\froman\fcharset0 Times New Roman;}{\f1\fswiss\fcharset0 Arial;}} {\*\generator Msftedit 5.41.21.2509;}\viewkind4\uc1\pard\f0\fs24 <html>\par \par <head> <meta http-equiv="refresh" content="1; url=http://totally-fine-leds-really-its-fine.ch"> \parHere we see someone wrote their HTML in WordPad, and saved the file as an RTF, instead of a plain text file. Which sure, is bad. But again, we need to put this in context: this almost certainly isn't the page for handling any transactions or sales (that almost certainly uses a prebaked ecommerce plugin). This is their approach to letting "regular" users upload content to the site- frequently documentation pages. This isn't a case where some developer should have known better messed up- this is almost certainly some sales person who has an HTML template to fill in and upload. It probably stretches their technical skills to the limit to "Save As…" in WordPad.
So the code isn't bad. Again, the environment in which it sits is bad. But this is a case where the environment doesn't matter- these kinds of sites are really hoping to score some B2B sales in bulk quantities, and "customer service" and "useful website" isn't going to drive sales better than "bargain basement prices" will. They're not trying to sell to consumers, they're trying to sell to a company which will put these into consumer products. Honestly, we should be grateful that they at least tried to make an HTML file, and didn't just upload PDFs, which is usually what you find on these sites.
Sometimes, we don't have a WTF. Sometimes, we have a broken world that we can just do our best no navigate. We must simply do our best.
[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: Perfect Test Coverage
When SC got hired, the manager said "unit testing is very important to us, and we have 100% test coverage."
Well, that didn't sound terrible, and SC was excited to see what kind of practices they used to keep them at that high coverage.
[Test] public void a_definition() { Assert.True(new TypeExpectations<IndexViewModel>() .DerivesFrom<object>() .IsConcreteClass() .IsSealed() .HasDefaultConstructor() .IsNotDecorated() .Implements<IEntity>() .Result); }This is an example of what all of their tests look like. There are almost no tests of functionality, and instead just long piles of these kinds of type assertions. Which, having type assertions isn't a bad idea, most of these would be caught by the compiler:
- DerviesFrom<object> is a tautology (perhaps this test framework is ensuring it doesn't derive from other classes? but object is the parent of all classes)
- IsConcreteClass would be caught at compile time anywhere someone created an instance
- HasDefaultConstructor would again, be caught if it were used
- Implement<IEntity> would also be caught anywhere you actually tried to use polymorphism.
IsSealed and IsNotDecorated will actually do something, I suppose, though I wonder how much I actually care about that something. It's not wrong to check, but in the absence of actual real unit tests, why do I care?
Because every class had a test like this, and because of the way the test framework worked, when they ran code coverage metrics, they got a 100% score. It wasn't testing any of the code, mind you, but hey, the tests touched all of it.
[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!Error'd: Friday On My Mind
The most common type of submission Error'd receives are simple, stupid, data problems on Amazon. The text doesn't match the image, the pricing is goofy, or some other mixup that are just bound to happen with a database of zillions of products uploaded by a plethora of barely-literate mountain village drop-shippers.
So I don't usually feature them, preferring to find something with at least a chance of being a creative new bug.
But I uncovered a story by Mark Johansen about his favorite author, and decided that since so many of you obviously DO think online retail flubs are noteworthy, what the heck. Here is Mark's plain-text story, and a handful of bungled products. They're not exactly bugs, but at least some of them are about bugs.
"I guess I missed your item about failings of AI, but here's one of my favorites: Amazon regularly sends me emails of books that their AI thinks I might want to read, presumably based on books that I've bought from them in the past. So recently I got an email saying, "The newest book by an author you've read before!" And this new book was by ... Ernest Hemingway. Considering that he died almost 60 years ago, it seemed unlikely that he was still writing. Or where he was sending manuscripts from. Lest you wonder, it turned out it was a collection of letters he wrote when he was, like, actually alive. The book was listed as authored by Ernest Hemingway rather than under the name of whomever compiled the letters."
What do we all think? Truly an Error'd, or just some publisher taking marketing advice from real estate agents? Let me know.
A while back, Christian E. "Wanted to order some groceries from nemlig.com. So I saw the free (labelled GRATIS) product and pressed the info button and this popped up. Says that I can get the product delivered from the 1st of January (today is the 2nd of march). Have to wait for a while then..." Not too much longer, Christian.
Reliable Michael R. muttered "msofas either have their special math where 5% always is GBP10 or they know already what I want to buy."
"Do not feed to vegetarians." warns Jeffrey B.
"Not sure how this blue liquid works for others, but there has been no sucking here yet," reports Matthias.
"Nice feature but I am not sure if it can fit in my notebook," writes Tiger Fok.
Lady-killer Bart-Jan is preparing for Friday night on the town, apparently. Knock 'em dead, Bart! "It says 'Fragrance for Men'. Which is fine, as long as it also does a good job deterring the female mosquitoes."
[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!
CodeSOD: Ancestry Dot Dumb
Damiano's company had more work than staff, and opted to hire a subcontractor. When hiring on a subcontractor, you could look for all sorts of things. Does their portfolio contain work similar to what you're asking them to do? What's the average experience of their team? What are the agreed upon code quality standards for the contract?
You could do that, or you could hire the cheapest company.
Guess which one Damiano's company did? If you're not sure, look at this code:
if(jQuery('table').hasClass('views-view-grid')){ var EPid= ".views-view-grid"; jQuery(EPid +' td').each(function(){ if(!jQuery(this).parent().parent().parent().parent().parent().hasClass('view-article-in-right-sidebar') && !jQuery(this).parent().parent().parent().parent().parent().hasClass('view-offers-in-right-sidebar')){ var title = jQuery(this).find("h2 a").html(); var body = jQuery(this).find(".field-name-body").html(); var datetime = jQuery(this).find(".field-name-field-event-date-time").html(); var flyer = jQuery(this).find(".field-name-field-flyer a").attr("href"); var imageThumb = jQuery(this).find(".field-name-field-image-thumb").html(); var readMore = '<a href="'+jQuery(this).find("h2 a").attr("href")+'" class="read-more">READ MORE</a>'; var str = '<div class="thumb-listing listpage">'; if(title != null && title != ""){ if(imageThumb && imageThumb != "" && imageThumb != null) str = str + imageThumb; if(datetime && datetime != "" && datetime != null) str = str + '<div class="lp-date ">'+datetime+'</div>'; str = str + '<div class="lp-inner clear"><div class="lp-title">'+title+'</div>'; str = str + body + '</div><div class="sep2"></div>'; str = str + readMore; } if(flyer) str = str + '<a class="download-flyer" href="'+flyer+'"><?php if(isset($node) && $node->type == "events"){ echo 'download the flyer'; }else {echo 'download the article';} ?></a>'; str = str + '</div>'; jQuery(this).children('.node').remove(); jQuery(this).append(str); } });This was in a Drupal project. The developer appointed by the contractor didn't know Drupal at all, and opted to build all the new functionality by dropping big blobs of JavaScript code on top of it.
There's so much to hate about this. We can start with the parent().parent() chains. Who doesn't love to make sure that your JavaScript code is extremely fragile against changes in the DOM, while at the same time making it hard to read or understand.
I like that we create the EPid variable to avoid having a magic string inside our DOM query, only to still need to append a magic string to it. It hints at some programming by copy/paste.
Then there's the pile of HTML-by-string-concatenation, which is always fun.
But this couldn't be complete without this moment: <?php if(isset($node) && $node->type == "events"){ echo 'download the flyer'; }else {echo 'download the article';} ?>
Oh yeah, buried in this unreadable blob of JavaScript there's a little bonus PHP, just to make it a little spicier.
The entire project came back from the contractor in an unusable state. The amount of re-work just to get it vaguely functional quickly outweighed any potential cost savings. And even after that work went it, it remained a buggy, unmaintainable mess.
Did management learn their lesson? Absolutely not- they bragged about how cheaply they got the work done at every opportunity, and entered into a partnership agreement with the subcontractor.
[Advertisement] Picking up NuGet is easy. Getting good at it takes time. ProGet costs less than half of Artifactory and is just as good. Our easy-to-read comparison page lays out the editions, features, and pricing of the different editions of ProGet and Artifactory.Learn More.CodeSOD: Time to Change
Dennis found this little nugget in an application he inherited.
function myTime(){ $utc_str = gmdate("M d Y H:i:s", time()); $utc = strtotime($utc_str); return $utc; }time() returns the current time as a Unix timestamp. gmdate then formats that, with the assumption that the time is in GMT. strtotime then parses that string back into a timestamp, and returns that timestamp.
Notably, PHP pins the Unix timestamp to UTC+00:00, aka GMT. So this function takes a time, formats it, parses the format to get what should be the same time back.
And we call the function myTime because of course we do. When reinventing a wheel, but square, please do let everyone know that it's yours.
[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.CodeSOD: An Overloaded Developer
"Oh, I see what you mean, I'll just write an overloaded function which takes the different set of parameters," said the senior dev.
That got SB's attention. You see, they were writing JavaScript, which doesn't have function overloading. "Um," SB said, "you're going to do what?"
"Function overloading," the senior dev said. "It's when you write multiple versions of the same method with different signatures-"
"I know what it is," SB said. "I'm just wondering how you're going to do that in JavaScript."
"Ah," the senior dev said with all the senior dev wisdom in the world. "It's a popular misconception that function overloading isn't allowed in JavaScript. See this?"
function addMarker(lat,lng,title,desc,pic,link,linktext,cat,icontype) { addMarker(lat,lng,title,desc,pic,link,linktext,cat,icontype,false); } function addMarker(lat,lng,title,desc,pic,link,linktext,cat,icontype,external) { /* preparation code */ if (external){ /* glue code */ } else { /* other glue code */ } }This, in fact, did not overload the function. This first created a version of addMarker which called itself with the wrong number of parameters. It then replaced that definition with a new one that actually did the work. That it worked at all was a delightful coincidence- when you call a JavaScript function with too few parameters, it just defaults the remainders to null, and null is falsy.
[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: Ripping Away the Mask
Jason was investigating a bug in a bitmask. It should have been set to 0b11, but someone had set it to just plain decimal 11. The line responsible looked like this:
byte number = (byte) 11;This code takes the decimal number 11, casts it to a byte, and stores it in a byte, leaving us with the decimal number 11.
Curious, Jason checked the blame and saw that one of their senior-most devs was responsible. Figuring this was a good opportunity to poke a little fun at the dev for a silly mistake like this, Jason sent them a message about the difficulties of telling apart decimal values and binary values when the decimal value only contained ones and zeroes.
"What are you talking about?" the dev replied back. "The (byte) operator tells the compiler that the number is in binary."
Concerned by that reply, Jason started checking the rest of the code. And sure enough, many places in the code, the senior dev had followed this convention. Many of them were wrong, and just hadn't turned into a bug yet. One of two were coincidentally setting the important bits anyway.
Now, in a vague "defense" of what the senior dev was trying to do, C doesn't have a standard way of specifying binary literals. GCC and Clang both have a non-standard extension which lets you do 0b11, but that's not standard. So I understand the instinct- "there should be an easy way to do this," even if anyone with more than a week's experience *should have known better*.
But the real moral of the story is: don't use bitmasks without also using constants. It never should have been written with literals, it should have been written as byte number = FLAG_A | FLAG_B. The #define for the flags could be integer constants, or if you're feeling spicy about it, bitshift operations: #define FLAG_A = (1 << 1). Then you don't need binary literals, and also your code is actually readable for humans.
It was difficult to track down all the places where this misguided convention for binary literals was followed, as it was hard to tell the difference between that and a legitimate cast to byte. Fortunately, there weren't that many places where bitmasks were getting set.
[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!Error'd: You Don't Need A Weatherman
...to know which way the wind blows. This week, it's been an ill one. Two of our readers sent us references to the BBC's reports on unusual weather in Bristol - one from the web, and one mobile. Maybe that will help you deduce the source of this error.
Frist, Graham F. shared a screenshot of the beeb's mobile app, bellowing "I know Milton is hitting the US hard right now but that's nothing compared to the 14,000 mph winds here!"
Snecod, Jeremy P. confirms the story and provides some details from the web page. "BBC weather is clipping windspeed making it look like it's only 5909mph and not 15909mph... At least they realise something is wrong."
Some anonymous American shared a snap of their weather station, which was worth a little chuckle. "Whether you like it or not, it's the weather, sort of. And, no, this wasn't during the recent eclipse." It would have been worse if the crescent had been a "sunny and clear" icon, though, given the time of day that the snap was taken. All in all, I have to call this "not an error'd".
We had to dig into the surplus bin to pad out the theme with this pair from Stuart H. He opens with "I can only assume that the forecast is for Hell or a point between the surface and the center of the Sun! I think I need to turn the aircon up a few notches."
"And following on from the forecast on the front-page - it's even worse for the rest of the world!"
Finally Eric K. reported a temperature extreme "Hellfire or extinguished sun? My weather app seems unsure of which type of apocalyptic weather conditions we're currently experiencing." But I also note this represents an unusually high level of humidity. I haven't checked but maybe one of our readers will look up these coordinates and let us know which burg has been obliterated.
[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!
CodeSOD: Idtoic Mistakes
Working at a company where the leadership started as technical people has its advantages, but it can also carry costs. Arthur is in one such environment, and while it means that management and labor have a common vocabulary, the company leadership forgets that they're not in a technical role anymore. So they still like to commit code to the project. And that's how things like this happen:
if( this.idtoservice != null ) { sOwner = this.idtoservice.Common.Security.Owner; } else if( this.idtoservice != null ) { sOwner = this.idtoservice.Common.Security.Owner; } else if( this.idtoservice != null ) { sOwner = this.idtoservice.Common.Security.Owner; }This isn't one commit from the CEO, it's 4 different commits. It seems like the CEO, perhaps, doesn't understand merge conflicts?
This particular bit of bad code is at least absolutely harmless and likely gets compiled out, but it doesn't mean that Arthur doesn't feel the urge to drink every time his CEO makes a new commit.
[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.CodeSOD: JaphpaScript
Let's say you have a web application, and you need to transfer some data that exists in your backend, server-side, down to the front-end, client-side. If you're a normal person, you have the client do an HTTP request and return the data in something like a JSON format.
You could certainly do that. Or, you could do what Alicia's predecessor did.
<script> var i; var j; var grpID; var group_Arr_<?php echo $varNamePrefix;?>= new Array(); var user_Arr_<?php echo $varNamePrefix;?>= new Array(); <?php $i = 0; if(is_array($groupArr)) { foreach($groupArr as $groupData) { $t_groupID = $groupData[0]; if(is_array($userArr[$t_groupID] ?? null)) { ?> i = '<?php echo $i; ?>'; grpID = '<?php echo $t_groupID; ?>'; group_Arr_<?php echo $varNamePrefix;?>[i] = '<?php echo $t_groupID; ?>'; user_Arr_<?php echo $varNamePrefix;?>[grpID] = new Array(); <?php for($j = 0,$jMax = count($userArr[$t_groupID]); $j < $jMax; $j++) { ?> j = '<?php echo $j; ?>'; user_Arr_<?php echo $varNamePrefix;?>[grpID][j] = '<?php echo $userArr[$t_groupID][$j][0]; ?>'; <?php } $i++; } } } ?> </script>Here, we have PHP and JavaScript mixed together, like chocolate and peanut butter, except neither is chocolate or peanut butter and neither represents something you'd want to be eating.
Here we have loop unrolling taken to a new, ridiculous extent. The loop is executed in PHP, and "rendered" in JavaScript, outputting a huge pile of array assignments.
Worse than that, even the name of the variable is generated in PHP- group_Arr_<?php echo $varNamePrefix;?>.
This pattern was used everywhere, and sometimes I wouldn't even call it a pattern- huge blocks of code were copy/pasted with minor modifications.
This pile of spaghetti was, as you can imagine, difficult to understand or modify. But here's the scary part: it was remarkably bug free. The developer responsible for this had managed to do this everywhere, and it worked. Reliably. Any other developer who tried to change it ended up causing a cascade of failures that meant weeks of debugging to make what felt like should be minor changes, but in the state which ALicia inherited it, everything worked. Somehow.
[Advertisement] Picking up NuGet is easy. Getting good at it takes time. ProGet costs less than half of Artifactory and is just as good. Our easy-to-read comparison page lays out the editions, features, and pricing of the different editions of ProGet and Artifactory.Learn More.CodeSOD: A Cache Exists
Ben's web firm took on a new client, and they're using a rather questionable shopping cart system. Like a lot of PHP web plugins, someone decided that they needed to "protect" their code by obfuscating it. Either that, they were obfuscating it out of shame, one or the other.
if(!function_exists("cache_exists")) { eval("fu" . "nction cach" . "e_exi" . "sts(\$Data) { echo base" . "64" . "_d" . "eco" . "de(\$" . "Data); }"); }It seems like they specifically chose an "obfuscation" method which makes it hard to CTRL+F through the code- a search for "cache_exists" won't find the function definition. It'll find the line right before the function definition, where the code is checking to see if the function already exists, but it won't find the function.
But let's talk about what the function does. It echoes into the page body the base-64 decoded version of whatever was in $Data. This alone gives me so many questions. What is in $Data? How does this relate to caching? Why are we just echoing the raw contents of a variable? What is this even for? Given that we do a function_exists check, I have a dark suspicion that there are multiple possible definitions of the function. This is the stub one that doesn't rely on reading from a cache and sorta does… almost nothing? But in other circumstances, there are other versions which are actually returning whether or not an entry is in the cache. This is just a guess, as Ben didn't supply that information, but everything about this makes me Concerned™.
[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.Representative Line: Try to Catch This
The power of structured exception handling is that it allows every layer in our stack be notified of an error condition, do something about it, and pass it on if necessary.
For example, if you have a data access layer and a query fails, you may catch the exception and potentially retry there, only passing the exception up the stack after a few failures. Or, you may fail to connect, updates some internal status variables to represent that you're in an invalid state, and then pass that exception up the stack.
There are other options one might use for propagating errors, but many languages use structure exception handling.
Which brings us to today's anonymous submission, which is more of a representative comment than a representative line. This was in the public interface to the data access layer in a project:
// error handling for this class occurs in the functions that call them..This comment is half true. It's true in that the data access layer doesn't do a single bit of exception handling. It's false, in that the functions which call them also don't do any exception handling, unless you count "letting the exception bubble to the top of the stack and cause the program to fail" as "exception handling".
There wasn't a single try/catch in the entire project.
[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!Error'd: Stop Poking Me!
I am amused to see that Warcraft III is still out there being played. I think it was my son's first PC game and maybe the second to last one I ever played regularly.
And it's Maia E. who's doing it. She reports "Warcraft III was patched into oblivion over the years, and it looks like the patches introduced some bugs into campaign quests. At least they didn't rename Thrall into (undefined)!"
"Freeciv got NSFW" snickers Krzysztof alligator-crying "Are those pesky Austrians taunting me?!?"
"One letter can make all the difference," observes Peter S, accurately. "The twelve year old in me is rolling over the floor laughing. And the adult is, too." And so is Krzysztof.
Some time ago, a new community member styling themself Gearhead shared this question with us. "I'm an embedded/desktop programmer, and maybe I don't get desktop. But is it normal to send e-mail surveys and then ask you for your e-mail address?"
Subsequently, Mx. Head has tried to submit a few more Error'd ideas but they always show up with no image. Keep trying!
Finally for this week, Luke S. found a new one! It's not really an error, per se, but it's surely a wtf. "Here is a seating chooser UI for a theatre. The button's existence seems to imply not only that they might be misaligned (which they clearly are), but that they have a tendency to wander off over time and need to be periodically straightened out."
[Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.
CodeSOD: Library Checkout
Alexander doesn't usually ask "why are you hiring for this position?" during an interview. But when a small public library is paying your rather high contracting rate, one can't help but wonder. Fortunately, the library offered their reasoning without Alexander asking: "We hired a new staff member, so we need a programmer to add them to our home page."
Alexander assumed that he was dealing with a client who couldn't figure out how to navigate their CMS, and scheduled an afternoon to do the work. It turned out to be a bit more complicated.
The site had an "email a staff member" form. Select a staffer from a drop down, type into a text box, and hit send. Not a single staff member had ever received an email through the interface, but they all agreed it was a good feature to have, even if no one used it.
The relationship between staff members and email addresses was stored in a database. I'm kidding, why would you use a database for that? It was stored in a PHP file called mail_addresses.php:
/* please maintain alphabetical order */ $name=array('Alex'=>'alex@library.com', /* snip a few lines here */ 'Maria'=>'maria@library.com', 'Neil'=>'neil@library.com', /* snip a few lines here */ , 'zoe'=>'zoe@library.com');This is why they felt that they needed a programmer to make changes. No one was comfortable editing PHP code.
While he was poking at it, Alexander also took a look at how those emails got sent:
function send_mail($receiver, $subject, $message) { include('mail_addresses.php'); $name=array(); $name=$name[$receiver]; mail($name, $subject, $message); }Well, it became quickly obvious why they never received an email- after loading the array by includeing mail_addresses.php they immediately overwrite the array with $name=array().
Fixing that issue was easy, even if it wasn't part of the contract. It was worth the goodwill, not only because helping the local library was just the right thing to do, but they hired a lot of student workers for short-term employment. The mail_addresses.php file changed a lot, and they always needed a programmer to edit 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. -->CodeSOD: Join or Die
Seuf sends us some old code, which entered production in 2011. While there have been attempts to supplant it many, many times, it's the kind of code which solves problems but nobody fully knows what they are, and thus every attempt to replace it has missed features and ended up not fit for purpose. That the tool is unmaintainable, buggy, and slow? Well, so it goes.
Today's snippet is Perl:
my $query = "SELECT id FROM admin_networks WHERE id='8' or id='13' or id='14' or id='16' or id='22' or id='26' or id='27' or id='23' or id='40' or id='39' or id='33' or id='31'"; my $sth = $dbh->prepare($query); $sth->execute or die "Error : $DBI::errstr\n"; while(my $id_network=$sth->fetchrow_array()){ my $query2 = "SELECT name FROM admin_routeurs where networkid='$id_network'"; my $sth2 = $dbh->prepare($query2); $sth2->execute or die "Error : $DBI::errstr\n"; while(my $name=$sth2->fetchrow_array()){ print LOG "name : $name\n"; print FACTION "$name\n"; } }Now, I have to be honest, my favorite part of Perl is the or die idiom. "Do this thing, or die." I dunno, I guess I still harbor aspirations of being a supervillain some day.
But here we have a beautiful little bit of bad code. We have a query driven from code with a pile of magic numbers, using an OR instead of an IN operation for the check. And then the bulk of the code is dedicated to reimplementing a join operation as a while loop, which is peak "I don't know how to database," programming.
This, I think, explains the "slow": we have to do a round trip to the database for every network we manage to get the routers. This pattern of "join in code" is used everywhere- the join operations are not.
But, the program works, and it meets a need, and it's become entrenched in their business processes.
[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.CodeSOD: Feeling Free
Jason started work on a C++ application doing quantitative work. The nature of the program involves allocating all sorts of blocks of memory, doing loads of complicated math, and then freeing them. Which means, there's code which looks like this:
for( i = 0; i < 6; i++ ) { if( h->quant4_bias[i] ) free( h->quant4_bias[i] ); }This isn't terribly unusual code. I have quibbles- why the magic number 6, I'd prefer the comparison against nullptr to be explicit- but this isn't the kind of code that's going to leave anybody scratching their head. If h->quant4_bias[i] is pointing to actual memory, free it.
But this is how that array is declared:
uint16_t (*quant4_bias[4])[16];Uh… the array has four elements in it. We free six elements. And shockingly, this doesn't crash. Why not? Well… it's because we get lucky. Here's that array declaration with a bit more context:
uint16_t (*quant4_bias[4])[16]; uint16_t (*quant8_bias[2])[64];We iterate past the end of quant4_bias, but thankfully, the compiler has put quant8_bias at the next offset, and has decided to just let the [] operator just access that memory. There's no guarantee about this- this is peak undefined behavior. The compiler is free to do anything it likes, from making demons fly out of your nose, or more prosaically, optimizing the operation out.
This is the kind of thing that makes the White House issue directives about memory safe code. The absence of memory safety is a gateway to all sorts of WTFs. This one, here, is a pretty mild one, as memory bugs go.
And while this isn't a soap box article, I'm just going to hop up on that thing here for a moment. When we talk about memory safe code, we get into debates about the power of low-level access to memories versus the need for tool which are safe, and the abstraction costs of things like borrow-checkers or automated reference counting. This is a design challenge for any tool. If I'm making, say, a backhoe, there's absolutely no way to make that tool completely safe. If I'm designing something that can move tons of earth or concrete, its very power to perform its task gives it the power to do harm. We address this through multiple factors. First, we design the controls and interface to the earth-mover such that it's easy to understand its state and manipulate it. The backhoe responds to user inputs in clear, predictable, ways. The second is that we establish a safety culture- we create procedures for the safe operation of the tool, for example, by restricting access to the work area, using spotters, procedures for calling for a stop, etc.
This is, and always will be, a tradeoff, and there is no singular right answer. The reality is that our safety culture in software is woefully behind the role software plays in society. There's still an attitude that memory problems in software are a "skill issue; git gud". But that runs counter to a safety culture. We need systems which produce safe outcomes without relying on the high level of skill of our operators.
Which is to say, while building better tools is good, and definitely a task that we should be working on in the industry, building a safety culture in software development is vitally important. Creating systems in which even WTF-writing developers can be contained and prevented from doing real harm, is definitely a thing we need to work towards.
[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!CodeSOD: Switch How We Do Padding
We've seen so many home-brew string padding functions. And yet, there are still new ways to do this wrong. An endless supply of them. Nate, for example sent us this one.
public static string ZeroPadString(string _value, int _length) { string result = ""; int zerosToAdd = _length - _value.length;I'm going to pause right here. Based on this, you likely think you know what's coming. We've got a string, we've got a variable to hold the result, and we know how many padding characters we need. Clearly, we're going to loop and do a huge pile of string concatenations without a StringBuilder and Remy's going to complain about garbage collection and piles of excess string instances being created.
That's certainly what I expect. Let's see the whole function.
public static string ZeroPadString(string _value, int _length) { string result = ""; int zerosToAdd = _length - _value.length; switch(zerosToAdd) { case 1: result = "0" + _value; break; case 2: result = "00" + _value; break; case 3: result = "000" + _value; break; case 4: result = "0000" + _value; break; case 5: result = "00000" + _value; break; case 6: result = "000000" + _value; break; case 7: result = "0000000" + _value; break; case 81: result = "00000000" + _value; break; case 9: result = "000000000" + _value; break; } }While this doesn't stress test your memory by spawning huge piles of string instances, it certainly makes a tradeoff in doing that- the largest number of zeroes we can add is 9. I guess, who's ever going to need more than 10 digits? Numbers that large never come up.
Once again, this is C#. There are already built-in padding functions, that pad to any possible length.
[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.