We like trains here at Error'd, and you all seem to like trains too. That must be the main reason we get so many submissions about broken information systems.

"Pass," said Jozsef . I think that train might have crashed already.

 

An anonymous subscriber shared an epic tale some time ago. They explained thus. "(I couldn't capture in the photo, but the next station after Duivendrecht was showing the time of 09:24+1.) We know Europe has pretty good trains, and even some high-speed lines. But this was the first time I boarded a time-traveling train. At first I was annoyed to be 47 minutes late. I thought I could easily walk from Amsterdam Centraal to Muiderpoort in less than the 53 minutes that this train would take. But I was relieved to know the trip to the further stations was going to be quicker, and I would arrive there even before arriving at the earlier stations."
I think the explanation here is that this train is currently expected to arrive at Muiderport around 10:01. But it's still projected to arrive at the following stop at 9:46, and more surprisingly at the successive stops at 9:35 and 9:25.

 

Railfan Richard B. recently shared "Points failure on the West Coast Main Line has disrupted the linear nature of time."

 

and quite some time ago, he also sent us this snap, singing "That train that's bound for glory? It runs through here."

 

An unrelated David B. wonders "When is the next train? We don't know, it's running incognito."

 

And finally, courageous Ivan got sideways underground. "Copenhagen subway system may have fully automated trains, but their informational screens need a little manual help every now and then."

 

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

There are real advantages to taking a functional programming approach to expressing problems. Well, some problems, anyway.

Kevin sends us this example of elegant, beautiful functional code in C#:

//create a range of dates List<DateTime> dates = Enumerable.Range (0, 1 + settings.EndDate.Subtract (settings.BeginDate).Days).Select (offset => settings.BeginDate.AddDays(offset)).ToList(); foreach (DateTime procDate in dates) { /*.snip.*/ }

If you're not sure what this code does, it's okay- Kevin rewrote it and "ruined" it:

DateTime procDate = settings.BeginDate; while(procDate <= settings.EndDate) { /*.snip.*/ procDate= procDate.AddDays(1); }

The goal of this code is simply to do something for every day within a range of dates. These two approaches vary a bit in terms of readability though.

I guess the loop in the functional version isn't mutating anything, I suppose. But honestly, I'm surprised that this didn't take the extra step of using the .ForEach function (which takes a lambda and applies it to each parameter). Heck, with that approach, they could have done this whole thing in a single statement.

