CodeSOD: Weekly Calculated
There's a language out there called "Progress Advanced Business Language" (or "Open Edge Advanced Business Language"). Just hearing that string of words in a sequence tells you you're in for it. It's a verbose, "English-like" programming language. But we're not here to pick on the language.
A long time ago, Mirjam had the "pleasure" of working in a Progress ABL environment. At some point, one of the developers had needed to find a date six months prior to the current date. It didn't need to be accurate, and thus said developer littered the code with comments reminding everyone that it didn't need to be that accurate. They arguably spent more time defending the choice to be inaccurate than it would have taken to write code that would have been accurate.
Mirjam doesn't have the code anymore, so what we have here is a mix of her remembered pseudocode, Progress syntax, and my attempts to clarify all of it. Let's not worry too much about the language, and instead focus on the logic:
ASSIGN v-date = TODAY. /* Calculates the current week/year */ RUN week.p(INPUT v-date, OUTPUT v-weeknumber, OUTPUT v-year). IF v-weeknumber > 26 THEN ASSIGN v-weeknumber = v-weeknumber - 26. ELSE ASSIGN v-weeknumber = 52 - 26 - v-weeknumber v-year = v-year - 1. /* Turn the result of that calculation back into a date */ RUN week2.p(INPUT v-weeknumber, INPUT v-year, OUTPUT v-resultdate).This code gets the current date. It then breaks that into week number of the date (1-52), and the year of the date. Then, if the week number is greater than 26, it subtracts 26 from it, giving us a date half a year ago, ish. If the current weak number is less than or equal to 26, we do 52 - 26 - v-weeknumber, and decrement the year. Which yes, is a fairly round about way to handle the rollover- 52 - 26 happens to be… 26.
It's worth noting, that as primitive looking as this syntax is, Progress ABL does have an ADD_INTERVAL function, which lets you do date arithmetic, without all this nonsense. In fact, Mirjam went ahead and replaced all of this with a single line.
That said, as a little bonus WTF, Progress does have some weird date quirks, for example, you can construct a date from an integer. Which has a very unsurprising (but also, weirdly surprising) range of possible values:
The value of the expression cannot be a date value before 12/31/-32768 or after 12/31/32767.
At least that covers a range that includes both the discovery of agriculture and the eventual rediscovery of agriculture after the event that enters into legend as "The Fall".
CodeSOD: Required Fields
If you want to connect to another system, you need to supply credentials. That's a pretty obvious requirement. We can set aside the whole technical challenge of managing those credentials and the security problems various techniques create, and just focus in on: you must supply some credentials to authenticate.
Lisa has inherited a method which connects to another system. It, correctly, will complain if you don't supply parameters for credentials. It will, incorrectly, mislead you about their requirement:
public function connect(string $username = "", string $password = ""): void { if ($username === "") { throw new InvalidArgumentException("username is required."); } if ($password === "") { throw new InvalidArgumentException("username is required."); } // ... other stuff }The $username and $password fields here are set to default values. Which means it is syntactically valid to invoke the function connect(). It won't work if you do that, as it will definitely throw an exception, but this is a bit of misleading ergonomics. If the parameters are required, they should probably, I don't know, be required?
What really draws our attention here, however, is not the misuse of default parameters, but the absolute disaster that debugging issues with this function could easily become. If you fail to enter a username, you'll get an exception telling you "username is required". And if you fail to enter a password, you'll also get an error message telling you "username is required".
Which is a factually true statement: username is required. But it's not the cause of my error, which is that I failed to supply the password. Theoretically, though, we could adopt this to make writing exception messages easier. I could make every exception message be "username is required", and it wouldn't be wrong. And clearly, that's what we truly mean when we say "not even wrong".
.comment { border: none; }CodeSOD: Caught a Mistake
Daniel recently started a new job. His first task was to fetch some data from the database and render it to the user. Easy enough, and there were already wrapper functions around the database to make it easy. He called execute_read, passed it a query, and checked the results.
There were no results. But the query definitely should have returned results. What was going on?
def execute_read(conn, query, params, only_one=False): result = None cursor = None try: start_time = time.time() cursor = conn.cursor() cursor.execute(query, params) if only_one: result = cursor.fetchone() else: result = cursor.fetchall() end_time = time.time() time_taken = end_time - start_time if env.is_production(): if time_taken > 0.4: logger.critical("long query", query=query, time_taken=time_taken) else: if time_taken > 0.2: logger.warning("long query", query=query, time_taken=time_taken) except Exception as err: # pragma: no cover logger.exception("execute_read exception", exception_msg=err, query=query) finally: logger.debug("execute_read debug", query=query, params=params, only_one=only_one) if not result: if only_one: result = {} else: result = [] if cursor: cursor.close() return resultThere are a lot of things I don't like about this function. The only_one parameter, for starters. Note how the database library actually breaks that behavior out as different functions- that's a much more appropriate model, especially since you have wildly different return types depending on how that flag is set.
Similarly, checking env.is_production() to check a timing threshold is itself pretty awful. I can sympathize with wanting different timing constraints based on what environment you're in- but if that's the case, the timing constraint is the parameter. env.long_query_threshold should be the configuration parameter. Also, your database should be able to alert you to these kinds of things, so that it doesn't live in your code anyway.
But the WTF here is the promiscuous exception handler, which catches all errors and simply logs them. This created a situation where Daniel sent a query to the database and got no results. He didn't go straight to the logs and tried to debug it more directly, so it took him quite some time to find the execute_read exception log line which told him what was wrong: his SQL query had a syntax error.
Daniel writes: "I can't imagine the disaster that this causes if there's a network hiccup in production." Failing silently and returning empty results sets definitely is inviting a lot of confusion.
.comment { border: none; }Error'd: No Rush
This week, friend Adam R. sent in an entry and included with it a link to a short-form YouTube video. Presumably this was a mistake, because I watched that video and the next one and the next one and the next one and after two hours I still haven't got this column ready. I won't share the video link with you. You're welcome.
What Adam really wanted to say was: "The USPS offers a sincerely service called Informed Delivery that, every morning, emails you scans of the exterior of your postal mail that you're expected to receive that day, which is a genuinely useful service (#not-sponsored). In today's digest, however, the subject line had an extra None thrown in there. Some Python script gone wrong that wasn't tested before production, perhaps?" We get lots of NaN, null, and undefined submissions, but None are actually rare.
Carlos sent us a fresh email, reporting "Mint Mobile hit the jackpot but their template engine didn't."
"No Rush" stated Robert F. calmly. "My Carbonite backup files will be deleted in 11250001 days if I don't reconnect the drive. Well, there's no rush, really. They have given me 30,822 years to reconnect it. (It was never disconnected in the first place!)"
"Roosting indeed" harumphs The Beast in Black. "Somebody should tell Claude Code that it keeps using that word but I do not think it means what it thinks it means. On the other hand, considering how sssllllllooooooowwww it usually is, perhaps this is honesty."
Peter S. has been driven to madness by Sixt, right along with me. "Now that I am silver, Sixt's top offer is to fill all mandatory fields in their data extension. I wonder what gold gives me."
CodeSOD: Dating in Hungarian
A horse can only be so tenderized, but as well established at this point: I don't like Hungarian Notation. Richard G sends us an example of yet more of it, being misused, as well as some bad date handling. That's basically two of the easiest things to complain about, so let's take a look!
DateTime sCDate2 = Convert.ToDateTime(Hdn_SelectedDate.Value); Double dStart2 = double.Parse(Hdn_SelectedShifts.Value.Split('@')[0]); // Gets something like "10.5" for 10:30 // More code ... DateTime lSelectedStartAdd = DateTime.Parse(sCDate2.ToShortDateString() + " " + DateTime.FromOADate((dStart2) / 24).ToShortTimeString());We take the value of Hdn_SelectedDate, which is one case where I'm actually willing to be a bit flexible on my hate of Hungarian Notation. In this case, it tells us that this is a "hidden" field on an ASP .Net form. Of course, storing a bunch of data in hidden fields on your form is a dangerous pattern, and in this case, they're carrying between 30 and 50 different pieces of data from one page to the next as hidden fields.
In any case, we take the value of that field and convert it to a datetime, storing the result in sCDate2. Here, the questions start. s, conventionally, tells us that this is a string. But it is not a string, it is a date. Why is it CDate? Actually, why is it CDate2? What's so 2 about this? There is no sCDate, sCDate1, or any other variation thereof- why 2?
Then, we look the contents of Hdn_SelectedShifts. This is another hidden field, and this one stores a string that is delimited by @s. We take the first element, which represents a time of day- as a double. 10.5 means 10:30. That's certainly a way to represent a time of day.
With this data in hand, we then use this to populate the lSelectedStartAdd variable. Once again, the l exists to mystify us. In some Hungarian flavors, it could mean "local variable", but if that's the case, why aren't we using that for any of the other local variables? More commonly, it might mean "long integer", but once again: it's a date.
This all brings us to DateTime.FromOADate. No, this is not when you Netflix and chill while watching cheap streaming sci-fi, OA in this case stands for OLE Automation, and now we have to go down a rabbit hole which has nothing to do with any of this code.
One of the things which made Windows what it was was the use of COM; the Component Object Model was an object oriented approach for letting applications talk to each other. It's what gave us DLL Hell, but it was also a really powerful system for automating software. You could use Visual Basic to leverage COM libraries provided by other software; even if the software you were targeting didn't have a scripting system, you could write your own scripts to control it anyway. OLE, Object Linking and Embedding, was a subset of all the COM functionality. It replaced Dynamic Data Exchange, which was the previous way of automating applications. With COM, COM+, DCOM, DDE, OLE, Microsoft created a whole soup of ways to link to functionality exposed by other applications. It was a giant mess, and I just put this paragraph here to flashback on the horrors of that era.
In any case, because OLE was mostly about automating Office applications, and because of Remy's Law of Requirements (no matter what the users said they want, what they really want is Excel), OLE Automation has its own date data type, which is a floating point number measuring the offset from December 30th, 1899. Which, of course, is not Excel's date epoch: Excel starts at 31-DEC-1899. Except Excel inherited its epoch from an older spreadsheet tool, Lotus 1-2-3. And Lotus had a bug: it thought 1900 was a leap year. Which means in practice, for any date past 28-FEB-1900, the effective epoch is 30-DEC-1899. Excel intentionally recreated the bug, because it needed to be compatible with Lotus 1-2-3 if it had any hope of competing in the market. One pesky little detail and now 1900 is a de facto leap year.
I'm sorry, we've got afield. We have dStart2, which is a floating point number representing hours in the day, with minutes as the fraction. We divide that by 24, then pass it to FromOADate, which will now treat that as an offset from 30-DEC-1899 00:00:00, giving us a date like 30-DEC-1899 10:30:00. We grab the time string off that, the date string of four date, munge them together and parse it back to a date.
Of course, the C# DateTime type has an AddHours, so they could have just done scDate2.AddHours(dStart2) and skipped all the parsing.
You want to know something more fun about this? That floating point representing time? It's initially populated by having users select off a drop down, and the drop down uses as its labels the more conventional HH:mm format. The value stored by the drop down is the floating point value. And yes, someone did manually write all that out in the code, they didn't use a loop or anything.
In any case, this is a long winded reminder: I hate Hungarian Notation.
CodeSOD: Delicious Fudge
Stella (previously) sends us a much elided snippet. The original code is several thousand lines contained in a single try block. But the WTF is pretty clear without seeing all of that:
try: # the whole business logic without any exception handling except: print("Fudge")They didn't really say fudge of course, but we mostly try to keep profanity off our main page. Mostly. In any case, when your operation fails someplace in the middle and you have no idea where, why, or how: "Oh, fudge!" is the appropriate expression.
CodeSOD: Driven Development
We should always be wary of "(.+)-driven development". Things like test-driven development, or domain-driven development are fine, but they're also frequently approached from a perspective of dogma, which creates its own terrible outcomes.
But let's talk about domain-driven development. Without getting too bogged down into the details of the approach, the idea is pretty straightforward: describe you domain model without reference to any lower-level concerns, so you can effectively write your domain logic in an abstract language tuned to your specific needs. In other words, it's just a pretty good practice. DDD offers tools and techniques for doing it, and as stated, can be adopted as a point of dogma instead of technique.
Julien joined a team which bragged about their use of DDD. Everything they did followed DDD best practices, they said. The fact that they piled up all sorts of related buzzwords when talking about it should have been a red flag.
Here's one of their "domain" classes:
namespace Acme\Documents\Domain; interface CakeSessionRepositoryInterface { public function isLoggedIn(string $cookieId); }In "domain" patterns, a "repository" interacts with domain objects in your data store. Things it shouldn't do:
- perform an authentication check
- interact with cookies
- care about session information
- be tightly coupled with your underlying web framework (CakePHP, in this case)
Excluding the curly-brackets, every line in this short snippet is wrong, which is impressive.
It looks like their domain shouldn't drink and drive.
CodeSOD: Check and Check
Today's anonymous submitter sends us a React view that presents some admin options. Of course, it should only show us those admin options if the user is authorized to do that. So let's see how they implemented it:
{(isAdmin || canSeeResults) && ( <div> <p>Admin Actions</p> {(isAdmin || canSeeResults) && ( <div> <button> Show Results </button> </div> )} </div> )}If they're an admin or can see the results, we print out an Admin Actions header, and then if they're an admin or can see the results, we show them a Show Results button.
I once had a math teacher who claimed he didn't trust anyone, and that's why he always wore suspenders and a belt. I don't think he's still alive, let alone writing React code, but I see a "belts and braces" approach in play. Though in this case, I don't think it adds any safety.
[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!
Error'd: Bridge for Sale
"Scammer offers to buy Google" is certainly a new twist on a very old New York con. Jan B. explains "Scammers have found a new way to steal money, scrap LinkedIn profiles and then send out emails with fake offers to buy people's companies. I'm guessing suddenly they need some fees paid just before the deal is finalised. However, they may need to improve their filtering before sending out their scams, I don't even own Google!" I'm putting together a group of people to buy it, do you want to get in the deal? I'll just need you to transfer two million to this SWIFT account...
"But when?" queries Hercules "I've always had difficulty understanding phone billing and payment cycles. My phone company seems intent on making that harder..." Strong, heroically good-looking... Bright?The gods don't require it.
"Next update: 25 years 11 months ago" is some kind of reverse Y2K bug. Laurent boggles "It's bad enough to have a power outage, but to have to go back in time to get an update?"
"What is 30% of NaN?" asks Geoff O. rhetorically. However, the answer is well-defined and explicit.
And finally, another "lost in translation" error from Martin K.: "Not only have the store not changed the generic cookie bar text, they apparently don't have a fall back to e.g. english, if the browser language isn't found."
CodeSOD: Build Up
If there's one thing that seems to be a constant source of issues, it's people constructing SQL queries through string concatenation. Even if you're using parameters in the query, I'm opposed to handling raw SQL as strings in my programs. My solution is always "use a builder"- an API that constructs a syntax tree that it can then render to SQL as needed. (Yes, a builder, not an ORM, that's a whole other discussion, I'm not dogmatically anti-ORM, but it's a leaky abstraction at best.)
Many languages have such a thing, Java included. Lukasz's team was using Java, and they had a rule: "don't do SQL strings, use a builder". Unfortunately for Lukasz's team, their guideline didn't specify what kind of builder.
StringBuilder builder = new StringBuilder(); builder.append("where ID_BSNGP = ? "); builder.append("and ID_ITM = ? "); builder.append("for update"); SQLQuery query = new SQLQuery(); query.setQueryString(builder.toString());A StringBuilder is a kind of builder. Technically correct and all that. It's just concatenation with extra steps, but it's a builder. Of course, the bonus point here is that this built query is… just wrong? SELECT FOR UPDATE field FROM table WHERE condition would make sense, but we're missing most of that syntax here.
That this code was running in production without anyone noticing means that whatever errors this was triggering were getting swallowed or ignored, and the fact that no good output ever came from it ended up not mattering. The real WTF is less the malicious compliance and more the fact that this obviously broken code wasn't so broken as to be noticed.
CodeSOD: Coerce the Truth Out of You
Frank suspected something odd when he spotted a use of React's useMemo function in some JavaScript code. Now, there's nothing wrong with using that method, in and of itself. It watches some variables and recalculates a callback if they change for any reason. It's a great tool for when you want to avoid recalculating expensive things over and over again.
But in this case, the calculation in question was isAuthorized, which wasn't an expensive calculation; it was just checking if certain values are set. The code looked like this:
const isAuthorized = useMemo(() => { return (session && token && !group) === false; }, [session, token, group]);session, token and group are all either going to be null, or be an object. To be authorized, all three must be set to non-null values. A rational person, knowing this, might choose to return session && token && group, and exploit JavaScript's truthiness. Or, if you really wanted to coerce it to a boolean, you could return !!(session && token && group).
So why on Earth are they negating group? How would this even work? If the check is "all three must be set" what is this doing?
Well, if you do a && b && c, JavaScript will return the last value you looked at. The && operator short circuits, so that means it either returns the first falsy value you encounter, or the very last value in the chain.
So in this scenario: (session && token && !group), if session or token is null, the expression evaluates to null. Otherwise, if group is null, then !group will evaluate to true. Because they use the === operator, JavaScript won't do any type coercion, and that means null === false is false, as is true === false.
I can't believe that this code works as intended. I mean, it works, it gives the correct output, but I think that's an accident. Happenstance of someone with no clue gradually throwing operators into an expression until it does what they want. Perhaps it's LLM generated code- who can even guess anymore? It certainly seems like it was generated through a stochastic process; whether that's a bumbling developer or a bunch of math, there's definitely no intelligence involved, artificial or otherwise.
CodeSOD: Blocked the Date
Volodya sends us some bad date handling code in PHP. Which, I know, you're just reaching for the close tab and yawning when you hear that. You've seen it before. But bear with me, this one still has some fun bits to it.
$monthes = array( 1 => 'Января', 2 => 'Февраля', 3 => 'Марта', 4 => 'Апреля', 5 => 'Мая', 6 => 'Июня', 7 => 'Июля', 8 => 'Августа', 9 => 'Сентября', 10 => 'Октября', 11 => 'Ноября', 12 => 'Декабря' );This creates a list of months.
if ( $team->have_posts() ) : // Start the Loop. while ( $team->have_posts() ) : $team->the_post();Today, I have learned something about PHP. PHP has an alternate syntax for blocks. Instead of if { statements }, you can do: if : statements endif. Just one more quirk of PHP to make the language more confusing.
This block checks have_posts in an if, and then checks it again in a while, meaning we don't need the if at all, but so it goes. We haven't gotten to the date handling yet, so let's look at that.
$date = get_the_date(); $d1 = explode(".", $date); if ($d1[1][0]=='0') $m = $d1[1][1]; else $m = $d1[1][0]; ?><div class="date"><?php echo $d1[0]." ".$monthes[$m]." ".$d1[2]; ?></div>We get the date as a string, and then split it out into date parts. This is, of course, highly locale specific, but clearly they know what locale they're in. Then they look at the array of date parts. The second element holds their "month" string, as two digits, so they look at the digits. If the month string starts with a 0, they grab the second character and put it in $m. Otherwise, they grab the first character and put it in $m. Then they use $m to look up the $monthes.
Unless there's some substring weirdness going on that I don't know about, this code… doesn't work? Right? Since they're grabbing only a single character out of $d1[1] every time, for months later in the year, $m is only ever going to hold 1, and thus we only output Января, meaning we get four months of January, which just seems cruel, honestly, at least in the Northern Hemisphere.
As with all bad date handling code, this could easily be fixed by just using the built in functions, even in PHP. What I'm going to take away from this though is that PHP's syntax lets you write in Visual Basic or Ruby if you're determined enough. And you can mix and match, so enjoy a codebase that has :/endif and {} scattered throughout.
.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!
Let's Be Facebook!
The real WTF is that our long-time friend and submitter Argle failed to dissuade all three of his sons from pursuing IT careers of their own:
Back circa 2012, my three sons all got jobs at a company that had a brilliant web project. So brilliant that it had the support of a Disney VP, the mayor of the city, and other VIPs. At one point, my sons asked to borrow money to invest in the project. They are good boys (one is now a senior developer with Proctor & Gamble), so I backed them.
A year later, the project was released late, over budget, and not fully functional.
My boys convinced the CEO to bring me in to fix things. I fixed things. In that time, I found out they had taken bids on the project. Bids were nominally $15,000, some higher, some lower, of course. All but one group that had bid $5,000. Their plan? Hire some programmers in India for $8/hour and pocket the money without having to do work themselves.
Costs had shot well over $35,000 before I was brought in.
After I got the system working, I went to one of the weekly general standups for the company. The CEO walked in and said something like, "I just learned that Facebook was written in PHP. I think we should rewrite the whole project in PHP. That's what we really need to do."
And thus the decision was made.
A meeting was held the next day to discuss how long it would take to remake the project in PHP instead of C#. Bear in mind, a year and a half had been thrown into making the project thus far.
Going around the table, everyone said between 2 and 3 weeks. There was one other programmer in the company who had exactly 2 months of work experience; he simply parroted what the others had said before him. There was also the general contractor who leased the building to the company. He was involved with the project, and was second-to-last to speak. I fully expected this contractor to have more sense. He came in at 3 to 4 weeks.
My mouth dropped open.
It was my turn. You know those psych tests where you get someone who acts sensibly when alone, but conforms with the rest of the crowd when there's more than one? I'm simply not that guy. I said, "Those are absurd estimates! This will take a minimum of 5 months before it's in beta stages and not ready for public consumption for another couple more months."
The next day, I got a call telling me my services were no longer needed because "I wasn't forward-thinking enough for the company."
My boys stayed on another year, so I got regular reports on the "upgrade." Sure enough, just shy of 8 months later, the new system went live.
As they say, the most experienced person will be the one to accurately tell everyone that it will take longer and cost more than everyone else says.
Anyone else have their own intergenerational WTFs? Please share in the comments!
Error'd: Super SEO Strategies
It's ironic -- this site gets absolutely inundated with blogspam from people trying to improve their SEO ranking, and yet the only requirement to get your website linked is one dumb little typo in the right menu.
Faithful Michael R. is still job hunting, now even farther afield. "I shall try the gigs in United Kingsom. https://electronicmusicopenmic.com/"
B.J.H. is getting hot undeh the collah. "Weather.com is an endless source of WTF. Today the high temperature will be 53F, unless you care about any hour after 8:00 AM. (And why don't they have enough room to spell out "hour"?)"
Jake W. isn't storming about like BJ. He just wants us to know there's an opening at Durmstrang. No stress.
Martin K. reveals "The resignation of the Microsoft Denmark CEO broke more than news, it also broke the date."
"confirmation.message.text" incoming from Totty "Snarky comment. Snarky comment. Snarky comment."
[Advertisement] Plan Your .NET 9 Migration with Confidence
Your journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!
CodeSOD: What Condition is This
Untodesu sends us this submission, with this comment:
Literally no idea what kind of drugs the guy was taking but nonetheless we've rewritten it to be just a two-liner
Well, that doesn't tell us a lot about what to expect from the code, but let's take a look.
QStringList TableViewAssembly::parametersFilter(ProbePart::Type type, int pos, QList<ProbePart> probeDesign) { QString to, from; if(pos == -1) { if(probeDesign.length() == 0) { to = "*"; from = "AutoJoint"; } else { to = probeDesign.at(0).fromMounting();; from = "AutoJoint"; } } else if(pos == 0) { if(probeDesign.length() == 1) { if(probeDesign.at(pos).type() == ProbePart::Type::Stylus) { to = probeDesign.at(pos).fromMounting(); from = "*"; } else { to = "*"; from = probeDesign.at(pos).toMounting(); } } else { to = probeDesign.at(pos + 1).fromMounting(); from = probeDesign.at(pos).toMounting(); } } else if(pos == probeDesign.length() - 1) { if(probeDesign.at(pos).type() == ProbePart::Type::Stylus) { if(probeDesign.length() <= 1) { from = "*"; to = probeDesign.at(pos).fromMounting(); } else { from = probeDesign.at(pos - 1).toMounting(); to = probeDesign.at(pos).fromMounting(); } } else { from = probeDesign.at(pos).toMounting(); to = "*"; } } else { from = probeDesign.at(pos).toMounting(); to = probeDesign.at(pos + 1).fromMounting(); } return { to, from }; }QStringList andQList tell me that this is a Qt-based application. The goal of this function seems to be to take some inputs about a "probe part" and construct a pair of strings. Let's trace through it.
Let's just walk through the conditions, quickly, without worrying too much about the inside. We look at pos, and check for three cases: either pos is -1, 0, or probeDesign.length() - 1.
Inside each of those branches, we also check the length of the list, testing if it contains no elements, exactly one elemnet, or more than one element. We also check if the part in question is a stylus.
With that in mind, let's see if we can summarize the conditions here. If pos == -1, we do some automatic stuff, using the first element in the list if there is one. If pos == 0 and there's exactly one element in the list, we grab the first element and link it to * (the to/from order depends on the stylus question). If there's more that one element in the list, we pair the current pos with pos+1; notably, in this branch, pos is definitely zero. If pos is the last element in the list, we follow the same logic, but pair with pos-1, with a side branch for checking against the length of the list.
It's all bounds checking. That's all this code is. Bounds checking that's gotten out of hand. The main branch here is actually the final else: that's where most of the code is going to pass through. All the other branches are just handling edge cases. Literal edge cases, as in "the edge of the list".
Untodesu didn't supply the two line version, but based on the fact such a version exists, I also suspect that many of these branches weren't actually used. Or, at least, based on the actual business rules, could be combined.
CodeSOD: Are There Files Yet?
Are there any files to send? That's the question that Chris C's predecessor had. So they asked it. Again. And again. And again.
Chris writes:
I'm occasionally called upon to troubleshoot an ecommerce application that was built in the PHP 5.x days and has been running largely untroubled by maintenance or modernity (aside from the backported security patches to its binaries) ever since.
if(sizeof($files) > 0){ if(sizeof($files) > 0){ foreach($files as $file){ $mime->addAttachment($file); } } }Indentation as per the original.
If the files array contains items, then if the files array contains items, then we iterate across the files array, which hopefully contains items, and add them as an attachment to an email.
I feel like the way this got indented, the developer responsible knew, deep down, that this was wrong. They lacked the reading comprehension to understand why, but deep down in their spleen, something was screaming at them. And thus those stacked curly brackets at the end there.
Of course, none of the conditionals are needed: a foreach on an empty object just does nothing.
Whales Ahoy!
The waters are even more dangerous than we imagined. Have a look at some of the crazed whales our brave submitters and commenters have encountered in the wild.
First comes an Anonymous tale of woe:
Our company makes apps for businesses. We have 1 MAIN client whose CEO can make or break our company, and his wish is our command. He sent a priority email on a Friday night saying the app was slow and needed to be fixed.
The client CEO is so important that he works directly with our CEO, who decided to PM this huge issue.
All weekend, we were trying out tons of different things to optimize this "slow" app that "wasn't loading or refreshing." We deployed the app Monday night after a weekend of unpaid overtime (darn salary). On Tuesday, the account manager made a bug card to officially represent the work we did, and they posted a previously-unseen video of the slowness.
There is a refresh icon that spins when clicked. The video was of the refresh icon, and it was spinning for an extra second after the data loaded (and jumping 2 pixels from padding styling).
That is what was high priority.
I mean, we all hate the system, but sometimes the system is actually there to protect us.
Next, we have Daniel's ongoing peril:
We do digital flyers/circulars/ads. Eight years ago, that meant we got PDFs from retailers and turned them into digital content. One huge retailer (hundreds of stores) wanted a dynamically-created flyer that would have up-to-date pricing twice a day. We didn't have time to build out a full digital solution (which would have made sense), so instead we spent six months banging together a solution with spit and duct tape which baked out hundreds of PDFs every morning and afternoon. This one retailer was responsible for about 40% of our processing power.
We're finally getting somewhat closer to phasing this out, but "it worked" for this long ...
Finally, let's be grateful Brian escaped with his life!
Worked for a company that was building a component of a high-profile weapons platform for one of the major military suppliers. We had taken over the project from another company that was under-performing, so we were already behind schedule from the minute the contract was signed. Of course this company saw fit to treat us more as a subsidiary than a subcontractor. Including, for a time, sending one of their own managers to sit in our lab and observe (read: babysit) us. On Saturdays. Then they demanded we start working shifts to make more use of the lab equipment, and I got the bad draw: 3 AM - noon. Never mind that I had just gotten married (they actually called to tell me this while I was on vacation the week after my wedding) and would like to actually spend some time with my wife ...
That experience soured me on the whole military-industrial complex for a long time. To this day I still get headhunters pinging me to work for that megacorp; I just chuckle and delete their messages.
Have these tales knocked loose any foul memories that your brain tried to repress? Send them to us!
CodeSOD: Classic WTF: One-and-a-Half-Tiered Application Design
There are several types of bad code; there's lazy code, frantic code, unaware-of-a-better-way code, and aware-of-a-better-way-but-too-apathetic-to-do-it code, to name a few. Then there're amalgamations of different types of bad code.
Môshe encountered such an amalgam when his company was trying out a new delivery service. Môshe spent some time evaluating the IE-only web interface, and was curious about some JavaScript errors he was getting. Strangely, he noticed variables named dateSQL, newSQLTag, and modeSQL.
Môshe dug a little deeper, probably thinking that his suspicions couldn't possibly be correct, only to find sendLinkVal() in the page's code:
function sendLinkVal(theDate,theStatus,MainTitle,PageTitle){ var dateSQL = " AND J.JBDeliveryDate=''" + theDate + "''" var status = "" var newSQLTag ="" var PageTitle = PageTitle var MainTitle = MainTitle //alert(dateSQL) switch (theStatus){ case "Confirmed": dateSQL= "" var modeSQL = "" modeSQL = " AND (J.JBCompanyID=31337) " status = " GlobalJobStatusView AS J WHERE J.JBCollectDate='' " + theDate + "'' AND J.JBConfirmed=''Yes'' AND J.MIStatusCode<>5" + modeSQL + " AND (ISNULL(J.JBCancelled, 0) <> 1) ORDER BY Convert(int, J.MIJobID)" break; case "Unconfirmed": dateSQL= "" var modeSQL = "" modeSQL = " AND (J.JBCompanyID=31337) " status = " GlobalJobStatusView AS J WHERE J.JBCollectDate='' " + theDate + "'' AND J.JBConfirmed=''No''" + modeSQL + " ORDER BY Convert(int, J.MIJobID)" break; case "Complete": dateSQL= "" var modeSQL = "" modeSQL = " AND (J.JBCompanyID=31337) " status = " GlobalJobStatusView AS J WHERE J.JBCollectDate='' " + theDate + "'' AND J.MIStatusCode=5" + modeSQL + " ORDER BY Convert(int, J.MIJobID)" break; case "Unconformed": dateSQL= "" var modeSQL = "" modeSQL = " AND (J.JBCompanyID=31337) " status = " GlobalJobStatusView AS J WHERE J.JBCollectDate='' " + theDate + "'' AND (J.MIConformance IS NOT NULL AND J.MIConformance<>'''') " + modeSQL + " ORDER BY Convert(int, J.MIJobID)" break; case "NoDelDate": dateSQL= "" var modeSQL = "" modeSQL = " AND (J.JBCompanyID=31337) " dateSQL =" GlobalJobStatusView AS J WHERE J.JBDeliveryDate IS NULL " + modeSQL + " ORDER BY Convert(int, J.MIJobID) " break; case "Collections": // the dateSQL is not required so set it to nothing so that it // doesn't interfere with the sql being generated at the end of // the function. dateSQL= "" var modeSQL = "" modeSQL = " AND (J.JBCompanyID=31337) " status = " GlobalJobStatusView AS J WHERE J.JBCollectDate='' " + theDate + "''" + modeSQL + " ORDER BY Convert(int, J.MIJobID)" break; case "Deliveries": // the dateSQL is not required so set it to nothing so that it // doesn't interfere with the sql being generated at the end of // the function. dateSQL= "" var modeSQL = "" modeSQL = " AND (J.JBCompanyID=31337) " status = " GlobalJobStatusView AS J WHERE J.JBDeliveryDate='' " + theDate + "''" + modeSQL + " ORDER BY Convert(int, J.MIJobID)" break; case "ColAndDel": // the dateSQL is not required so set it to nothing so that it // doesn't interfere with the sql being generated at the end of // the function. dateSQL= "" var modeSQL = "" modeSQL = " AND (J.JBCompanyID=31337) " status = " GlobalJobStatusView AS J WHERE ((J.JBDeliveryDate='' " + theDate + "'') OR (J.JBCollectDate=''" + theDate + "''))" + modeSQL + " ORDER BY Convert(int, J.MIJobID)" break; case "Subcontractor": // the dateSQL is not required so set it to nothing so that it // doesn't interfere with the sql being generated at the end of // the function. dateSQL= "" var modeSQL = "" modeSQL = " AND (J.JBCompanyID=31337) " status = " JobAndLoadView AS J WHERE (J.JBDeliveryDate='' " + theDate + "'') " + modeSQL + " ORDER BY Convert(int, J.MIJobID)" break; case "Cancelled": // the dateSQL is not required so set it to nothing so that it // doesn't interfere with the sql being generated at the end of // the function. dateSQL= "" var modeSQL = "" modeSQL = " AND (J.JBCompanyID=31337) " status = " GlobalJobStatusView AS J WHERE (J.JBCollectDate=='' " + theDate + "'') " + modeSQL + " AND ISNULL(J.JBCancelled, 0) = 1 ORDER BY Convert(int, J.MIJobID)" break; default : status =""; } newSQLTag = dateSQL + status; document.all.hiddenForm.linkVal.value = newSQLTag; document.all.hiddenForm.PageTitle.value = PageTitle document.all.hiddenForm.MainTitle.value = MainTitle document.all.hiddenForm.submit(); //alert(newSQLTag) }Môshe could replace his customer ID with any other and access customer data, and for that matter, to modify or delete whatever he wanted. He could add or remove columns to tables. He could possibly even change permissions, add his own database user and deny all other users access.
Shocked, Môshe called the delivery service, who got him in touch with the developer of the system. This developer was equally shocked to learn that it was even possible to view a web page's JavaScript code, let alone that his architecture was open to SQL injection attacks from virtually any angle. He took immediate and decisive action; all queries were moved to the .NET backend.
Of course, the queries still didn't use parameters and are therefore still open to SQL injection, but now it takes slightly more effort to hack.
Error'd: April is Special, and so are you
"April is special," writes Elwin. It is, but take heart May, every month is special at TDWTF.
"Admiral Ackbar is pinterested," punned The Beast in Black
Manuel H. clocked something off on this website. "Noon seems to be very late in Lithuania, or maybe only in this hotel restaurant in Vilnius." 15H AM must be on some planet with a 32H day.
"Amazon can't make up its mind!" ranted an anon. "Do I need to wait 2 business days or 3? Make up your mind Amazon!"
Duston decided to close us out with a pun. "Looks like they have a problem, but it's trivial." Well done.
CodeSOD: In the Know
Delilah works in a Python shop. Despite Python's "batteries included" design, that doesn't stop people from trying to make their own batteries from potatoes. For example, her co-worker wrote this function:
def key_exists(element, key): if isinstance(element, dict): try: element = element[key] except KeyError: return False return TruePython, of course, has an in operator. key in dictionary is an extremely common idiom. There's no reason to implement your own. Certainly, there's no reason to re-implement it by catching and throwing exceptions.
This is ugly, stupid, and bad. It gets worse, though, when you see how it gets used.
for key in old_yaml_data: if key in new_yaml_data: if old_yaml_data[key] != new_yaml_data[key]: temp = new_yaml_data[key] new_yaml_data[key] = merge(new_yaml_data[key], old_yaml_data[key]) if key_exists(new_yaml_data[key], 'image') and key_exists(old_yaml_data[key], 'image'): new_yaml_data[key]['image'] = temp['image'] elif key == "databases": revert_db_tags(new_yaml_data[key], temp)This code is attempting to upgrade "old" YAML data with "new" data. So it's basically merging dictionaries, which is a great case for the in operator.
And they use the correct idiom on the second line there! This was written by one developer! They do the standard key in new_yaml_data check. And they also use key_exists. I can only assume that they had a stroke between starting and finishing this script, which I'll note is, in total, 48 lines long.
Here's the whole short script, which is just generally a mess. Slapped together Python code that's trying to be a "smarter" shell script, but is definitely written with the elegance of hacked-together-bash.
import sys import yaml from jsonmerge import merge appHomePath = sys.argv[1] oldValuesYAML = appHomePath + "values.yaml" newValuesYAML = appHomePath + "/upgrade_version/values.yaml" with open(newValuesYAML, 'r') as f: new_yaml_data = yaml.load(f, Loader=yaml.loader.FullLoader) with open(oldValuesYAML, 'r') as f: old_yaml_data = yaml.load(f, Loader=yaml.loader.FullLoader) def key_exists(element, key): if isinstance(element, dict): try: element = element[key] except KeyError: return False return True def revert_db_tags(old_yaml_data, new_yaml_data): dbList = ["mongoDB", "postgresDB"] mongoDbTagsToRevert = ["mongoRestore"] mongodbKeysToDelete = [] postgresDbTagsToRevert = [] for db in dbList: old_yaml_data[db]['image'] = new_yaml_data[db]['image'] for mongoDbTag in mongoDbTagsToRevert: old_yaml_data['mongoDB'][mongoDbTag]['image'] = new_yaml_data['mongoDB'][mongoDbTag]['image'] for mongoDbTag in mongoKeysToDelete: del old_yaml_data['mongoDB'][mongoDbTag] for postgresDbTag in postgresDbTagsToRevert: old_yaml_data['postgresDB'][postgresDbTag]['image'] = new_yaml_data['postgresDB'][postgresDbTag]['image'] for key in old_yaml_data: if key in new_yaml_data: if old_yaml_data[key] != new_yaml_data[key]: temp = new_yaml_data[key] new_yaml_data[key] = merge(new_yaml_data[key], old_yaml_data[key]) if key_exists(new_yaml_data[key], 'image') and key_exists(old_yaml_data[key], 'image'): new_yaml_data[key]['image'] = temp['image'] elif key == "databases": revert_db_tags(new_yaml_data[key], temp) with open(newValuesYAML, 'w') as f: data = yaml.dump(new_yaml_data, f, sort_keys=False) [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!