CodeSOD: Authorized Logger
Gretchen's company recently got purchased by Initech. Specifically, they were bought for their dev team, of all things. They had a few software products that were high performers, and Initech wanted that secret sauce. They bought the company, and then split the dev team up and migrated the developers to new products.
That actually worked out okay for Gretchen, most of the time. For a few projects, the dev team was given some requirements and a free hand to figure out how to deliver them. They were free to reuse code that existed or rewrite entirely, based on their own judgement. They were free to pick the tools they wanted to use, and the results worked out well.
But there were some projects that… were a different story. After those successes, Gretchen got moved onto a project that was 90% firefighting. The app had code like this:
req.body.externalId = !!req.body.externalId ? req.body.externalId + "" : "";How's that for some null handling.
The whole thing can't run on a version of NodeJS newer than 14: a version that last got an update in 2023.
"The code follows no conventions," Gretchen writes, "there's no logging."
exports.create = (req, res) => { logger.debug('creating new staffClient'); logger.debug(req.body) // let staffClient = new StaffClient({}); // // run through and create all fields on the model // for(var k in req.body) { // if(req.body.hasOwnProperty(k)) { // staffClient[k] = req.body[k]; // } // } StaffClient.query().insert(req.body) .returning('*') .then(staffClient => { if(staffClient) { res.send({success: true, staffClient}) } else { res.send({ success: false, message: "Could not save StaffClient"}) } }); }Now, you may say to yourself, "What do you mean there's no logging? I see it right there!" There is a logger utility class, and do you know what it prints when you call logger.debug("some message")? It prints DEBUG.
This code handles an HTTP request, and stuffs the body of the request into the database; here's hoping that it's a well formed request. Somebody's got a lot of faith in their front end. WHat's interesting about this one is they've tried two different ways of copying the request object into the database, the first one focusing on making sure they only copied non-inherited properties, and the second just YOLOing the data into the database.
Now, this particular segment goes through their ORM to write data into the database. But not all the code does that. Many places write data through direct SQL, and guess what happens there: SQL injection vulnerabilities.
You may also notice that this function doesn't do any authorization checks, which is fine, that should be configured in the middleware. Should be- but isn't. Most endpoints have no authorization checks at all. Even the endpoints that do, like their admin API, have copies of the same endpoint with no authentication configured.
.comment { border: none; }CodeSOD: Do a Lot to Do Nothing
Today's anonymous submitter works in finance. I'll let them start the introduction:
This is a legacy application that has been running for nearly a decade in production so one could say that it's been thoroughly tested by daily production use and nothing needs changing
This is a collection of two C# methods, and we'll start with ValueAGPFund, which isn't a WTF per se, but definitely not code I'd want to maintain either.
public Valuation ValueAGPFund(int valuationId, ValueAFundParameters parameters, CapitalAccount capitalAccount, int? lotId) { if (parameters.UseActiveCoefficientSet) { parameters.CoefficientSet = _coefficientSetQueries.GetActive(); } parameters.InternationalDveCoefficientSets = _coefficientSetQueries.GetInternationalDveActive(); var referenceData = _referenceDataFactory.CreateReferenceData(parameters, capitalAccount); if (lotId != null) { var di = referenceData.FundDirectInvestments.Where(x => x.PositionId == lotId); referenceData.FundDirectInvestments = di; } var countryMappings = _countryQueries.GetFullIsoCountryList(); var valuation = _valuationFactory.Initialise(referenceData, parameters, countryMappings); valuation = ApplyValuators(valuation, referenceData, _valuatorFactory.CreateValuators(valuation, this)); var valuationForCoverage = _valuationQueries.GetWithDirectValuationsAndFundValuations(valuationId); valuation = ApplyCoverage(valuation, valuationForCoverage); foreach (var fv in valuation.FundValuations) { _logger.Info($"Debugging distributions: for fund (parameter fund id = {parameters.FundId}, valuation fund id = {valuation.FundId}, fund valuation fund id = {fv.GpFundId}) in valuation {valuationId}," + $" loaded fund investment distributions from {string.Join(", ", fv.FundInvestmentDistributions.Select(x => $"{x.InvestmentId}:{x.TransactionDate:yyyy/MM/dd}"))}"); } foreach (var fv in valuation.FundValuations.Where(x => parameters.InvestmentIds.Contains(x.EqtInvestmentId))) { fv.ValuationId = valuationId; _fundValuationCommands.Add(fv); } foreach (var dv in valuation.DirectValuations.Where(x => x.LotIdDiOnly == lotId)) { dv.ValuationId = valuationId; _directValuationCommands.Add(dv); } foreach (var vw in valuation.ValuationWarnings) { vw.ValuationId = valuationId; _valuationWarningCommands.Add(vw); } var previousValuation = CheckPreviousValuationIfRequired(valuationId, parameters, capitalAccount, lotId); if (previousValuation != null) valuation.ChildValuations.Add(previousValuation); if (parameters.Frequency == ValuationFrequency.Daily) { var unapprovedValuations = _valuationQueries.GetList(valuation.FundId, valuation.ValuationDate, valuation.Frequency, valuation.Purpose) .Where(x => x.IsApproved == ValuationStatus.Unapproved) .ToList(); _valuationCommands.Delete(unapprovedValuations.Select(x => x.Id).ToArray()); } valuation.Id = valuationId; _valuationCommands.Update(valuation); _valuationCacheService.Refresh(valuation.Frequency, true); return valuation; }The key problem with this function is that it's got loads of side effects. It modifies the parameters parameter, which while it was passed by value, the value itself is a reference, so you are updating it on the caller, whether the caller likes it or not. It also modifies a bunch of internal class members. It's also just… doing a lot of different steps. It's not a WTF, but it's bad code. Note the call in the middle to CheckPreviousValuationIfRequired- we're going to come back to that in a second.
Let's take a look at how it's called.
private Valuation CheckPreviousValuationIfRequired(int valuationId, ValueAFundParameters parameters, CapitalAccount capitalAccount, int? lotId) { if ((parameters.Frequency == ValuationFrequency.Quarterly || parameters.Frequency == ValuationFrequency.Monthly) && ValuationPurposeHelper.UserGenerated(parameters.Frequency).Contains(parameters.Purpose)) { var inPeriodParams = new ValueAFundParameters { FundId = parameters.FundId, ValuationDate = parameters.ValuationDate.GetPreviousValuationDate(parameters.Frequency), CreatedBy = parameters.CreatedBy, Purpose = ValuationPurpose.InPeriodCalculation, Frequency = parameters.Frequency, InvestmentIds = parameters.InvestmentIds, UseActiveCoefficientSet = true, UseAmericanDve = parameters.UseAmericanDve, ValuationOptions = parameters.ValuationOptions }; var openingValuation = _valuationQueries.GetInPeriodOpeningValuation(inPeriodParams.FundId, inPeriodParams.ValuationDate, valuationId); //return openingValuation == null // ? null // : ValueAGPFund(openingValuation.Id, inPeriodParams, capitalAccount, lotId); return openingValuation == null ? ValueAGPFund(openingValuation.Id, inPeriodParams, capitalAccount, lotId) : null; } return null; }This function checks the input parameters. Depending on the values, it will either return null, or it will call ValueAGPFund. Wait a second, ValueAGPFund calls this function. That's not good.
But let's really focus in on the return statement and its comment:
//return openingValuation == null // ? null // : ValueAGPFund(openingValuation.Id, inPeriodParams, capitalAccount, lotId); return openingValuation == null ? ValueAGPFund(openingValuation.Id, inPeriodParams, capitalAccount, lotId) : null;The current version checks if openingValuation is null, and if it is, tries to access it, thus triggering a NullReferenceException. This function either returns null or throws a NullReferenceException. So all that worrying about side effects and circular calls doesn't matter, but this likely isn't correct. The comment indicates that there used to be a correct version, which only called ValueAGPFund if the valuation wasn't null- but that version likely had all the problems of circular calls and unpredictable side effects.
As it stands, the application as a whole works. Since CheckPreviousValuationIfRequired only ever returns null or throws an exception, and since ValueAGPFund is only called from here, it looks like these functions could just both be removed without problems. But our submitter is wary of doing that:
The problem is that I first need to figure out whether 1) this piece of code produces any side effects and 2) nobody is relying on the System.NullReferenceException being thrown here.
No worries, though, right? I'm sure your unit tests will catch any regressions caused by removing that. Because this is the kind of code that definitely has great unit tests.
.comment { border: none; }CodeSOD: When False is True
Lillith was integrating some new tools into an existing Ruby on Rails API. The existing API allowed you to send a dry_run flag along with the request, so that you could have the service calculate its changes without applying them.
The problem was, the new tool Lillith was integrating could send, in the body of the request, {"dry_run": false}, but the service would see it as true. Consistently.
The helper method which checked for "true" parameters looked like this:
def param_true?(param_name) param_value = params[param_name] params.key?(param_name) && (!param_value || param_value.to_s.downcase == 'true') endThe purpose of this function is to handle stringy or nil inputs gracefully. And there's one thing I can say about the function: it will always identify a true value correctly. If your false value is a string, "false", it also works. But that pesky !param_value mean that any actual boolean false value will be seen as true.
This function has been in wide use through the application. Lillith's best guess is that up to this point, no one had set the dry run flag on anything but GET requests, where everything was strings. On POST/PATCH/PUT requests, where the data was passed in the body as JSON, it got parsed into actual boolean values, and thus failed.
That's the WTF, certainly, that this function was lurking and waiting to cause this confusion. But the annoying thing in this function is that it fetches the value from the associative array, then calls params.key? to see if the key exists. That's fine, since Ruby just returns a nil if a key doesn't exist, it's just annoying. I hate to see it. This is, admittedly, more of a "me" problem, but I hate it.
Error'd: Microbits
This week we have got a couple of Mathanon's. Maybe they're the same person, maybe they're not, there's really no way to know!
Frist anon has a "Numeric fun fact" for us: "Got a form sent from work to express interest in some event. They actually enforced the validation that the answer must be a number, so I submitted "42"." Bravo.
Next anon has a different numeric fun factor: " The SAS website wants us to know the size of the file behind the link down to the nanobyte precision." They split the bit! That must be what this quantum computing thing is about.
Conscientious dad Mark R. takes all the responsibilities. "My kid's school ensures they're legally covered on all things said and unsaid."
Philipp H. points out "The Redmond philosophers have created something the old Greek philosophers will have to rethink. Or is this a pun on Schrödinger's Cat? German→English translation: "We cannot bring/transfer/switch you to this message because you're in a chat, in which you're not in.""
We haven't heard from Michael R. in a while. Here he is with a pithy "The irony is not lost on me."
Happy Juneteenth to those who celebrate.
[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!
Representative Line: Sort This Out
Today's anonymous submitter has spent a long time toiling through many, many tickets. Their effort has been an attempt to "save" their employer from the disaster left behind by by a highly-paid consultant. As one does, our submitter started with the highest priority tickets with the highest severity. Eventually, they whittled down that list, and had some bandwidth to start looking at the pieces of the code which clearly weren't exploding right now (because there were no tickets), but were likely to explode at some point in the future (creating a storm of tickets).
Scanning through the JavaScript, our submitter found a sort function. That was automatically concerning- why was that particular wheel being reinvented?
The first line of the sort function was this:
obj[x._id.account_id] = x.count_totalIn this case, x._id is meant to be the unique identifier from their Mongo DB. That, uh, should be not precisely a UUID (Mongo does its own weird version), but it definitely shouldn't have an account_id field on it. They are storing an arbitrary object as their unique identifier in the database. Which, I'm no Mongo expert, but I don't need to be Flash Gordon to know that's a bad idea.
But setting aside the choice of using random objects as unique identifiers, there's also the other question: how is this furthering the goal of sorting? Why on Earth am I building an object in the form: {"id0": 5, "id1": 7, "id2": 11}? Or am I even doing that? This is the first line of the function, so we're not even doing a loop, it's just {"id0": 5}.
This isn't just an unexploded bomb, it's a mystery: the primary mystery being why hasn't this exploded already? The second mystery is: what's going to happen when your luck runs out?
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.