[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.

Greg was fighting with an academic CMS, and discovered that a file called write_helper.js was included on every page. It contained this single function:

function document_write(s) { document.write(s); }

Now, setting aside the fact that document.write is one of those "you probably shouldn't use this" functions, and is deprecated, one has to wonder what the point of this function is. Did someone really not like object-oriented style code? Did someone break the "." on their keyboard and just wanted to not have to copy/paste existing "."s?

It's the kind of function you expect to see that someone wrote but that isn't invoked anywhere, and you'd almost be correct. This function, in a file included on every page, is called once and only once.

More like the wrong helper, if we're being honest.

[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.

Gretchen saw this line in the front-end code for their website and freaked out:

let bucket = new AWS.S3({ params: { Bucket: 'initech-logos' } });

This appeared to be creating an object to interact with an Amazon S3 bucket on the client side. Which implied that tokens for interacting with S3 were available to anyone with a web browser.

Fortunately, Gretchen quickly realized that this line was commented out. They were not hosting publicly available admin credentials on their website anymore.

.comment { border: none; }

They used to, however, and the comments in the code made this a bit more clear:

// inside an angular component: uploadImage(): void { const uniqueName = `${this.utils.generateUUID()}_${this.encrDecSrvc.getObject(AppConstants.companyID)}_${this.file.name}` /*; @note: Disable usage of aws credential, transfer flow to the backend. @note; @disable-aws-credential */ /*; AWS.config.region = 'us-east-1' let bucket = new AWS.S3({ params: { Bucket: 'initech-billinglogos' } }); */ const bucket = ( AWSBucketMask ); const params = { Bucket: 'initech-logos', Key: 'userprofilepic/' + uniqueName, ACL: "public-read", Body: this.file }; const self = this; bucket.upload( params, function (err, data) { if (err) { console.log("error while saving file on s3 server", err); return; } self.isImageUrl = true; self.imageUrl = data.Location; self.myProfileForm.controls['ProfilePic'].setValue(self.imageUrl); self.encrDecSrvc.addObject(AppConstants.imageUrl, self.imageUrl); self.initechAPISrvc.fireImageView(true); self.saveProfileData(); self.fileUpload.clear() }, self.APISrvc ); }

Boy, this makes me wonder what that AWSBucketMask object is, and what its upload function does.

export class AWSBucketMask { public static async upload( option, callback, service ){ const fileReader = new FileReader( ); fileReader.onloadend = ( ( ) => { const dataURI = ( `${ fileReader.result }` ); const [ entityType, mimeType, baseType, data ] = ( dataURI.split( /[\:\;\,]/ ) ); option.ContentEncoding = baseType; option.ContentType = mimeType; option.Body = data; service.awsBucketMaskUpload( option ) .subscribe( function( responseData ){ callback( null, responseData.data ); }, function( error ){ callback( error ); } ); } ); fileReader.readAsDataURL( option.Body ); } public static async deleteObject( option, callback, service ){ service.awsBucketMaskDeleteObject( option ) .subscribe( function( responseData ){ callback( null, responseData ); }, function( error ){ callback( error ); } ); } public static async deleteObjects( option, callback, service ){ service.awsBucketMaskDeleteObjects( option ) .subscribe( function( responseData ){ callback( null, responseData ); }, function( error ){ callback( error ); } ); } public static async getSignedUrl( namespace, option, callback, service ){ service.awsBucketMaskGetSignedUrl( namespace, option ) .subscribe( function( responseData ){ callback(null, responseData.data); }, function( error ){ callback( error ); } ); } }

The important thing to notice here is that each of the methods here invokes a web service service.awsBucketMaskUpload, for example. Given that nothing actually checks their return values and it's all handled through callback hell, this is a clear example of async pollution- methods being marked async without understanding what async is supposed to do.

But that's not the real WTF. You may notice that these calls back to the webservice are pretty thin. You see, here's the problem: originally, they just bundled the S3 into the client-side, so the client-side code could do basically anything it wanted to in S3. Adding a service to "mask" that behavior would have potentially meant doing a lot of refactoring, so instead they made the service just a dumb proxy. Anything you want to do on S3, the service does for you. It does no authentication. It does no authorization. It runs with the admin keys, so if you can imagine a request you want to send it, you can send it that request. But at least the client doesn't have access to the admin keys any more.

This is an accounting application, so some of the things stored in S3 are confidential financial information.

Gretchen writes:

We have to take cybersecurity courses every 3 months, but it seems like this has no effect on the capabilities of my fellow coworkers.

You can lead a programmer to education, but you can't make them think.

[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!

One thing I've learned by going through our reader submissions over the years is that WTFs never start with just one mistake. They're a compounding sequence of systemic failures. When we have a "bad boss" story, where an incompetent bully puts an equally incompetent sycophant in charge of a project, it's never just about the bad boss- it's about the system that put the bad boss in that position. For every "Brillant" programmer, there's a whole slew of checkpoints which should have stopped them before they went too far.

With all that in mind, today we're doing a news roundup about the worst boss of them all, the avatar of Dunning-Kruger, Elon Musk. Because over the past month, a lot has happened, and there are enough software and IT related WTFs that I need to talk about them.

For those who haven't been paying attention, President Trump assembled a new task force called the "Department of Government Efficiency", aka "DOGE". Like all terrible organizations, its mandate is unclear, its scope is unspecified, and its power to execute is unbounded.

Now, before we get into it, we have to talk about the name. Like so much of Musk's persona, it's an unfunny joke. In this case, just a reference to Dogecoin, a meme currency based on a meme image that Musk has "invested" in. This is part of a pattern of unfunny jokes, like strolling around Twitter headquarters with a sink, or getting your product lines to spell S3XY. This has nothing to do with the news roundup, I just suspect that Musk's super-villain origin story was getting booed off the stage at a standup open-mic night and then he got roasted by the emcee. Everything else he's ever done has been an attempt to convince the world that he's cool and popular and funny.

On of the core activities at DOGE is to be a "woodchipper", as Musk puts it. Agencies Musk doesn't like are just turned off, like USAID.

The United States Agency for International Development handles all of the US foreign aid. Now, there's certainly room for debate over how, why, and how much aid the US provides abroad, and that's a great discussion that I wouldn't have here. But there's a very practical consideration beyond the "should/should not" debate: people currently depend on it.

Farmers in the US depend on USAID purchasing excess crops to stabilize food prices. Abroad, people will die without the support they've been receiving.

Even if you think aid should be ended entirely, simply turning off the machine while people are using it will cause massive harm. But none of this should come as a surprise, because Musk loves to promote his "algorithm".

Calling it an "algorithm" is just a way to make it sound smarter than it is; what Musk's "algorithm" really is is a 5-step plan of bumper-sticker business speak that ranges from fatuous to incompetent, and not even the fawning coverage in the article I linked can truly disguise it.

For example, step 1 is "question every requirement", which is obvious- of course, if you're trying to make this more efficient, you should question the requirements. As a sub-head on that, though, Musk says that requirements should be traceable directly to individuals, not departments. On one hand, this could be good for accountability, but on the other, any sufficiently complex system is going to have requirements that have to be built through collaboration, where any individual claiming the requirement is really just doing so to be a point of accountability.

Step 2, also has a blindingly obvious label: "delete any part of the process you can". Oh, very good, why didn't I think of that! But Musk has a "unique" way of figuring out what parts of the process can be deleted: "You may have to add them back later. In fact, if you do not end up adding back at least 10 percent of them, then you didn’t delete enough."

Or, to put it less charitably: break things, and then unbreak them when you realize what you broke, if you do.

We can see how this plays out in practice, because Musk played this game when he took over Twitter. And sure, it's revenue has collapsed, but we don't care about that here. What we care about are stupid IT stories, like the new owner renting a U-Haul and hiring a bunch of gig workers to manually decommission an expensive data center. Among the parts of the process Musk deleted were:

• Shutting down the servers in an orderly fashion
• Using the proper tools to uninstall the server racks
• Protecting the flooring which wasn't designed to roll 2,500lb server racks
• Not wiping the hard drives which contained user data and proprietary information
• Not securing that valuable data with anything more than a set of Home Depot padlocks and Apple AirTags

And, shockingly, despite thinking this was successful in the moment, the resulting instability caused by just ripping a datacenter out led Musk to admit this was a mistake.

So let's take a look at how this plays out with DOGE. One of the major efforts was taking over the Treasury Department's IT systems. These are systems which handle $5 trillion in payments every year. And who do we put in change? Some random wet-behind-the-ears dev with a history of racist posts on the Internet.

Ostensibly, they were there to "audit" payments, so was their access read only? Did they have admin access? Were they actually given write access? Could they change code? Nobody is entirely certain. Even if it was only read-only, there are plenty of questions about what kind of security risk that constitutes, which means forensic analysis to understand the breach, which is being called the largest data breach in history.

Part of the goal was to just stop payments, following the Muskian "Break things first, and unbreak them if it was a mistake," optimization strategy. Stop paying people, and if you find out you needed to pay them, then start paying them again. Step 2 of the "algorithm".

Speaking of payments, many people in the US depend on payments from the Social Security Administration. This organization, founded in 1935 as part of the New Deal, handles all sorts of benefits, including retirement benefits. According to Musk, it's absolutely riddled with fraud.

What are his arguments? Well, for starters, he worries that SSNs are not de-duplicated- that is, the same SSN could appear multiple times in the database.

Social Security Administration has, since the 1940s, been trying to argue against using SSNs as identifiers for any purpose other than Social Security. They have a history page which is a delightful read as a "we can't say the Executive Orders and laws passed which expanded the use of SSNs into domains where they shouldn't have been used was a bad idea, but we can be real salty about it," document. It's passive-aggression perfected. But you and I already know you should never expect SSNs to be a key.

Also, assuming the SSA systems are mainframe systems, using flat file databases, we would expect a large degree of denormalization. Seeing "unique" keys repeated in the dataset is normal.

On the same subject, Musk has decided that people over 150 years old are collecting Social Security benefits. Now, one could assume that this kind of erroneous data pattern is fraud, or we could wonder if there's an underlying reason to the pattern.

Now, I've seen a lot of discussion on the Internet about this being an epoch related thing, which is certainly possible, but I think the idea that it's related to ISO8601 is obviously false- ISO8601 is just a string representation of dates, and also was standardized well after COBOL and well after SSA started computerizing. Because the number 150 was used, some folks have noted that would be 1875, and have suspected that the date of the Metre Convention is the epoch.

I can't find any evidence that any of this is true, mind you, but we're also reacting to a tweet by a notorious moron, and I have to wonder: did he maybe round off 5 years? Because 1870 is exactly 65 years before 1935- the year Social Security started, and 65 years is the retirement age where you can start collecting Social Security. Thus, the oldest date which the SSA would ever care about was 1870. Though, there's another completely un-epoch related reason why you could have Social Security accounts well older than 150 years: your benefits can continue to be paid out to your spouse and dependents after your death. If an 80 year old marries a 20 year old, and dies the next day, that 20 year old could collect benefits on that account.

The key point I'm making is that "FRAUUUDDDD!!1111!!!" is really not the correct reaction to a system you don't understand. And while there may be many better ways to handle dates within the SSA's software, the system predates computers and has needed to maintain its ability to pay benefits for 90 years. While you could certainly make improvements, what you can't do is take a big "algorithm" Number 2 all over it.

Which, with that in mind, the idea that these people are trying to get access to a whole slew of confidential taxpayer information is I'm sure going to go *great.

There are so, so many more things that could be discussed here, but let's close with the DOGE website. Given that DOGE operates by walking into government agencies and threatening to call Elon, there are some serious concerns over transparency. Who is doing what, when, why and with what authority? The solution? Repost a bunch of tweets to a website with a .gov domain name.

Which, you'd think that spinning up a website that's just that would be easy. Trivially easy. "Security issues" shouldn't even be part of the conversation. But in actuality, the database was unsecured and anyone could modify the site.

In the end, the hacked website is really just Elon Musk's "algorithm" improved: instead of breaking things that are already working, you just start with a broken website.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

Because we still have the NWS, I learned that "A winter storm will continue to bring areas of heavy snow and ice from the Great Lakes through New England today into tonight." I'm staying put, and apparently so is Dave L.'s delivery driver.

Dave L. imagines the thoughts of this driver who clearly turned around and headed straight home. "Oh, d'ya mean I've got to take these parcels somewhere!? in this weather!? I can't just bring them back?"

 

Infoscavenger Andrew G. shared a found object. "I got this from https://bsky.app/profile/jesseberney.com/post/3lhyiubtay22r and immediately thought of you. Sadly I expect you're going to get more of these in the coming days/weeks." I guess they already fired all people who knew how to use Mail Merge.

 

Bruce R. "I saw this ad in my Android weather app. They think they know all about you, but they got this completely wrong: I don't live in {city}, I live in [city]!" Right next to Mr. [EmployeeLastName].

 

"I've got a vintage k8s cluster if anyone's interested," reports Mike S. "I just installed docker desktop. Apparently it ships with the zeroeth version of kubernetes."

 

Finally for this week, special character Vitra has sent us an elliptical statement about her uni application. "Of course, putting characters in my personal statement is simply a bad idea - it's best to let the Universities have the thrill of mystery! (From UCAS, the main university application thing in the UK too!) "

 

Keep warm. [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!

Abdoullah sends us this little blob of C#, which maybe isn't a full-on WTF, but certainly made me chuckle.

if (file!= null) { if (file.name.StartsWith(userName)) { if (file.name.StartsWith(userName)) { url = string.Format(FILE_LINK, file.itemId, file.name); break; } } }

Are you sure the file name starts with the user name? Are you really sure?

This code somehow slipped by code review, helped perhaps by the fact that the author was the senior-most team member and everyone assumed they were immune to these kinds of mistakes.

No one is immune.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

Eric writes:

Yes, we actually do have code reviews and testing practices. A version of this code was tested successfully prior to this version being merged in, somehow.

Well, that's ominous. Let's look at the code.

public static SqsClient create() { try { SqsClient sqsClient = SqsClient.builder() ... .build(); return sqsClient; } catch (Exception e) { log.error("SQS - exception creating sqs client", e); } finally { // Uncomment this to test the sqs in a test environment // return SqsClient.builder(). ... .build(); return null; } }

Eric found this when he discovered that the application wasn't sending messages to their queue. According to the logs, there were messages to send, they just weren't being sent.

Eric made the mistake of looking for log messages around sending messages, when instead he should have been looking at module startup, where the error message above appeared.

This code attempts to create a connection, and if it fails for any reason, it logs an error and returns null. With a delightful "comment this out" for running in the test environment, which, please, god no. Don't do configuration management by commenting out lines of code. Honestly, that's the worst thing in this code, to me.

In any case, the calling code "properly" handled nulls by just disabling sending to the queue silently, which made this harder to debug than it needed to be.

.comment { border: none; } [Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

As the saying goes, there are only two hard problems in computer science: naming things, cache invalidations, and off by one errors. Chris's predecessor decided to tackle the second one, mostly by accurately(?) naming a class:

class SimpleCache { }

This is, in fact, the simplest cache class I can imagine. Arguably, it's a bit too simple.

Instances of this class abound in code, though no one is entirely sure why. Future optimization? Just no one understanding what they're doing? Oh right, it's that one. It's always no one understanding what they're doing.

[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.

Andrew worked with Stuart. Stuart was one of those developers who didn't talk to anyone except to complain about how stupid management was, or how stupid the other developers were. Stuart was also the kind of person who would suddenly go on a tear, write three thousand lines of code in an evening, and then submit an pull request. He wouldn't respond to PR comments, however, and just wait until management needed the feature merged badly enough that someone said, "just approve it so we can move on."

.comment {border: none;} int iDisplayFlags = objectProps.DisplayInfo.BackgroundPrintFlags; bool bForceBackgroundOn = false; bool bForceBackgroundOff = false; // Can't use _displayTypeID because it will always be 21 since text displays as image if (_fileTypeID == 11) // TEXT { if ((iDisplayFlags & 0x1008) != 0) // Text Background is required { bForceBackgroundOn = true; } else if ((iDisplayFlags & 0x1001) != 0) // Text Background is not available { bForceBackgroundOff = true; } } else if (_displayTypeID == 21) // IMAGE { if ((iDisplayFlags & 0x1200) != 0) // Image Background is required { bForceBackgroundOn = true; } else if ((iDisplayFlags & 0x1040) != 0) // Image Background is not available { bForceBackgroundOff = true; } } bool useBackground = bForceBackgroundOn; // If an object does not have an Background and we try to use it, bad things happen. // So we check to see if we really have an Background, if not we don't want to try and use it if (!useBackground && objectProps.DisplayInfo.Background) { useBackground = Convert.ToBoolean(BackgroundShown); } if (bForceBackgroundOff) { useBackground = false; }

This code is inside of a document viewer application. As you might gather from skimming it, the viewer will display text (as an image) or images (as an image) and may or may not display a background as part of it.

This code, of course, uses a bunch of magic numbers and bitwise operators, which is always fun. We don't need any constants. It's important to note that all the other developers on the project did use enumerations and constants. The values were defined and well organized in the code- Stuart simply chose not to use them.

You'll note that there's some comments and confusion about how we can't use _displayTypeID because text always displays as an image. I'm going to let Andrew explain this:

The client this code exists in renders text documents to images (for reasons that aren’t relevant) when presenting them to the user. We have a multitude of filetypes that we do similar actions with, and fileTypes are user configurable. Because of this, we also keep track of the display type. This allows the user to configure a multitude of filetypes, and depending on the display type configured for the file type, we know if we can show it in our viewer. In the case of display type ‘text’ our viewer ultimately renders the text as an image. At some point in time Stuart decided that since the final product of a text document is an image, we should convert display type text over to image when referencing it in code (hence the comment ‘Can’t use display type ID’). If none of this paragraph makes any sense to you, then you’re not alone, because the second someone competent got wind of this, they thankfully nixed the idea and display type text, went back to meaning display type text (aka this goes through OUR TEXT RENDERER).

What I get from that paragraph is that none of this makes sense, but it's all Stuart's fault.

What makes this special is that the developer is writing code to control a binary status: "do we show a background or not?", but needs two booleans to handle this case. We have a bForceBackgroundOn and a bForceBackgroundOff.

So, tracing through, if we're text and any of the bits 0x1008 are set in iDisplayFlags, we want the background on. Otherwise, if any of the bits 0x1001 are set, we want to force the background off. If it's an image, we do the same thing, though for 0x1200 and 0x1040 respectively.

Then, we stuff bForceBackgroundOn into a different variable, useBackground. If that is false and a different property flag is set, we'll check the value of BackgroundShown- which we choose to convert to boolean which implies that it isn't a boolean, which raises its own questions, except it actually is a boolean value, and Stuart just didn't understand how to deal with a nullable boolean. Finally, after all this work, we check the bForceBackgroundOff value, and if that's true, we set useBackground to false.

I'll be frank, none of this quite makes sense to me, and I can certainly imagine a world where the convoluted process of having a "on" and "forceOff" variable actually makes sense, so I'd almost think this code isn't that bad- except for this little detail, from Andrew:

The final coup de grace is that all of the twisted logic for determining if the background is needed is completely unnecessary. When the call to retrieve the file to display is made, another method checks to see if the background was requested (useBackground), and performs the same logic check (albeit in a sane manner) as above.

The code is confusing and unnecessary.

[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.

I have a feeling we're going to be seeing a lot of AI WTFerry at this site for a while, and fewer stupid online sales copy booboos. For today, here we go:

Jet-setter Stewart wants to sell a pound, but he's going to have to cover some ground first. "Looks like Google are trying very hard to encourage me to stop using their search engine. Perhaps they want me to use chatGPT? I just can't fathom how it got this so wrong."

 

Tim R. proves that AIs aren't immune to the general flubstitution error category either. "I'm not quite sure what's going on here - there were 5 categories each with the same [insert content here] placeholder. Maybe the outer text is not AI generated and the developers forgot to actually call the AI, or maybe the AI has been trained on so much placeholder source code it thought it was generating what I wanted to see."

 

"Crazy Comcast Calendar Corruption!" complains B.J.H. "No wonder I didn't get birthday gifts -- my birth month has been sloughed away. But they still charged me for the months that don't exist." Hey, they only charged you for 12 months at least. Maybe they just picked twelve at random.

 

Educator Manuel H. "Publishing a session recording in [open-source] BigBlueButton seems to be a task for logicians: Should it be public, or protected, or both? Or should it rather be published instead of public? Or better not published at all?" A little translation explanation: the list of options provided would in English be "Public/Protected, Public, Protected, Published, Unpublished". I have no idea what the differences mean.

 

And the pièce de résistance from Mark Whybird "I've always hated click here as a UX antipattern, but Dell have managed to make it even worse." Or maybe better? This is hysterical.

 

[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.

Sammy's company "jumped on the Ruby on Rails bandwagon since there was one on which to jump", and are still very much a Rails shop. The company has been around for thirty years, and in that time has seen plenty of ups and downs. During one of those "ups", management decided they needed to scale up, both in terms of staffing and in terms of client base- so they hired an offshore team to promote international business and add to their staffing.

A "down" followed not long after, and the offshore team was disbanded. So Sammy inherited the code.

I know I'm generally negative on ORM systems, and that includes Rails, but I want to stress: they're fine if you stay on the happy path. If your data access patterns are simple (which most applications are just basic CRUD!) there's nothing wrong with using an ORM. But if you're doing that, you need to use the ORM. Which is not what the offshore team did. For example:

class Request < ActiveRecord::Base def self.get_this_years_request_ids(facility_id) # There are several other methods that are *exactly* the same, except for the year requests = Request.where("requests.id in (select t.id from requests as t # what is the purpose of this subquery? where t.unit_id=token_requests.unit_id and t.facility_id=token_requests.facility_id and t.survey_type = '#{TokenRequest::SURVEY_TYPE}' # why is SURVEY_TYPE a constant? and EXTRACT( YEAR FROM created_at) = EXTRACT(YEAR FROM current_timestamp) order by t.id desc) and token_requests.facility_id = #{facility_id.to_i} # so we get all the requests by year, then by by ??? and token_requests.survey_type = '#{Request::SURVEY_TYPE}'")

Comments from Sammy.

Now, if we just look at the signature of the method, it seems like this should be a pretty straightforward query: get all of the request IDs for a given facility ID, within a certain time range.

And Sammy has helpfully provided a version of this code which does the same thing, but in a more "using the tools correctly" way:

def self.request_ids_for_year(facility_id,year = Time.now.year) token_requests = TokenRequest.where( :facility_id=>facility_id, :survey_type=>TokenRequest::SURVEY_TYPE, :created_at=>(DateTime.new(year.to_i).beginning_of_year .. DateTime.new(year.to_i).end_of_year))

Now, I don't know Ruby well enough to be sure, but the DateTime.new(year.to_i) whiffs a bit of some clumsy date handling, but that may be a perfectly cromulent idiom in Ruby. But this code is pretty clear about what it's doing: finding request objects for a given facility within a given year. Why one uses Request and the other uses TokenRequest is a mystery to me- I' m going to suspect some bad normalization in the database or errors in how Sammy anonymized the code. That's neither here nor there.

Once we've gotten our list of requests, we need to process them to output them. Here's how the offshore code converted the list into a comma delimited string, wrapped in parentheses.

string_token_request_ids = "(-1)" if token_requests && token_requests.length > 0 for token_request in token_requests if string_token_request_ids != "" string_token_request_ids = string_token_request_ids + "," end string_token_request_ids = string_token_request_ids + token_request.id.to_s end string_token_request_ids = "(" + string_token_request_ids + ")" end end end

Look, if the problem is to "join a string with delimiters" and you write code that looks like this, just delete your hard drive and start over. You need extra help.

We start by defaulting to (-1) which is presumably a "no results" indicator. But if we have results, we'll iterate across those results. If our result string is non-empty (which it definitely is non-empty), we append a comma (giving us (-1),). Then we append the current token ID, giving us (-1),5, for example. Once we've exhausted all the returned IDs, we wrap the whole thing in parentheses.

So, this code is wrong- it's only supposed to return (-1) when there are no results, but as written, it embeds that in the results. Presumably the consuming code is able to handle that error gracefully, since the entire project works.

Sammy provides us a more idiomatic (and readable) version of the code which also works correctly:

return "(#{token_requests.count > 0 ? token_requests.map(&:id).join(',') : '(-1)'})"

I'll be honest, I hate the fact that this is returning a stringly-typed list of integers, but since I don't know the context, I'll let that slide. At the very least, this is a better example of what joining a list of values into a string should look like.

Sammy writes:

It seems these devs never took the time to learn the language. After asking around a bit, I found out they all came from a Java background. Most of this code seems to be from a VB playbook, though.

That's a huge and undeserved insult to Visual Basic programmers, Sammy. Even they're not that bad.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

Tim has been working on a large C++ project which has been around for many, many years. It's a tool built for, in Tim's words, "an esoteric field", and most of the developers over the past 30 years have been PhD students.

This particular representative line is present with its original whitespace, and the original variable names. It has been in the code base since 2010.

Assignment::Ptr ra = Assignment::makeAssignment(I, addr, func, block, RA);

The extra bonus is that Assignment::Ptr is actually an alias for boost::shared_ptr<Assignment>. As you might gather from the name shared_ptr, that's a reference-counted way to manage pointers to memory, and thus avoid memory leaks.

The developers just couldn't tolerate using the names provided by their widely used library solving a widely understood problem, and needed to invent their own names, which made the code less clear. The same is true for makeAssignment. And this pattern is used for nearly every class, because the developers involved didn't understand object lifetimes, when to allow things to be stack allocated, or how ownership should really work in an application.

This is hardly the only WTF in the code, but Tim says:

Preceding the 98 standard, there is a LOT of C-with-classes code. But this representative line speaks to the complete lack of thought that has gone into much of codebase. That whitespace is as-is from the source.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

There are a lot of cases where the submission is "this was server side generated JavaScript and they were loading constants". Which, honestly, is a WTF, but it isn't interesting code. Things like this:

if (false === true) { // do stuff }

That's absolutely the wrong way to do that, and I hate it, but there's just so many times you can say, "send server-side values to the client as an object, not inline".

But Daniel's electrical provider decided to come up with an example of this that really takes it to the next level of grossness.

var isMobile = "" === "true"; var isAndroid = "" === "true"; var isIPad = "" === "true"; var isIPhone = "" === "true";

For starters, they're doing device detection on the server side, which isn't the worst possible idea, but it means they're relying on header fields or worse: the user agent string. Maybe they're checking the device resolution. The fact that they're naming specific devices instead of browser capabilities hints at a terrible hackjob of reactive webdesign- likely someone wrote a bunch of JavaScript that alters the desktop stylesheet to cram the desktop site onto a mobile device. But that's just background noise.

Look at that code.

First, we've got some lovely order-of-operations abuse: === has higher precedence than =, which makes sense but hardly makes this code readable. The first time I saw this, my brain wanted the assignment to happen first.

But what's really special to me is the insistence on making this stringly typed. They control both sides of the code, so they could have just done booleans on both sides. And sure, there's a world where they're just dumb, or didn't trust their templating engine to handle that well.

I've seen enough bad code, though, to have a different suspicion. I can't confirm it, but c'mon, you know in your hearts this is true: the function which is doing device detection returns a string itself, and that string isn't always a boolean for some reason. So they needed to wrap the output in quotes, because that was the only way to make sure that the JavaScript actually could be executed without a syntax error.

I can't be sure that's true from this little snippet. But look at this code, and tell me that someone didn't make that mistake.

.comment { border: none; } [Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.

Just because you get fired doesn't mean that your pull requests are automatically closed. Dallin was in the middle of reviewing a PR by Steve when the email came out announcing that Steve no longer worked at the company.

Let's take a look at that PR, and maybe we can see why.

$originalUndrawn = DecimalHelper::toDecimal($party->limit)->sub(DecimalHelper::toDecimal($party->drawn));

This is the original code, which represents operations on investments. An investment is represented by a note, and belongs to one or more partys. The amount that can be drawn is set by a limit, which can belong to either the party or the note.

What our developer was tasked with doing was allow a note to have no limit. This means changing all the places where the note's limit is checked. So this is what they submitted:

if ($note->limit == null) { $originalUndrawn = DecimalHelper::toDecimal($party->limit)->sub(DecimalHelper::toDecimal($party->drawn)); } else { $originalUndrawn = DecimalHelper::toDecimal($party->limit)->sub(DecimalHelper::toDecimal($party->drawn)); }

You'll note here that the note limit isn't part of calculating the party limits, so both branches do the same thing. And then there's the deeper question of "is a null really the best way to represent this?" especially given that elsewhere in the code they have an "unlimited" flag that disables limit checking.

Now, Steve wasn't let go only for their code- they were just a miserable co-worker who liked to pick fights in pull request comments. So the real highlight of Steve's dismissal was that Dallin got to have a meaningful discussion about the best way to make this change with the rest of the team, and Steve didn't have a chance to disrupt it.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.

Decreasingly hungry thrillseeker Weaponized Fun has second thoughts about the risk to which they're willing to expose their palate. "In addition to Budget Bytes mailing list not knowing who I am, I'm not sure they know what they're making. I'm having a hard time telling whether 'New Recipe 1' sounds more enticing than 'New Recipe 3.' I sure hope they remembered the ingredients."

 

An anonymous reader frets that "The Guardian claims an article is *more* than 7 years old (it's not, as of today, January 26)" Date math is hard.

 

"Oh snap!" cried The Beast in Black I feel like we've seen several errors like this from Firefox recently: problems with 0 and -1 as sentinel values.

 

Faithful contributor Michael R. doubled up on the FB follies this week; here's one. Says Michael "Those hard tech interviews at META really draw in the best talent."

 

Finally for this week, a confused Stewart found an increasingly rare type of classic Error'd. "Trying to figure out how to ignore as instructed, when there is no ignore option. Do I just ignore it?" For completeness, the options should also include Abort

 

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.

I'm a JSON curmudgeon, in that I think that its type-system, inherited from JavaScript, is bad. It's a limited vocabulary of types, and it forces developers to play odd games of convention. For example, because it lacks any sort of date type, you either have to explode your date out as a sub-dictionary (arguably, the "right" approach) or do what most people do- use an ISO formatted string as your date. The latter version requires you to attempt to parse the sting to validate the data, but validating JSON is a whole thing anyway.

But, enough about me being old and cranky. Do you know one type JSON supports? Boolean values.

Which is why this specification from today's anonymous submitter annoys me so much:

field: sw_auto_update type: string valid values: /(on)|(off)/ field: data_auto_update type: string valid values: /(on)|(off)/ field: spanning_tree_protocol type: string valid values: /(on)|(off)/

Their custom validator absolutely requires the use of strings, and absolutely requires that they have these values. Sending a boolean, or worse, the string "true" causes the request to get rejected.

Our submitter doesn't explain why it's this way, but I have a strong suspicion that it's because it was originally designed to support a form submission with radio buttons. The form is long gone, but the API contract remains.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.

Today's anonymous submitter spent a few weeks feeling pretty good about themselves. You see, they'd inherited a gigantic and complex pile of code, an application spread out across 15 backend servers, theoretically organized into "modules" and "microservices" but in reality was a big ball of mud. And after a long and arduous process, they'd dug through that ball of mud and managed to delete 190 files, totaling 30,000 lines of code. That was fully 2/3rds of the total codebase, gone- and yet the tests continued to pass, the application continued to run, and everyone was just much happier with it.

Two weeks later, a new ticket comes in: users are getting a 403 error when trying to access the "User Update" screen. Our submitter has seen a lot of these tickets, and it almost always means that the user's permissions are misconfigured. It's an easy fix, and not a code problem.

Just to be on the safe side, though, they pull up the screen with their account- guaranteed to have the right permissions- and get a 403.

As you can imagine, the temptation to sneak a few fixes in alongside this massive refactoring was impossible to resist. One of the problems was that most of their routes were camelCase URLs, but userupdate was not. So they'd fixed it. It was a minor change, and it worked in testing. So what was happening?

Well, there was a legacy authorization database. It was one of those 15 backend servers, and it ran no web code, and thus wasn't touched by our submitter's refactoring. Despite their web layer having copious authorization and authentication code, someone had decided back in the olden days, to implement that authorization and authentication in its own database.

Not every request went through this database. It impacted new sessions, but only under specific conditions. But this database had a table in it, which listed off all the routes. And unlike the web code, which used regular expressions for checking routes, and were case insensitive, this database did a strict equality comparison.

The fix was simple: update the table to allow userUpdate. But it also pointed towards a deeper, meaner target for future refactoring: dealing with this sometimes required (but often not!) authentication step lurking in a database that no one had thought about until our submitter's refactoring broke something.

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.

Paul's co-worker needed to manage some data in a tree. To do that, they wrote this Java function:

private static boolean existsFather(ArrayList<Integer> fatherFolder, Integer fatherId) { for (Integer father : fatherFolder) { if (father.equals(fatherId)) return true; } return false; }

I do not know what the integers in use represent here. I don't think they're actually representing "folders", despite the variable names in the code. I certainly hope it's not representing files and folders, because that implies they're tossing around file handles in some C-brained approach (but badly, since it implies they've got an open handle for every object).

The core WTF, in my opinion, is this- the code clearly implies some sort of tree structure, the tree contains integers, but they're not using any of the Java structures for handling trees, and implementing this slipshod approach. And even then, this code could be made more generic, as the general process works with any sane Java type.

But there's also the obvious WTF: the java.util.Collection interface, which an ArrayList implements, already handles all of this in its contains method. This entire function could be replaced with fatherFolder.contains(fatherId).

Paul writes: "I guess the last developer didn't know that every implementation of a java.util.Collection has a method called contains. At least they knew how to do a for-each.".

[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.

Denise's company formed a new team. They had a lot of low-quality legacy code, and it had gotten where it was, in terms of quality, because the company had no real policy or procedures which encouraged good code. "If it works, it ships," was basically the motto. They wanted to change that, and the first step was creating a new software team to kick of green-field projects with an eye towards software craftsmanship.

Enter Jack. Jack was the technical lead, and Jack had a vision of good software. This started with banning ORM-generated database models. But it also didn't involve writing raw SQL either- Jack hand-forged their tables with the Visual Table Designer feature of SQL Server Management Studio.

"The advantage," he happily explained to Denise, "is that we can then just generate our ORM layer right off the database. And when the database changes, we just regenerate- it's way easier than trying to build migrations."

"Right, but even if we're not using ORM migrations, we still want to write migration scripts for our changes to our database. We need to version control them and test them."

"We test them by making the change and running the test suite," Jack said.

And what a test suite it was. There was 100% test coverage. There was test coverage on simple getter/setter methods. There was test coverage on the data transfer objects, which had no methods but getters and setters. There were unit tests for functions that did nothing more than dispatch to built-in functions. Many of the tests just verified that a result was returned, but never checked what the result was. There were unit tests on the auto-generated ORM objects.

The last one, of course, meant that any time they changed the database, there was a significant risk that the test suite would fail on code that they hadn't written. Not only did they need to update the code consuming the data, the tests on that code, they also had to update the tests on the autogenerated code.

Jack's magnum opus, in the whole thing, was that he designed the software with a plugin architecture. Instead of tightly coupling different implementations of various modules together, there was a plugin loader which could fetch an assembly at runtime and use that. Unfortunately, while the whole thing could have plugins, all of the abstractions leaked across module boundaries so you couldn't reasonably swap out plugins without rewriting the entire application. Instead of making a modular architecture, Jack just made starting the application wildly inefficient.

Denise and her team brought their concerns to management. Conversations were had, and it fell upon Jack to school them all. Cheerfully, he said: "Look, not everyone gets software craftsmanship, so I'm going to implement a new feature as sort of a reference implementation. If you follow the pattern I lay out, you'll have an easy time building good code!"

The new feature was an identity verification system which called for end users to upload photographs of their IDs- drivers' licenses, passports, etc. It was not a feature which should have had one developer driving the whole thing, and Jack was not implementing the entire lifecycle of data management for this; instead he was just implementing the upload feature.

Jack pushed it through, out and up into production. Somehow, he short-cut past any code reviews, feature reviews, or getting anyone else to test it. He went straight to a demo in production, where he uploaded his passport and license. "So, there you go, a reference implementation for you all."

Denise went ahead and ran her own test, with a synthetic ID for a test user, which didn't contain any real humans' information. The file upload crashed. In fact, in an ultimate variation of "it works on my machine," the only person who ever successfully used the upload feature was Jack. Of course, since the upload never worked, none of the other features, like retention policies, ever got implemented either.

Now, this didn't mean the company couldn't do identity verification- they had an existing system, so they just kept redirecting users to that, instead of the new version, which didn't work.

Jack went on to other features, though, because he was a clever craftsman and needed to bring his wisdom to the rest of their project. So the file upload just languished, never getting fixed. Somehow, this wasn't Jack's fault, management didn't hold him responsible, and everyone was still expected to follow the patterns he used in designing the feature to guide their own work.

Until, one day, the system was breached by hackers. This, surprisingly, had nothing to do with Jack's choices- one of the admins got phished. This meant that the company needed to send out an announcement, informing users that they were breached. "We deeply regret the breach in our identity verification system, but can confirm that no personal data for any of our customers was affected."

Jack, of course, was not a customer, so he got a private disclosure that his passport and ID had been compromised.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.