Eri has a mix of a "true confession" and a "wait, really?" today.

The programming language Vala bills itself as a C# like language that compiles into something pretty close to C performance, designed specifically for writing code against Gnome and its associated libraries.

One of the C#-isms in brings in is async/await type semantics. You can yield someAsyncFunction(), which returns control to the caller, allowing it to proceed until the yielded function returns an actual value.

Because it has asynchronous functions, many library functions for handling I/O are already async. So you can make_directory_async, which yields control so you can keep executing while waiting for the filesystem to make your directory.

There are also synchronous versions of those methods. And then there's create_directory_with_parents, which will create a chain of directories for you. That's the synchronous version, and Vala's core library has decided not to provide an asynchronous version of it, which is my "wait, really?" I suspect it's really about the race conditions involved and the risks of things going wrong while doing it asynchronously; all solvable problems, but tricky ones to solve.

But it's the problem Eri had, and this is their solution:

/// Note: does not throw if target already exists
async void create_directory_with_parents_async(File file, Cancellable? cancellable = null) throws Error {
	var to_create = new File[0];
	var? current_target = file;
	while(current_target != null) {
		try {
			yield current_target.make_directory_async(Priority.DEFAULT, cancellable);
		} catch(IOError.NOT_FOUND e) {
			to_create += current_target;
			current_target = current_target.get_parent();
			continue;
		} catch(IOError.EXISTS e) {
			break;
		}
		break;
	}

	for (int i = to_create.length - 1; i >= 0; --i) {
		try {
			yield to_create[i].make_directory_async(Priority.DEFAULT, cancellable);
		} catch(IOError.EXISTS e) {
			// Created by another process
		}
	}
}

If I'm reading this correctly, we start by trying to create the full path to our leaf node. If there's a not found error, we go up one level and try and create that one. We keep trying that until we either run out of parent nodes to try against, or we hit a directory that already exists, or we successfully create a directory. All along the way, we keep appending the current_target to our to_create array.

Once we've gotten that baseline, we then iterate across our to_create array, backwards, creating the shortest non-existent paths first.

This works, but it's ugly as sin. Mostly, it's ugly because we're using exceptions for flow control instead of doing things like checking for file existence, though I suppose those checks may also break our goal of doing all our I/O operations in an async context. I don't know enough about Vala to know the better way of doing this.

Eri writes:

The function works as intended, but trying to trace control flow through the first loop is not pleasant. Ironically, the C mechanism Vala wraps is slightly advanced error codes, which would be nicer to work with in this case

Eri also provides a slightly re-worked version of the main loop, that is at least a bit easier to follow, but still an ugly approach:

	while(current_target != null) {
		try {
			yield current_target.make_directory_async(Priority.DEFAULT, cancellable);
			break;
		} catch(IOError.EXISTS e) {
			break;
		} catch(IOError.NOT_FOUND e) {
			to_create += current_target;
			current_target = current_target.get_parent();
		}
	}

Still, since this is an attempt to patch over a missing core library method and solve a tricky problem about how to handle race conditions, I think absolution is reasonable. It's ugly, it's weird, but it does the job. Go hide it in a box, and never touch its implementation again- except to make it go away.

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

It's a holiday here in the US, a celebration of labor, so we're reaching back through the archives for a story about an attempt to be labor saving that was not successful. Original. --Remy

I’ve had to write a few domain-specific-languages in the past. As per Remy’s Law of Requirements Gathering, it’s been mostly because the users needed an Excel-like formula language. The danger of DSLs, of course, is that they’re often YAGNI in the extreme, or at least a sign that you don’t really understand your problem.

XML, coupled with schemas, is a tool for building data-focused DSLs. If you have some complex structure, you can convert each of its features into an XML attribute. For example, if you had a grammar that looked something like this:

The Source specification obeys the following syntax

source = ( Feature1+Feature2+... ":" ) ? steps

Feature1 = "local" | "global"

Feature2 ="real" | "virtual" | "ComponentType.all"

Feature3 ="self" | "ancestors" | "descendants" | "Hierarchy.all"

Feature4 = "first" | "last" | "DayAllocation.all"

If features are specified, the order of features as given above has strictly to be followed.

steps = oneOrMoreNameSteps | zeroOrMoreNameSteps | componentSteps

oneOrMoreNameSteps = nameStep ( "." nameStep ) *

zeroOrMoreNameSteps = ( nameStep "." ) *

nameStep = "#" name

name is a string of characters from "A"-"Z", "a"-"z", "0"-"9", "-" and "_". No umlauts allowed, one character is minimum.

componentSteps is a list of valid values, see below.

Valid 'componentSteps' are:

- GlobalValue
- Product
- Product.Brand
- Product.Accommodation
- Product.Accommodation.SellingAccom
- Product.Accommodation.SellingAccom.Board
- Product.Accommodation.SellingAccom.Unit
- Product.Accommodation.SellingAccom.Unit.SellingUnit
- Product.OnewayFlight
- Product.OnewayFlight.BookingClass
- Product.ReturnFlight
- Product.ReturnFlight.BookingClass
- Product.ReturnFlight.Inbound
- Product.ReturnFlight.Outbound
- Product.Addon
- Product.Addon.Service
- Product.Addon.ServiceFeature

In addition to that all subsequent steps from the paths above are permitted, that is 'Board', 
'Accommodation.SellingAccom' or 'SellingAccom.Unit.SellingUnit'.
'Accommodation.Unit' in the contrary is not permitted, as here some intermediate steps are missing.

You could turn that grammar into an XML document by converting syntax elements to attributes and elements. You could do that, but Stella’s predecessor did not do that. That of course, would have been work, and they may have had to put some thought on how to relate their homebrew grammar to XSD rules, so instead they created an XML schema rule for SourceAttributeType that verifies that the data in the field is valid according to the grammar… using regular expressions. 1,310 characters of regular expressions.

<xs:simpleType>
    <xs:restriction base="xs:string">
            <xs:pattern value="(((Scope.)?(global|local|current)\+?)?((((ComponentType.)?
(real|virtual))|ComponentType.all)\+?)?((((Hierarchy.)?(self|ancestors|descendants))|Hierarchy.all)\+?)?
((((DayAllocation.)?(first|last))|DayAllocation.all)\+?)?:)?(#[A-Za-z0-9\-_]+(\.(#[A-Za-z0-9\-_]+))*|(#[A-Za-z0-
9\-_]+\.)*
(ThisComponent|GlobalValue|Product|Product\.Brand|Product\.Accommodation|Product\.Accommodation\.SellingAccom|Prod
uct\.Accommodation\.SellingAccom\.Board|Product\.Accommodation\.SellingAccom\.Unit|Product\.Accommodation\.Selling
Accom\.Unit\.SellingUnit|Product\.OnewayFlight|Product\.OnewayFlight\.BookingClass|Product\.ReturnFlight|Product\.
ReturnFlight\.BookingClass|Product\.ReturnFlight\.Inbound|Product\.ReturnFlight\.Outbound|Product\.Addon|Product\.
Addon\.Service|Product\.Addon\.ServiceFeature|Brand|Accommodation|Accommodation\.SellingAccom|Accommodation\.Selli
ngAccom\.Board|Accommodation\.SellingAccom\.Unit|Accommodation\.SellingAccom\.Unit\.SellingUnit|OnewayFlight|Onewa
yFlight\.BookingClass|ReturnFlight|ReturnFlight\.BookingClass|ReturnFlight\.Inbound|ReturnFlight\.Outbound|Addon|A
ddon\.Service|Addon\.ServiceFeature|SellingAccom|SellingAccom\.Board|SellingAccom\.Unit|SellingAccom\.Unit\.Sellin
gUnit|BookingClass|Inbound|Outbound|Service|ServiceFeature|Board|Unit|Unit\.SellingUnit|SellingUnit))"/>
    </xs:restriction>
</xs:simpleType>
</xs:union>

There’s a bug in that regex that Stella needed to fix. As she put it: “Every time you evaluate it a few little kitties die because you shouldn’t use kitties to polish your car. I’m so, so sorry, little kitties…”

The full, unexcerpted code is below, so… at least it has documentation. In two languages!

<xs:simpleType name="SourceAttributeType">
                <xs:annotation>
                        <xs:documentation xml:lang="de">
                Die Source Angabe folgt folgender Syntax

                        source = ( Eigenschaft1+Eigenschaft2+... ":" ) ? steps

                        Eigenschaft1 = "local" | "global"

                        Eigenschaft2 ="real" | "virtual" | "ComponentType.all"

                        Eigenschaft3 ="self" | "ancestors" | "descendants" | "Hierarchy.all"

                        Eigenschaft4 = "first" | "last" | "DayAllocation.all"

                        Falls Eigenschaften angegeben werden muss zwingend die oben angegebene Reihenfolge der Eigenschaften eingehalten werden.

                        steps = oneOrMoreNameSteps | zeroOrMoreNameSteps | componentSteps

                        oneOrMoreNameSteps = nameStep ( "." nameStep ) *

                        zeroOrMoreNameSteps = ( nameStep "." ) *

                        nameStep = "#" name

                        name ist eine Folge von Zeichen aus der Menge "A"-"Z", "a"-"z", "0"-"9", "-" und "_". Keine Umlaute. Mindestens ein Zeichen

                        componentSteps ist eine Liste gültiger Werte, siehe im folgenden

                Gültige 'componentSteps' sind zunächst:

                        - GlobalValue
                        - Product
                        - Product.Brand
                        - Product.Accommodation
                        - Product.Accommodation.SellingAccom
                        - Product.Accommodation.SellingAccom.Board
                        - Product.Accommodation.SellingAccom.Unit
                        - Product.Accommodation.SellingAccom.Unit.SellingUnit
                        - Product.OnewayFlight
                        - Product.OnewayFlight.BookingClass
                        - Product.ReturnFlight
                        - Product.ReturnFlight.BookingClass
                        - Product.ReturnFlight.Inbound
                        - Product.ReturnFlight.Outbound
                        - Product.Addon
                        - Product.Addon.Service
                        - Product.Addon.ServiceFeature

                Desweiteren sind alle Unterschrittfolgen aus obigen Pfaden erlaubt, also 'Board', 'Accommodation.SellingAccom' oder 'SellingAccom.Unit.SellingUnit'.
                'Accommodation.Unit' hingegen ist nicht erlaubt, da in diesem Fall einige Zwischenschritte fehlen.

                                </xs:documentation>
                        <xs:documentation xml:lang="en">
                                The Source specification obeys the following syntax

                                source = ( Feature1+Feature2+... ":" ) ? steps

                                Feature1 = "local" | "global"

                                Feature2 ="real" | "virtual" | "ComponentType.all"

                                Feature3 ="self" | "ancestors" | "descendants" | "Hierarchy.all"

                                Feature4 = "first" | "last" | "DayAllocation.all"

                                If features are specified, the order of features as given above has strictly to be followed.

                                steps = oneOrMoreNameSteps | zeroOrMoreNameSteps | componentSteps

                                oneOrMoreNameSteps = nameStep ( "." nameStep ) *

                                zeroOrMoreNameSteps = ( nameStep "." ) *

                                nameStep = "#" name

                                name is a string of characters from "A"-"Z", "a"-"z", "0"-"9", "-" and "_". No umlauts allowed, one character is minimum.

                                componentSteps is a list of valid values, see below.

                                Valid 'componentSteps' are:

                                - GlobalValue
                                - Product
                                - Product.Brand
                                - Product.Accommodation
                                - Product.Accommodation.SellingAccom
                                - Product.Accommodation.SellingAccom.Board
                                - Product.Accommodation.SellingAccom.Unit
                                - Product.Accommodation.SellingAccom.Unit.SellingUnit
                                - Product.OnewayFlight
                                - Product.OnewayFlight.BookingClass
                                - Product.ReturnFlight
                                - Product.ReturnFlight.BookingClass
                                - Product.ReturnFlight.Inbound
                                - Product.ReturnFlight.Outbound
                                - Product.Addon
                                - Product.Addon.Service
                                - Product.Addon.ServiceFeature

                                In addition to that all subsequent steps from the paths above are permitted, that is 'Board', 'Accommodation.SellingAccom' or 'SellingAccom.Unit.SellingUnit'.
                                'Accommodation.Unit' in the contrary is not permitted, as here some intermediate steps are missing.

                        </xs:documentation>
                </xs:annotation>
                <xs:union>
                        <xs:simpleType>
                                <xs:restriction base="xs:string">
                                        <xs:pattern value="(((Scope.)?(global|local|current)\+?)?((((ComponentType.)?(real|virtual))|ComponentType.all)\+?)?((((Hierarchy.)?(self|ancestors|descendants))|Hierarchy.all)\+?)?((((DayAllocation.)?(first|last))|DayAllocation.all)\+?)?:)?(#[A-Za-z0-9\-_]+(\.(#[A-Za-z0-9\-_]+))*|(#[A-Za-z0-9\-_]+\.)*(ThisComponent|GlobalValue|Product|Product\.Brand|Product\.Accommodation|Product\.Accommodation\.SellingAccom|Product\.Accommodation\.SellingAccom\.Board|Product\.Accommodation\.SellingAccom\.Unit|Product\.Accommodation\.SellingAccom\.Unit\.SellingUnit|Product\.OnewayFlight|Product\.OnewayFlight\.BookingClass|Product\.ReturnFlight|Product\.ReturnFlight\.BookingClass|Product\.ReturnFlight\.Inbound|Product\.ReturnFlight\.Outbound|Product\.Addon|Product\.Addon\.Service|Product\.Addon\.ServiceFeature|Brand|Accommodation|Accommodation\.SellingAccom|Accommodation\.SellingAccom\.Board|Accommodation\.SellingAccom\.Unit|Accommodation\.SellingAccom\.Unit\.SellingUnit|OnewayFlight|OnewayFlight\.BookingClass|ReturnFlight|ReturnFlight\.BookingClass|ReturnFlight\.Inbound|ReturnFlight\.Outbound|Addon|Addon\.Service|Addon\.ServiceFeature|SellingAccom|SellingAccom\.Board|SellingAccom\.Unit|SellingAccom\.Unit\.SellingUnit|BookingClass|Inbound|Outbound|Service|ServiceFeature|Board|Unit|Unit\.SellingUnit|SellingUnit))"/>
                                </xs:restriction>
                        </xs:simpleType>
                </xs:union>
</xs:simpleType>

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

A common option for retrofitting heating and cooling into older homes is a mini-split, frequently tied to a heat pump. They're (relatively) cheap to install, energy efficient, and can be added without substantial modifications to the home. They also, annoyingly, are mostly controlled via IR remotes, making them challenging to wire up to home automation or even a household thermostat.

People have made solutions, and today's code comes from one of those solutions. Which, I want to stress, this code comes from an open source project for home automation, so it's not the code that's wrong, here. At first I thought it was, and had a moment of, "I'm not going to pick on some hobby project," but then I realised the hobby project points at a deeper issue.

// temperature helper these are direct mappings based on the remote
float toFahrenheit(float fromCelsius) {
    // Lookup table for specific mappings
    const std::map<float, int> lookupTable = {
        {16.0, 61}, {16.5, 62}, {17.0, 63}, {17.5, 64}, {18.0, 65},
        {18.5, 66}, {19.0, 67}, {20.0, 68}, {21.0, 69}, {21.5, 70},
        {22.0, 71}, {22.5, 72}, {23.0, 73}, {23.5, 74}, {24.0, 75},
        {24.5, 76}, {25.0, 77}, {25.5, 78}, {26.0, 79}, {26.5, 80},
        {27.0, 81}, {27.5, 82}, {28.0, 83}, {28.5, 84}, {29.0, 85},
        {29.5, 86}, {30.0, 87}, {30.5, 88}
    };

    // Check if the input is in the lookup table
    auto it = lookupTable.find(fromCelsius);
    if (it != lookupTable.end()) {
        return it->second;
    }

    // Default conversion and rounding to nearest integer
    return roundf(fromCelsius * 1.8 + 32.0);
}

Okay, I am going to pick on their code a little bit; using float as a key in a map is asking for trouble, because rounding errors are going to surprise you. But honestly, failing to find the key you're looking for is better than the opposite, since that actually does the correct thing. Because if you look carefully at the table, you'll see that it's wrong.

18C, for example, should be 64F. Well, 64.4F, but we're rounding to an integer. The choice here is to roughly map every 0.5C increase to a 1F increase, which is not the conversion factor. They try and correct- note how the table mostly steps by 0.5C, but skips 19.5C.

The opposite direction is similarly bad:

// temperature helper these are direct mappings based on the remote
float toCelsius(float fromFahrenheit) {
    // Lookup table for specific mappings
    const std::map<int, float> lookupTable = {
        {61, 16.0}, {62, 16.5}, {63, 17.0}, {64, 17.5}, {65, 18.0},
        {66, 18.5}, {67, 19.0}, {68, 20.0}, {69, 21.0}, {70, 21.5},
        {71, 22.0}, {72, 22.5}, {73, 23.0}, {74, 23.5}, {75, 24.0},
        {76, 24.5}, {77, 25.0}, {78, 25.5}, {79, 26.0}, {80, 26.5},
        {81, 27.0}, {82, 27.5}, {83, 28.0}, {84, 28.5}, {85, 29.0},
        {86, 29.5}, {87, 30.0}, {88, 30.5}
    };

    // Check if the input is in the lookup table
    auto it = lookupTable.find(static_cast<int>(fromFahrenheit));
    if (it != lookupTable.end()) {
        return it->second;
    }

    // Default conversion and rounding to nearest 0.5
    return roundf((fromFahrenheit - 32.0) / 1.8 * 2) / 2.0;
}

Here, we can be off by as much as a 1C, which is certainly a noticeable feeling.

At first glance, I thought this was just a misguided attempt at optimizing the lookup. For common values, do a lookup instead of calculating because it's faster. Seems like the kind of mistake a hobby project might make, and definitely not a WTF. But it's the comment which corrects me: these are direct mappings based on the remote.

These remotes usually have a display. So when you see on the remote that you're trying to set the temperature to a comfortable 72F, the remote is actually sending 22.5C to the unit. That's the actual temperature being sent.

Now, why on Earth does the remote behave this way? Well, I haven't cracked one open to read off the part numbers, but I'm going to go out on a limb and guess that the microcontoller in the remote doesn't handle floating point operations all that well. So it almost certainly does use a lookup table to decide what signal to send, and the lookup table is populated by "good enough" approximations of temperature conversions. There aren't a lot of places that use Fahrenheit, so being "close enough" is a reasonable solution. If you want accurate temperatures, use SI units, not "freedom units".

In the end, I'd say that neither the hobby project, nor the remote control are the WTF here; locales that insist on using weird ass units are.

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

Rachel joined a new team which was proudly "metrics driven". When she first met with her boss, Zane, he explained his thinking.

"We need to be data-driven to make good decisions, right? We're a manufacturing company. We make widgets. At the end of the day, we need to make the most widgets for the lowest cost of goods sold. So we track that, and that feeds into every decision."

The team oversaw an automated production line, which meant the software was a mix of robotics, embedded firmware, high-level web based monitoring tools, and thickets of dreaded PLC code. And because you can't build an entire factory for test purposes, they only way they could test real-world scales with real-world data was to roll changes out to production. They could simulate, they could run tests on subsets of the system, but a change in the production line software couldn't truly be validated until it rolled out into the real world.

Rachel's first task on the new team involved making some changes to their metrics dashboard. It was viewed as a good way to get her feet wet with the new team. As it turned out, the metrics dashboard was a Google Sheet, with a complex series of formulas that involved multi-level INDEX functions- essentially querying the spreadsheets like they were a database. Why not use an actual database? Oh, they did — six actually — but the company obeyed Remy's Law of Requirements Gathering: "no matter what the requirements the users ask for, what they really wanted was Excel". The database data was pulled into the spreadsheet for reporting.

Now, a complicated sheet pulling in data from not one, but six different databases, they must have a pretty complex model to explain how changes to their software would impact productivity. And since they needed to model the software to make predictions about how it'd behave in production, that model must be extremely useful.

Of course it wasn't. The only metrics they tracked were output metrics, variations on "widgets produced per unit time". There were some performance metrics, so you could maybe potentially identify "oh, our overall throughput dropped because unit 5 became a bottleneck and started taking 1.5 extra seconds per widget", but nothing that actually helped you understand how the complex system made decisions. Or even why unit 5 was taking longer.

For example, there was an automated quality control scanner. It examined widgets as they came off the line, and rejected defective ones based on a computer vision algorithm. Did that subsystem record why it rejected a widget? No, it did not. The CV model was able to tag widgets with a defect category based on what it saw, but that information didn't get recorded anywhere. In fact, it didn't even record how many widgets got rejected. The only way to know was to have an operator on the assembly line count widgets in the bin manually. Since that ate up a bunch of an operator's time, it never happened unless the developers begged for it. And since the operator still couldn't answer the question "why was this widget rejected", it wasn't all that useful anyway.

Every change to the software was scored against the overall output metrics. This meant that when Rachel was ready to push out her first software change, something that would record how many widgets were rejected and why, whether or not it could be deployed was dependent on seeing the change improve, or at least not regress, the widgets-over-time scores. But the widgets-over-time were a noisy metric; it varied based on which operators were working any given shift, or based on supply chain constraints. Or sometimes, based on when one of the machines was last calibrated- theoretically something that happened on a set schedule, but really was up to the operators. This meant the first three times Rachel rolled her code out for a test run, the metrics regressed. Nothing she changed should have impacted the metrics, but the metrics regressed due to environmental issues.

This meant making a simple change could take weeks, because you could only do final validation on the real system, which means you had to mark off a block of time for a test run, you could only run a handful of tests a day, and if metrics regressed you had to account for that before you could release the software for actual production use.

Over the first few months, Rachel added instrumentation to the code. Anything along the way to generating an output widget, she recorded. The hope was that once they had enough data, they could build a useful model of the system. Unfortunately, Zane had other ideas.

"So, you haven't improved our metrics," Zane said. "Which, I remind you, we're a metrics driven organization. Every change needs to improve our metrics."

"Sure, but I'm gathering more data so we have a better idea of what makes our metrics tick. We don't know why our system does some of the things it does, because we don't record any logging about the decisions it makes."

"Right, but we already gather the key metrics."

"But you don't gather the data that tells you why those metrics are what they are!"

"Sure," Zane said. "But those aren't our key metrics."

That, unfortunately for Rachel, was where things landed. Understanding their complex system was a low priority. Pushing top-level metrics without understanding what fed into them, that was the priority. That didn't mean Rachel was powerless: any time she made a change that she thought might help the top level metrics, she also made sure to add instrumentation that explained how that change behaved. It was the compromise that kept Zane happy: she released features that impacted the top-level metrics, but she also made the system more observable.

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

Today's representative comment ran out of room.

int maxLen = getColumnSize(session, "audit", "text_value1") - 16; // Leave some room for

No, it isn't continued on the next line and just got trimmed out, except perhaps by a careless merge. This is the entire comment.

Clearly, written by David Chase, the creator of "The Sopranos".

There are so many things we might be leaving room for. We could leave some room for dessert. Leave some room for activities. Leave some room for the holy spirit. Leave some room for improvisation.

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

Some time ago, Charles shared with us some awful PHP, aka the most common sort. Today's code sample is maybe a little too big to sum up, but I'll let Charles take a crack at it.

It's so bad that even analyzing and laughing at it feels impossible. But it’s so bad, I couldn’t not share it.

I’m the only one handling all the IT-related tasks at my company, and I don’t have anyone here to vent or laugh about this kind of thing with. So, I figured, why not share it here? I’m hoping it’ll provide at least a little bit of catharsis or some dark humor.

To make sure the confidentiality of the codebase was respected, I took the liberty of generalizing it. You might notice some inconsistencies, but that’s just me trying to keep things neutral while protecting the original structure and functionality. Apologies if it looks a bit patchy – the goal was to avoid revealing any specific details or sensitive code.

The whole block is north of 400 lines, and it's doing a lot. Or well, maybe it's not, as you'll see.

Let's star with the outermost layer.

$resm_data = $data_source->fetchData("group=" . $item_id);
foreach ($resm_data as $key => $value) {
// rest of the code here
}

We fetch data from a data source, presumably a database, passing our condition as a string, which reeks of probable SQL injection, but I don't know what library they're using. I also note they're using the key/value style of array iteration, but never actually check the key.

    $option_id = $value->option_id;
    $resm_details = $detail_source->fetch($option_id);
    if ($resm_details) {
        $label = $resm_details->{"label$lang"};
        $description = $resm_details->{"description$lang"};
        $category = $resm_details->category;

Nice little bit of "meta" programming to get their localization working, it'll fetch labelen or labelde as needed. Definitely not a horrible, dangerous way to solve that problem.

We use that again to get our currency figured out. That lets us do number formatting. So much number formatting code.

        if ($category == 0) {
            $cost = $resm_details->{"cost" . $currency};
            $child_cost = $resm_details->{"child_cost" . $currency};
            $cost_info =
                $mot[101] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol . " - " .
                $mot[102] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
        } elseif ($category == 1) {
            $cost = $resm_details->{"cost" . $currency};
            $child_cost = 0;
            $cost_info = $mot[101] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
        } elseif ($category == 2) {
            $cost = 0;
            $child_cost = $resm_details->{"child_cost" . $currency};
            $cost_info = $mot[102] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
        } elseif ($category == 4) {
            $cost = $resm_details->{"cost" . $currency};
            $child_cost = 0;
            $cost_info = $mot[500] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
        } elseif (
            $resm_details->{"cost" . $currency} == 0 and
            $resm_details->{"child_cost" . $currency} == 0
        ) {
            $cost = 0;
            $child_cost = 0;
            $cost_info = "";
        } else {
            $cost = $resm_details->{"cost" . $currency};
            $child_cost = 0;
            $cost_info = $mot[200] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
        }

What is the $mot array? Why am I jumping to seemingly random locations in that array? Clearly it contains some headers for our output.

Anyway, there's plenty of HTML string munging happening too, don't you worry.

        if ($location == 1) {
            $quantity_block =
                '<div class="quantity-container">
                    <input type="text" value="1" id="quantity-' . $option_id . '" class="qty-control" name="quantity" min="1" max="1">
                    <div class="increase button">+</div>
                    <div class="decrease button">-</div>
                </div>';
            $quantity = 1;
        } else {
            $quantity_block = "";
            $quantity = "all";
        }

And then there's this little treat for parsing the time stored in our database: $time_data = json_decode($resm_details->time_data); That tells me they're storing date times as strings, so that's fun.

There are also a couple more bon mots as they build a drop down list:

            $departure_select = '<option value="-1">' . $mot[300] . '</option>';
            $arrival_select = '<option value="-1">' . $mot[301] . '</option>';

And then there's this monstrosity:

        // Add the main option to the template
        $TEMPLATE->SET_BLOCK_VARIABLES("Block_OPTIONS", [
            "ID" => $option_id,
            "QUANTITY" => $quantity,
            "LABEL_CLASS" => $label_class,
            "ACTION_CLASS" => $action_class,
            "LABEL" => $label,
            "DESCRIPTION" => $description,
            "QUANTITY_BLOCK" => $quantity_block,
            "COST_INFO" => $cost_info,
            "HOST_COST_DISPLAY" => $host_cost_display,
            "COST" => $cost,
            "CHILD_COST" => $child_cost,
            "LOCATION" => $location,
            "CATEGORY" => $category,
            "HOST_CLASS" => $host_class,
            "EXTRA" => $extra,
            "HOST_EXTRA" => $host_extra,
            "TIME_CHECKED" => ($has_times == 1) ? 'selected="' . $option_id . '" checked="true"' : '',
            "TIME_BLOCK" => $time_block
        ]);

All the nonsense that we concatenate together above gets shoved into some sort of template. And after that, that's where the good stuff starts. Because guess what? We have to do the same thing for child items.

        $resm_children_data = $data_source->fetchData("parent=" . $option_id);
        foreach ($resm_children_data as $child_key => $child_value) {
            $child_option_id = $child_value->option_id;
            $resm_child_details = $detail_source->fetch($child_option_id);

That's right, it's the same block of code, not quite copy/pasted, since they needed to put the word child in everything.

                // Add the child option to the template
                $TEMPLATE->SET_BLOCK_VARIABLES("Block_OPTIONS.Block_OPTIONS_CHILD", [
                    "ID" => $child_option_id,
                    "QUANTITY" => $child_quantity,
                    "LABEL_CLASS" => $child_label_class,
                    "ACTION_CLASS" => $child_action_class,
                    "LABEL" => $child_label,
                    "DESCRIPTION" => $child_description,
                    "QUANTITY_BLOCK" => $child_quantity_block,
                    "COST_INFO" => $child_cost_info,
                    "HOST_COST_DISPLAY" => $child_host_cost_display,
                    "COST" => $child_cost,
                    "CHILD_COST" => $child_extra_cost,
                    "LOCATION" => $child_location,
                    "CATEGORY" => $child_category,
                    "HOST_CLASS" => $child_host_class,
                    "EXTRA" => $child_extra,
                    "HOST_EXTRA" => $child_host_extra,
                    "TIME_CHECKED" => ($child_has_times == 1) ? 'selected="' . $child_option_id . '" checked="true"' : '',
                    "TIME_BLOCK" => $child_time_block
                ]);

And now, if you liked the child record, guess what? Those children have got siblings. What can I say, it's a big family.

                $resm_sibling_data = $data_source->fetchData("parent=" . $parent_option_id);
                foreach ($resm_sibling_data as $sibling_key => $sibling_value) {
                    $sibling_option_id = $sibling_value->option_id;
                    $resm_sibling_details = $detail_source->fetch($sibling_option_id);

And that means, yes, we also use that template thing again:

                        // Add the sibling option to the template
                        $TEMPLATE->SET_BLOCK_VARIABLES("Block_OPTIONS.Block_OPTIONS_CHILD", [
                            "ID" => $sibling_option_id,
                            "QUANTITY" => $sibling_quantity,
                            "LABEL_CLASS" => $sibling_label_class,
                            "ACTION_CLASS" => $sibling_action_class,
                            "LABEL" => $sibling_label,
                            "DESCRIPTION" => $sibling_description,
                            "QUANTITY_BLOCK" => $sibling_quantity_block,
                            "COST_INFO" => $sibling_cost_info,
                            "HOST_COST_DISPLAY" => $sibling_host_cost_display,
                            "COST" => $sibling_cost,
                            "SIBLING_COST" => $sibling_extra_cost,
                            "LOCATION" => $sibling_location,
                            "CATEGORY" => $sibling_category,
                            "HOST_CLASS" => $sibling_host_class,
                            "EXTRA" => $sibling_extra,
                            "HOST_EXTRA" => $sibling_host_extra,
                            "TIME_CHECKED" => ($sibling_has_times == 1) ? 'selected="' . $sibling_option_id . '" checked="true"' : '',
                            "TIME_BLOCK" => $sibling_time_block
                        ]);

Someday, I hope the person who wrote this learn about methods and function calls. Maybe they could write their own one day.

In any case, here's the whole thing:

$resm_data = $data_source->fetchData("group=" . $item_id);
foreach ($resm_data as $key => $value) {
    $option_id = $value->option_id;
    $resm_details = $detail_source->fetch($option_id);
    if ($resm_details) {
        $label = $resm_details->{"label$lang"};
        $description = $resm_details->{"description$lang"};
        $category = $resm_details->category;

        // Process cost based on category
        if ($category == 0) {
            $cost = $resm_details->{"cost" . $currency};
            $child_cost = $resm_details->{"child_cost" . $currency};
            $cost_info =
                $mot[101] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol . " - " .
                $mot[102] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
        } elseif ($category == 1) {
            $cost = $resm_details->{"cost" . $currency};
            $child_cost = 0;
            $cost_info = $mot[101] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
        } elseif ($category == 2) {
            $cost = 0;
            $child_cost = $resm_details->{"child_cost" . $currency};
            $cost_info = $mot[102] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
        } elseif ($category == 4) {
            $cost = $resm_details->{"cost" . $currency};
            $child_cost = 0;
            $cost_info = $mot[500] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
        } elseif (
            $resm_details->{"cost" . $currency} == 0 and
            $resm_details->{"child_cost" . $currency} == 0
        ) {
            $cost = 0;
            $child_cost = 0;
            $cost_info = "";
        } else {
            $cost = $resm_details->{"cost" . $currency};
            $child_cost = 0;
            $cost_info = $mot[200] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
        }

        $location = $resm_details->location;
        $label_class = $location == 1 ? "has-quantity" : "no-quantity";
        $action_class = $location == 1 ? "active-with-quantity" : "active-no-quantity";
        if ($location == 1) {
            $quantity_block =
                '<div class="quantity-container">
                    <input type="text" value="1" id="quantity-' . $option_id . '" class="qty-control" name="quantity" min="1" max="1">
                    <div class="increase button">+</div>
                    <div class="decrease button">-</div>
                </div>';
            $quantity = 1;
        } else {
            $quantity_block = "";
            $quantity = "all";
        }

        $has_times = $resm_details->has_times;

        if ($has_times == 1) {
            $time_counter++;
            $time_data = json_decode($resm_details->time_data);

            $departure_select = '<option value="-1">' . $mot[300] . '</option>';
            $arrival_select = '<option value="-1">' . $mot[301] . '</option>';

            $departures = $time_data->departures;
            $arrivals = $time_data->arrivals;

            foreach ($departures as $dep_key => $departure) {
                $departure_select .= '<option value="' . $dep_key . '">' . $departure . '</option>';
            }

            foreach ($arrivals as $arr_key => $arrival) {
                $arrival_select .= '<option value="' . $arr_key . '">' . $arrival . '</option>';
            }

            $departure_block = '<div class="col-half time-select-' . $option_id . '" style="padding-right: 0;">
                <select class="form-control time-departure" style="text-align: center;">' . $departure_select . '</select>
                </div>';

            $arrival_block = '<div class="col-half time-select-' . $option_id . '" style="padding-left: 0;">
                <select class="form-control time-arrival" style="text-align: center;">' . $arrival_select . '</select>
                </div>';

            $time_block = '<div class="row time-container">
                ' . $departure_block . $arrival_block . '
            </div><small class="error-message time-error">' . $mot[302] . '</small>';
        } else {
            $time_block = '';
        }

        $host_cost_display = "";
        $extra = $resm_details->extra;
        $host_extra = $resm_details->host_extra;
        $host_class = "";

        if ($extra == 1) {
            $extra_cost_1 = $resm_details->{"cost" . $currency . "_1"};
            $extra_cost_2 = $resm_details->{"cost" . $currency . "_2"};

            $default_extra = $host_price == 0 ? $extra_cost_2 : $extra_cost_1;
            $cost_info = $mot[101] . ' : +<span class="extra-cost">' . number_format($default_extra, 2, ",", " ") .
                "</span>" . $currency_symbol . " - " . $mot[102] . ' : +<span class="child-cost">0.00</span>' . $currency_symbol;

            $host_class = " extra-option";
        }
        if ($host_extra == 1) {
            $host_cost_display =
                '<span class="host-cost">' . $mot[500] . ' : +<span class="host-price">0.00</span>' . $currency_symbol .
                "</span><br>";
        }

        // Add the main option to the template
        $TEMPLATE->SET_BLOCK_VARIABLES("Block_OPTIONS", [
            "ID" => $option_id,
            "QUANTITY" => $quantity,
            "LABEL_CLASS" => $label_class,
            "ACTION_CLASS" => $action_class,
            "LABEL" => $label,
            "DESCRIPTION" => $description,
            "QUANTITY_BLOCK" => $quantity_block,
            "COST_INFO" => $cost_info,
            "HOST_COST_DISPLAY" => $host_cost_display,
            "COST" => $cost,
            "CHILD_COST" => $child_cost,
            "LOCATION" => $location,
            "CATEGORY" => $category,
            "HOST_CLASS" => $host_class,
            "EXTRA" => $extra,
            "HOST_EXTRA" => $host_extra,
            "TIME_CHECKED" => ($has_times == 1) ? 'selected="' . $option_id . '" checked="true"' : '',
            "TIME_BLOCK" => $time_block
        ]);

        $resm_children_data = $data_source->fetchData("parent=" . $option_id);
        foreach ($resm_children_data as $child_key => $child_value) {
            $child_option_id = $child_value->option_id;
            $resm_child_details = $detail_source->fetch($child_option_id);
            if ($resm_child_details) {
                $child_label = $resm_child_details->{"label$lang"};
                $child_description = $resm_child_details->{"description$lang"};
                $child_category = $resm_child_details->category;

                // Process cost for child category
                if ($child_category == 0) {
                    $child_cost = $resm_child_details->{"cost" . $currency};
                    $child_extra_cost = $resm_child_details->{"child_cost" . $currency};
                    $child_cost_info =
                        $mot[101] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol . " - " .
                        $mot[102] . " : +" . number_format($child_extra_cost, 2, ",", " ") . $currency_symbol;
                } elseif ($child_category == 1) {
                    $child_cost = $resm_child_details->{"cost" . $currency};
                    $child_extra_cost = 0;
                    $child_cost_info = $mot[101] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
                } elseif ($child_category == 2) {
                    $child_cost = 0;
                    $child_extra_cost = $resm_child_details->{"child_cost" . $currency};
                    $child_cost_info = $mot[102] . " : +" . number_format($child_extra_cost, 2, ",", " ") . $currency_symbol;
                } elseif ($child_category == 4) {
                    $child_cost = $resm_child_details->{"cost" . $currency};
                    $child_extra_cost = 0;
                    $child_cost_info = $mot[500] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
                } elseif (
                    $resm_child_details->{"cost" . $currency} == 0 and
                    $resm_child_details->{"child_cost" . $currency} == 0
                ) {
                    $child_cost = 0;
                    $child_extra_cost = 0;
                    $child_cost_info = "";
                } else {
                    $child_cost = $resm_child_details->{"cost" . $currency};
                    $child_extra_cost = 0;
                    $child_cost_info = $mot[200] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
                }

                $child_location = $resm_child_details->location;
                $child_label_class = $child_location == 1 ? "has-quantity" : "no-quantity";
                $child_action_class = $child_location == 1 ? "active-with-quantity" : "active-no-quantity";
                if ($child_location == 1) {
                    $child_quantity_block =
                        '<div class="quantity-container">
                    <input type="text" value="1" id="child-quantity-' . $child_option_id . '" class="qty-control" name="quantity" min="1" max="1">
                    <div class="increase button">+</div>
                    <div class="decrease button">-</div>
                </div>';
                    $child_quantity = 1;
                } else {
                    $child_quantity_block = "";
                    $child_quantity = "all";
                }

                $child_has_times = $resm_child_details->has_times;

                if ($child_has_times == 1) {
                    $child_time_counter++;
                    $child_time_data = json_decode($resm_child_details->time_data);

                    $child_departure_select = '<option value="-1">' . $mot[300] . '</option>';
                    $child_arrival_select = '<option value="-1">' . $mot[301] . '</option>';

                    $child_departures = $child_time_data->departures;
                    $child_arrivals = $child_time_data->arrivals;

                    foreach ($child_departures as $child_dep_key => $child_departure) {
                        $child_departure_select .= '<option value="' . $child_dep_key . '">' . $child_departure . '</option>';
                    }

                    foreach ($child_arrivals as $child_arr_key => $child_arrival) {
                        $child_arrival_select .= '<option value="' . $child_arr_key . '">' . $child_arrival . '</option>';
                    }

                    $child_departure_block = '<div class="col-half time-select-' . $child_option_id . '" style="padding-right: 0;">
                <select class="form-control time-departure" style="text-align: center;">' . $child_departure_select . '</select>
                </div>';

                    $child_arrival_block = '<div class="col-half time-select-' . $child_option_id . '" style="padding-left: 0;">
                <select class="form-control time-arrival" style="text-align: center;">' . $child_arrival_select . '</select>
                </div>';

                    $child_time_block = '<div class="row time-container">
                ' . $child_departure_block . $child_arrival_block . '
            </div><small class="error-message time-error">' . $mot[302] . '</small>';
                } else {
                    $child_time_block = '';
                }

                $child_host_cost_display = "";
                $child_extra = $resm_child_details->extra;
                $child_host_extra = $resm_child_details->host_extra;
                $child_host_class = "";

                if ($child_extra == 1) {
                    $child_extra_cost_1 = $resm_child_details->{"cost" . $currency . "_1"};
                    $child_extra_cost_2 = $resm_child_details->{"cost" . $currency . "_2"};

                    $default_child_extra = $host_price == 0 ? $child_extra_cost_2 : $child_extra_cost_1;
                    $child_cost_info = $mot[101] . ' : +<span class="extra-cost">' . number_format($default_child_extra, 2, ",", " ") .
                        "</span>" . $currency_symbol . " - " . $mot[102] . ' : +<span class="child-cost">0.00</span>' . $currency_symbol;

                    $child_host_class = " extra-option";
                }
                if ($child_host_extra == 1) {
                    $child_host_cost_display =
                        '<span class="host-cost">' . $mot[500] . ' : +<span class="host-price">0.00</span>' . $currency_symbol .
                        "</span><br>";
                }

                // Add the child option to the template
                $TEMPLATE->SET_BLOCK_VARIABLES("Block_OPTIONS.Block_OPTIONS_CHILD", [
                    "ID" => $child_option_id,
                    "QUANTITY" => $child_quantity,
                    "LABEL_CLASS" => $child_label_class,
                    "ACTION_CLASS" => $child_action_class,
                    "LABEL" => $child_label,
                    "DESCRIPTION" => $child_description,
                    "QUANTITY_BLOCK" => $child_quantity_block,
                    "COST_INFO" => $child_cost_info,
                    "HOST_COST_DISPLAY" => $child_host_cost_display,
                    "COST" => $child_cost,
                    "CHILD_COST" => $child_extra_cost,
                    "LOCATION" => $child_location,
                    "CATEGORY" => $child_category,
                    "HOST_CLASS" => $child_host_class,
                    "EXTRA" => $child_extra,
                    "HOST_EXTRA" => $child_host_extra,
                    "TIME_CHECKED" => ($child_has_times == 1) ? 'selected="' . $child_option_id . '" checked="true"' : '',
                    "TIME_BLOCK" => $child_time_block
                ]);

                $resm_sibling_data = $data_source->fetchData("parent=" . $parent_option_id);
                foreach ($resm_sibling_data as $sibling_key => $sibling_value) {
                    $sibling_option_id = $sibling_value->option_id;
                    $resm_sibling_details = $detail_source->fetch($sibling_option_id);
                    if ($resm_sibling_details) {
                        $sibling_label = $resm_sibling_details->{"label$lang"};
                        $sibling_description = $resm_sibling_details->{"description$lang"};
                        $sibling_category = $resm_sibling_details->category;

                        // Process cost for sibling category
                        if ($sibling_category == 0) {
                            $sibling_cost = $resm_sibling_details->{"cost" . $currency};
                            $sibling_extra_cost = $resm_sibling_details->{"sibling_cost" . $currency};
                            $sibling_cost_info =
                                $mot[101] . " : +" . number_format($sibling_cost, 2, ",", " ") . $currency_symbol . " - " .
                                $mot[102] . " : +" . number_format($sibling_extra_cost, 2, ",", " ") . $currency_symbol;
                        } elseif ($sibling_category == 1) {
                            $sibling_cost = $resm_sibling_details->{"cost" . $currency};
                            $sibling_extra_cost = 0;
                            $sibling_cost_info = $mot[101] . " : +" . number_format($sibling_cost, 2, ",", " ") . $currency_symbol;
                        } elseif ($sibling_category == 2) {
                            $sibling_cost = 0;
                            $sibling_extra_cost = $resm_sibling_details->{"sibling_cost" . $currency};
                            $sibling_cost_info = $mot[102] . " : +" . number_format($sibling_extra_cost, 2, ",", " ") . $currency_symbol;
                        } elseif ($sibling_category == 4) {
                            $sibling_cost = $resm_sibling_details->{"cost" . $currency};
                            $sibling_extra_cost = 0;
                            $sibling_cost_info = $mot[500] . " : +" . number_format($sibling_cost, 2, ",", " ") . $currency_symbol;
                        } elseif (
                            $resm_sibling_details->{"cost" . $currency} == 0 and
                            $resm_sibling_details->{"sibling_cost" . $currency} == 0
                        ) {
                            $sibling_cost = 0;
                            $sibling_extra_cost = 0;
                            $sibling_cost_info = "";
                        } else {
                            $sibling_cost = $resm_sibling_details->{"cost" . $currency};
                            $sibling_extra_cost = 0;
                            $sibling_cost_info = $mot[200] . " : +" . number_format($sibling_cost, 2, ",", " ") . $currency_symbol;
                        }

                        $sibling_location = $resm_sibling_details->location;
                        $sibling_label_class = $sibling_location == 1 ? "has-quantity" : "no-quantity";
                        $sibling_action_class = $sibling_location == 1 ? "active-with-quantity" : "active-no-quantity";
                        if ($sibling_location == 1) {
                            $sibling_quantity_block =
                                '<div class="quantity-container">
                    <input type="text" value="1" id="sibling-quantity-' . $sibling_option_id . '" class="qty-control" name="quantity" min="1" max="1">
                    <div class="increase button">+</div>
                    <div class="decrease button">-</div>
                </div>';
                            $sibling_quantity = 1;
                        } else {
                            $sibling_quantity_block = "";
                            $sibling_quantity = "all";
                        }

                        $sibling_has_times = $resm_sibling_details->has_times;

                        if ($sibling_has_times == 1) {
                            $sibling_time_counter++;
                            $sibling_time_data = json_decode($resm_sibling_details->time_data);

                            $sibling_departure_select = '<option value="-1">' . $mot[300] . '</option>';
                            $sibling_arrival_select = '<option value="-1">' . $mot[301] . '</option>';

                            $sibling_departures = $sibling_time_data->departures;
                            $sibling_arrivals = $sibling_time_data->arrivals;

                            foreach ($sibling_departures as $sibling_dep_key => $sibling_departure) {
                                $sibling_departure_select .= '<option value="' . $sibling_dep_key . '">' . $sibling_departure . '</option>';
                            }

                            foreach ($sibling_arrivals as $sibling_arr_key => $sibling_arrival) {
                                $sibling_arrival_select .= '<option value="' . $sibling_arr_key . '">' . $sibling_arrival . '</option>';
                            }

                            $sibling_departure_block = '<div class="col-half time-select-' . $sibling_option_id . '" style="padding-right: 0;">
                <select class="form-control time-departure" style="text-align: center;">' . $sibling_departure_select . '</select>
                </div>';

                            $sibling_arrival_block = '<div class="col-half time-select-' . $sibling_option_id . '" style="padding-left: 0;">
                <select class="form-control time-arrival" style="text-align: center;">' . $sibling_arrival_select . '</select>
                </div>';

                            $sibling_time_block = '<div class="row time-container">
                ' . $sibling_departure_block . $sibling_arrival_block . '
            </div><small class="error-message time-error">' . $mot[302] . '</small>';
                        } else {
                            $sibling_time_block = '';
                        }

                        $sibling_host_cost_display = "";
                        $sibling_extra = $resm_sibling_details->extra;
                        $sibling_host_extra = $resm_sibling_details->host_extra;
                        $sibling_host_class = "";

                        if ($sibling_extra == 1) {
                            $sibling_extra_cost_1 = $resm_sibling_details->{"cost" . $currency . "_1"};
                            $sibling_extra_cost_2 = $resm_sibling_details->{"cost" . $currency . "_2"};

                            $default_sibling_extra = $host_price == 0 ? $sibling_extra_cost_2 : $sibling_extra_cost_1;
                            $sibling_cost_info = $mot[101] . ' : +<span class="extra-cost">' . number_format($default_sibling_extra, 2, ",", " ") .
                                "</span>" . $currency_symbol . " - " . $mot[102] . ' : +<span class="sibling-cost">0.00</span>' . $currency_symbol;

                            $sibling_host_class = " extra-option";
                        }
                        if ($sibling_host_extra == 1) {
                            $sibling_host_cost_display =
                                '<span class="host-cost">' . $mot[500] . ' : +<span class="host-price">0.00</span>' . $currency_symbol .
                                "</span><br>";
                        }

                        // Add the sibling option to the template
                        $TEMPLATE->SET_BLOCK_VARIABLES("Block_OPTIONS.Block_OPTIONS_CHILD", [
                            "ID" => $sibling_option_id,
                            "QUANTITY" => $sibling_quantity,
                            "LABEL_CLASS" => $sibling_label_class,
                            "ACTION_CLASS" => $sibling_action_class,
                            "LABEL" => $sibling_label,
                            "DESCRIPTION" => $sibling_description,
                            "QUANTITY_BLOCK" => $sibling_quantity_block,
                            "COST_INFO" => $sibling_cost_info,
                            "HOST_COST_DISPLAY" => $sibling_host_cost_display,
                            "COST" => $sibling_cost,
                            "SIBLING_COST" => $sibling_extra_cost,
                            "LOCATION" => $sibling_location,
                            "CATEGORY" => $sibling_category,
                            "HOST_CLASS" => $sibling_host_class,
                            "EXTRA" => $sibling_extra,
                            "HOST_EXTRA" => $sibling_host_extra,
                            "TIME_CHECKED" => ($sibling_has_times == 1) ? 'selected="' . $sibling_option_id . '" checked="true"' : '',
                            "TIME_BLOCK" => $sibling_time_block
                        ]);
                    }
                }
            }
        }
    }
}

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

Kevin sends us an exception handler from C++. Let's see if we can spot what's going wrong:

catch (Exception::Deadlock)
{
   retry;
}

When we catch a deadlock happening, we retry. That's not a keyword in C++, and looking at how it's used, it has to be some kind of macro, and I suspect that the macro is hiding a goto underneath it.

The real problem, though, is that we suspect we're in a deadlock situation. That means this thread is waiting on a resource held by another thread which is waiting for a resource held by this thread. Neither train may continue until the other has passed. So this retry only works if it releases the resource held by this thread (letting the deadlocking thread proceed). But does it?

Not according ot Kevin. The code already had a pile of deadlocks in it, so they brought in a highly paid consultant to try and fix them by reordering access and tracing where mutexes were causing issues. This retry just jumps back up to the top of the block, without releasing any resources. It "seems the consultant wanted to add some deadlocks of their own," Kevin says.

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

There are many cases where some sort of debugging block sneaks by, especially cases where we see preprocessors or templates working, which leave us with nonsense like if (true == false) running in production. But Codemonkey found a new twist on that sort of thing, in a SQL query being run in production.

WHERE (some conditions) AND (1 = 0 OR (1 = 1 AND (other conditions)))

The OR means that by twiddling the first equality check, we can toggle "always return rows" with "return based on condition". Toggling the second we can make it "never return rows", which I'm not certain is actually useful. I can see how these likely did start life as debugging flags, but they're still weird, still unnatural. They point to some other problem in observability in the code. And, as all "good" flags go, they're not documented anywhere, this seems like it started life as a query an analyst was running until it got turned into stored procedure to be run again and again. The flags have never been changed since the code was released, as far as anyone can tell.

Today's maybe more of a "representative data sheet entry" than anything else.

Every developer has the experience of reading the documentation. If you've been at this for some time, you've probably read bad documentation. Documentation that is incomplete, inaccurate, or otherwise flawed. Or, my personal favorite, the brief time where Oracle tried to put all of its documentation into an Adobe Flex site (aka, a Flash application, not a real web app). That one had fun bonus features, like "breaking copy and paste" and "preventing you from deep linking to a piece of the documentation".

But software documentation has got nothing on bad data sheets. When you buy an integrated chip from a vendor, whether it's a microcontroller that'll run your code, a sensor you're trying to get data from, you're at the mercy of the datasheet for understanding how it works. Sometimes, even finding an English language datasheet can be a challenge. The more complex the chip you're trying to interact with, the more complex the datasheet needs to be, and at a certain point, a lot of vendors say, "meh, you'll figure it out." I've had chips where the datasheet and reality disagreed about what registers were available, which often means that core functions of the chip require twiddling undocumented registers. For more fun, they sometimes lie about which pins on the chip do which thing, including mislabeling which pins handle power. There's nothing more fun than the tiny little "pop" of a chip dying when you throw 5V power onto a pin that's actually ground.

Now, there are some vendors, and some products, where the datasheets are pretty solid. This isn't a universal problem, but when you're working in an embedded space, "cheapest" is frequently the main criteria for picking components, and "cheapest" means "worst documented".

Which brings us to Jarek's recent experience going through a data sheet. The chip in question had a "fantastic feature" that would change how debugging worked, which was for "super users" to enable by setting a register.

3.2.4 Super User Fantastic Feature Enable Register
The Super User Fantastic Feature Enable Register allows the user to modify the behavior of the mEDBG.
Name: SUFFER
Offset:  0x0120
Reset: 0xFF

Sometimes, doing embedded work definitely feels like the SUFFER register is set.

Matlab is special. Scientists and researchers love it. Programmers hate it, and not just because it uses 1-based arrays. I've worked on a number of projects where the task was "take this Matlab code and convert it to C so we can run it on an embedded CPU". Somehow, in that process, I've avoided learning much about Matlab.

Andre works on a team that uses Matlab to manage experimental scenarios. They wanted to do a simple task: generate a set of participant-specific images, store them in a database, and reference them later. Somewhere in the intersection of the database product they were using, the Matlab license they had, and other constraints, they discovered that there simply was no good way to do this.

Enter "Jude". Jude said, "Don't worry about it, I can hack something together."

I present the code in its entirety, but don't ask me to explain it. Instead, read the comments.

nMk = 1;%counting non-response triggers, this cycles with each trial
nPress = 0;%counting button presses, noting the position in the log

for v = 1:height(resVmrk)%read each trigger
	switch nMk
		%it's kinda roundabout, but the only recognisable part is the response
		%yet I refer to it only by elision
		%and instead count the stimuli to reconstruct the pattern
		case 1%an almost reliable stimulus
			nPress = nPress+1;%trial start
			if strcmp(resVmrk.TriggerCode{v},'S1')%it must be a non-response
				resVmrk.TriggerCode{v} = 'cross';%name it properly
				nMk = 2;%and expect the next one
			else%except when it is not
				resLog.miss(nPress) = 1;%then note it down as missed
				nMk = 0;%and skip to response
			end
			
		case 2%usually reliable
			if strcmp(resVmrk.TriggerCode{v},'S1')%if the face loaded successfully
				resVmrk.TriggerCode{v} = 'face';%note it
				nMk = 3;%and proceed accordingly
				if nPress<=height(resLog)%trailing triggers at the end should be ignored
					resLog.facePos(nPress) = v;%note the position
				end
			else%if it failed to load it is a response
				resVmrk.Dur(v-1:v+1) = 0;%mark the whole trial for deletion
				resLog.miss(nPress) = 1;%and note it down as missing the face
				nMk = 0;%and skip to response
			end
			
		case 3%this one is not reliable, and sometimes is duplicated instead of missing
			if strcmp(resVmrk.TriggerCode{v},'S1')%if it is present at all
				resVmrk.TriggerCode{v} = 'empty';%first name it
				if v<height(resVmrk)%if it is not a trailing trigger, since it'll break the check otherwise
					if ~strcmp(resVmrk.TriggerCode{v+1},'S1')%if the next trigger is a response
						nMk = 0;%all is fine and it didn't freak out, proceed to response
					else%otherwise
						nMk = 3;%just treat as a double
						%and then count how many excess triggers are actually here
						nExcess = 1;%definitely one here already
						while strcmp(resVmrk.TriggerCode{v+nExcess+1},'S1')
							nExcess = nExcess+1;%and everything until the response
						end
						resVmrk.Dur(v-2:v+nExcess+2) = 0;%then mark the whole trial for deletion
						%this overwrites the same positions several time, but the important part is to get the preceding two, because I don't know which one of them is correct one, so I delete the whole trial
						if nPress<=height(resLog)
							resLog.bad(nPress) = 1;%also note it down as borked
						end
					end
				end
			else
				nMk = 0;%if it didn't happen at all simply proceed to response
			end
			
		case 0%this one reliably follows the response, so I address the response by elision
			if strcmp(resVmrk.TriggerCode{v},'S1')%skip response itself
				resVmrk.TriggerCode{v} = 'blink';%note the only reliable non-response (always following the response)
				nMk = 1;%start the trial anew
				if nPress<=height(resLog)%if it is not a trailing trigger
					resLog.respPos(nPress) = v-1;%note down the response position
					if resLog.miss(nPress)==1%and if it's a response without a stimulus
						resVmrk.Dur(v-1:v) = 0;%mark it for deletion as well
					end
				end
			end
	end
end

Ah, the classic "for-case" antipattern. That's gross enough, but what the heck is happening inside each of those cases?

My personal favorite comment is this one: "%this overwrites the same positions several time, but the important part is to get the preceding two, because I don't know which one of them is correct one, so I delete the whole trial"

Now, you may suspect comments like "usually reliable" are about what we see in the dataset, but I'm not so certain. Andre writes:

After reverting the last discovered way for his creation to corrupt the data I was able to figure out that 20% of the logs provided corresponded to different (unknown) experiments altogether.

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

Developing software can't simply be done with a text editor and a compiler. There are a variety of other tools we have to bring to bear that support our efforts and keep the team organized, like say, source control.

There are certain tools we all have to use that I would argue, nobody has actually make a version that's any good. Build tooling is one of my go-to examples: there are no good build systems, only build systems that are good enough for this task.

Another is ticket/task management. In fact, I'd go so far as to say, there are no good ticket management tools. Amongst the not good tools, I'd put Jira as one of the not goodest of all.

What makes Jira attractive to companies is the same thing that makes it miserable, and the thing that infects any "enterprise" software platform and turns it into garbage: it has all the features and expect you to build your own workflows with it. You don't merely use Jira, you have to program your own interfaces in Jira to get your workflow into the system. And if you have the misfortune to have a project manager who thinks they're more technical than they are, they'll endlessly spin up new views, new workflows, and rearrange how the work is tracked in lieu of actually working.

I've been on that team.

One of Jira's features is the ability to describe the ticket workflow: the state machine that describes your process from the initial entry of the ticket all the way down to released software or project completion. This includes routing, so that as one team member does their part of the work, it automatically goes to someone else to do the next portion of the work.

Which brings us to Klinsten. They were working on a new team, and wanted to change the ticket status from its current status to whatever came next in the workflow. So they looked at the workflow.

These are two different versions of the same workflow, one with transition labels added, which as you can see, does nothing to clarify the workflow. That it's a mix of Dutch and English doesn't help matters.

The purpose of this workflow is to help the team understand how to sequence and organize their work. But this workflow has so many states and so many transitions, it fails at this goal. Looking at it makes me just want to gesloten my browser tab, because this user isn't accepting any of this.

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

Maciej works as a freelancer, and that frequently means picking up old PHP code that nobody wants to support.

One project had been lingering for ages with key features missing. Specifically, it was supposed to make HTTP requests to other services on an interval, and use that to populate its data. "The old dev tried, but never got it working." It was Maciej's turn to give it a shot.

$ch = curl_init( $url );
curl_setopt( $ch, CURLOPT_COOKIEFILE, $cookie );
curl_setopt( $ch, CURLOPT_COOKIEJAR, $cookie );
curl_setopt( $ch, CURLOPT_COOKIE, $cookie);
// ... many other options set, of course not in a function, just copy-pasted in many locations in the code ...
curl_setopt( $ch, CURLOPT_TIMEOUT, $interval  );
$s = curl_exec( $ch );
curl_close( $ch );

This particular block of code appeared multiple times in the code. Every place they meant to send an HTTP request, they copy/pasted this code in. The URL would be a different value, but the bulk of the code was just a dozen lines of copy/pasted curl_setopts.

Now, I don't know that they were dreaming that setting CURLOPT_TIMEOUT was setting a recurrence interval. But they do call the value $interval, and I can imagine the ignorant hoping to set up cURL to automatically reinvoke the request on an interval. But even if that's their goal, that's not the actual problem with this code.

They initialize a cURL wrapper, set a pile of options, and then execute the request, storing the result in $s. And do you know what they do with the contents of $s after this?

Nothing.

The request works, perhaps not on an interval, and populates the variable, and they just never use it. The old dev "tried" and never got it working? It seems like they started and got bored.

There was far worse spaghetti code to manage in the project, but it was this gap that really got Maciej's attention.

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

"You submitted a pull request."

Indika was, in fact, reviewing the comments she'd gotten on that very same pull request, when her boss, Bill, walked up behind her. What she didn't understand is why Bill said it like it was an accusation.

"Yes?" she replied.

"Okay, well, we don't do that here. You're new, so I'll let it slide, but please review the developer guide."

Well, Indika had reviewed the developer guide, or at least thought she had. As it turned out, there was the official, company wide developer guide. That's the one she'd read. But Bill maintained his own, for his team. He hadn't ever told her about it, but apparently assumed she'd have the oracular blessings of Apollo and find it by herself.

It had this to say:

Branching is prohibited. Merging is a time wasting activity and goes against CI principles. Only use git to commit, push, and pull.

And rebase, presumably, if everyone was just committing on the main branch?

Indika asked one of her co-workers, Elise, over coffee: "Is this real?"

"Yeah," Elise said. "I'm not sure how he found out about your PR, I don't think anybody added him to the review. I mean, why would they?"

"Oh, I sent him the link," Indika said. "Just a whole, 'I'm new here, look at me doing the work!' type heads up."

"Oh yeah, definitely don't do that."

"So we do use PRs?"

Elise nodded. "Of course we do. We're not crazy. We just make sure Bill never finds out."

That seemed like a terrible way to work, but Indika went along with it, at least for a few weeks. Then an opportunity presented itself; she and Bill bumped into each other in the kitchenette grabbing coffee, and nobody else was around. At this point, Indika had already submitted a number of PRs without Bill knowing.

"Bill, I've been meaning to ask, what's your rationale for prohibiting branching?"

Bill loved being asked that question. "Well, well, it comes from twenty years of experience. What exactly does a branch get you?"

"A distinct history of changes that can be maintained and eventually merged in once a large unit of work has been done without disrupting other work that might be in flight?"

"Another point of conflict! A chance for the code you're working on to get stale. A chance to fall behind the rest of the team. Now, for a large open source team, with a lot of collaborators, a branch might make sense. I'm skeptical, but I can at least understand it. But for our internal team? It's just developers seeing a new toy and going, 'oh, shiny!'"

Indika sipped her coffee and went back to her desk. She was fortunate to have a window nearby, and looked at the squirrels playing in the branches of the tree.

Eric O was trawling through an API for handling concurrency, and found this little mismatch between the comment and the definition:

/// <summary>
/// private Status, because while this object needs to be able to set the status, consumers should only be able to check it, lest everything break.
/// </summary>
public StatusType Status {
    get {
        return _status;
    }
    set {
        if (value != _status) {
            RaisePropertyChanged("Status");
        }
    }
}

It's very important we make this property private, lest clients abuse it, and unleash dragons, chaos, and other potential horrors. Given that this happens inside of a concurrency API, I can only imagine what could go wrong when you mess this up. So sure, the comment makes sense.

The definition on the other hand, doesn't agree.

In practice, it's probably fine to do it this way, and at least the comment will show up in the documentation. If a consumer of the API misbehaves, they'll at least see that the docs suggest this is private.

The joke, of course, is the idea that the users of the API are going to read the docs, or care that one of the public methods suggests that it should be private.

Greta (previously) sends us more updates from her "Ancient Development Environment".

An important task an IDE must do is report build errors to its users. Arguably, that's one of the most important parts. I wouldn't know, I insist on building from the CLI all the time, because IDEs confuse and frighten me. I recognize I'm the weird one here, who is more comfortable in GDB than in a GUI debugger, but this isn't about me, it's about the IDE Greta is using.

It needs to display an error. Why does it need to display an error? Well, Greta hasn't figured that out yet. The error I'm about to show you doesn't really explain what happened or why or give any hint as to what needs to be done to fix it. To make matters more confusing, it doesn't happen consistently, so simply re-running the build could potentially fix it.

None of that is why we're here, though. What makes this a WTF is how the error is displayed:

Greta shares her bullet points about what she hates about this:

• It's a popup that happens arbitrarily during build, under unknown conditions
• It says nothing about what went wrong, or indeed if anything went wrong
• It's in a non-user-legible XHTML format. It shows the markup of this XHTML rather than it being rendered.
• It arguably shows the XHTML markup in the worst way possible: in a tree view (?!) with one row per source line
• The markup isn't even well-formed. I'll let you count the reasons why.
• The control used is from some sort of legacy windowing toolkit that doesn't support text anti-aliasing.
• The first button on the upper-left, "Get latest C++ Builder Direct headlines from the Internet", does nothing.

I'd honestly forgotten about the era when the Internet was still kinda novel so every application had a "push a button and go to our web page, and this somehow definitely won't just break when we change our URL structure in the future."

Greta adds:

For all of the hatred I harbour for this, the second button ("Information about C++ Builder Direct") brings up the following powerful dose of 90s nostalgia, so I can't stay mad:

Frederick A sends us a bit of null checking code, and offers us a better solution.

class ConferenceService
{
	/// <summary>
	/// Checks if conference is active
	/// </summary>
	public bool IsCalling()
	{
		try
		{
			return m_ConnectionService.Core.State.IsWebRTCConnected;
		}
		catch
		{
			return false;
		}
	}
}

This is for a web conferencing tool, which uses WebRTC to set up connections between clients in the chat. This function checks if the chat is active by checking a IsWebRTCConnected flag. But as you can see in this code, that flag is on a long chain of objects, some of which may not exist when this function is called. Thus, we wrap the whole thing up in a try/catch. If anything throws an exception, we know we can just return false. It's probably fine.

The obvious and easy fix, which Frederick proposes, is to use the C# coalescing operator: ?. m_ConnectionService?.Core?.State?.IsWebRTCConnected ?? false would solve this problem just fine.

That said, I wouldn't say that's a true fix. We're talking about a state machine here, though admittedly with two states under discussion (connected/disconnected), though there are probably more not being checked here. This information should be managed via a state machine, not via boolean flags stuffed deep in an object chain. The fix isn't a WTF, but it definitely hints at a better way to manage all of this. Now, my solution likely requires a lot more modification and code changes than what we have here, so I'm not suggesting anyone go off and rewrite this from scratch just to have a cleaner way of managing state. But folks definitely should think more carefully about how they manage state.

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

Frequent submitter Capybara James sends us this simple snippet, which highlights that even when you have the lovely convenience of Optional types, you can use them wrong.

if (StringUtils.hasLength(dto.getAssetModelUUID())
		// Other conditions
		) {
	return Optional.ofNullable(dto);
}

We access the getAssetModelUUID member of dto, and if it's a non-empty string, we can then return a nullable of this thing that's definitely not null in the first place.

Okay, in the scheme of things, that's not that bad. All we're really doing is just not using the syntactic sugar that automatically boxes your dto into a nullable type. On it's own, it's not bad, just ugly. But like all things, it doesn't exist on its own. It exists inside of a giant pile of code where this pattern is used all the time. Even functions which don't return nullable types box (and unbox) the type. Optional is scattered through the code like a magic ward against null reference exceptions.

Does it help? No, not really, the code is buggy and error prone. Will it ever get fixed? Probably not in this lifetime.

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

Flat-file style databases were designed to fit the constraints of the systems they were running on. You specify your schema in terms of "how many characters in a file we use to store this data", meaning something like this: JOHN    SMITH    12343rd    StAnytown PA12345 is read in my knowing that the first name field is 8 characters wide, the last name field is 8 characters wide, the street number is 4 digits, and so on.

It's also a terrible schema, and woe to anyone with a long name. But many a mainframe had a similar schema.

Now, let's think about maintenance here. What happens when we also want to store a middle initial? We've created for ourselves a problem. Somehow, I have to insert a character into every row, which basically means making a new table with a new schema, copying every record out of it and updating it to use the new schema. I can't just ALTER TABLE like an RDBMS. And worse, every piece of software that touches the table also needs to be updated. On a large legacy system, a simple task like "add a field to our database" could take weeks of developer time, and depending on the software, be a high risk operation.

Which is why the smart developer, when working with flat files, includes padding. Maybe my schema for an address record looks more like this: JOHN    SMITH    12343rd     StAnytown PA12345                . That's 16 characters of padding at the end of the file. Now somebody says that I need to store a middle initial, I can just shrink the padding by one and add a middle initial field, like so: JOHN    SMITH    12343rd     StAnytown PA12345Q               

Is this elegant? No. But it works. I haven't changed the length of the row at all, so I don't need to move data around. Software modules only need to be updated if they care about what's in the middle initial field; if they're out of date, they just think there's a "Q" in the padding, and don't care.

In real-world applications, instead of putting all the padding at the end, you'd usually put the padding in a few spots in the middle of the table. Any time you need a new column, you just steal a few characters from padding. Sure, someday you'll run out of padding, or at least out of padding blocks big enough for your new field, and then you'll have to do the hard work of shuffling data around. But in practice, you can get very far without that happening.

Which brings us to Brenda's adventure. Her team supports an IBM mainframe storing data in VSAM flat files. In other words, they've been doing the sort of thing I just talked about for many, many years.

Of course, in the modern era, you can't just leave your data sitting in an mainframe. Even if the mainframe is the source of truth, you want to be able to report on it and connect it with your other data systems. You need to, somehow, get the data into a modern RDBMS.

So the company hired a bunch of developers to write an extract-transform-load process, which pulls the data out of the mainframe. The mainframe team handed them a "copybook" for the flat file, which described the structure, and the ETL devs went to work.

And maybe those ETL devs didn't understand the importance of padding. Maybe they just missed the padding. Whatever it was, there were several places where the data was structured like SOME_USEFUL_FIELD PADDING PADDING PADDING SOME_OTHER_FIELD, and they opted to split it like so: SOME_USEFUL_FIELD PADDING PAD, DING PADDING SOME_OTHER_FIELD.

When they released this process, it was fine. The padding characters got stripped before displaying, so the users never saw them. They were stored in the database, though, so when someone tried to reconstruct the data in a way that was compatible with the flat files, you could just concatenate the columns together and get a valid result.

It was fine- until it wasn't. The ETL devs, bless their hearts, only tested against the production mainframe. And why not, they were doing read only operations, what's the harm? Had they tested against the development mainframe, they would have seen new features in flight, features which consumed some of that padding, and realized that they should have paid closer attention to the copybook.

But instead, the test cases all passed. The software was, as far as the project managers and ETL developers could tell, working perfectly. So it was accepted, released to production, and running for a few weeks before the mainframe released its features. Those features then ruined all the beautiful reports with extraneous data.

And since the ETL devs were on contract, any request to have them rework it under the original contract was met with a stern "Works as designed". Instead of paying the contractors to come back and rework the system, the mainframe devs instead were tasked with finding different padding fields they could use, padding fields which wouldn't end up ruining any reports management liked to see.

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

Honestly, with the wildfire smoke and the oppressive heat, maybe it's time to find a nice quiet cave to hang out in. Something with no natural light and no natural ventilation. I wonder if anybody has a place like that… Original. --Remy

In the long ago, wild-west days of the late 90s, there was an expectation that managers would put up with a certain degree of eccentricity from their software developers. The IT and software boom was still new, people didn't quite know what worked and what didn't, the "nerds had conquered the Earth" and managers just had to roll with this reality. So when Barry D gave the okay to hire Sten, who came with glowing recommendations from his previous employers, Barry and his team were ready to deal with eccentricities.

Of course, on the first day, building services came to Barry with some concerns about Sten's requests for his workspace. No natural light. No ventilation ducts that couldn't be closed. And then the co-workers who had interacted with Sten expressed their concerns.

During the hiring process, Sten had come off as a bit odd, but this seemed unusual. So Barry descended the stairs into the basement, to find Sten's office, hidden between a janitorial closet and the breaker box for the building. Barry knocked on the door.

"Sten awaits you. Enter."

Barry entered, and found Sten precariously perched on an office chair, removing several of the fluorescent bulbs from the ceiling fixture. The already dark space was downright cave-like with Sten's twilight lighting arrangement. "He welcomes you," Sten said.

"Uh, yeah, hi. I'm Barry, I'm working on the Netware 3.x portion of the product, and Carl just wanted me to check in. Everything okay?

"This is acceptable to Sten," Sten said, gesturing at the dim office as he descended from the chair. Sten's watched beeped on the hour, and Sten carefully placed the fluorescent bulb off to the side, in a stack of similarly removed bulbs, and then went to his desk. In rapid succession, he popped open a few pill containers- 5000mg of vitamin C, a handful of herbal and homeopathic pills- and gulped them down. He then washed the pills down with a tea that smelled like a mixture of kombucha and a dead raccoon buried in a dumpster.

"He is pleased to meet you," Sten said, with a friendly nod. Barry blinked, trying to track the conversation. "And he is pleased with it, and has made great progress on building it. You will like his things, yes?"

"Uh… yes?"

"He is pleased, and I hope you can go to him and tell him that he is pleased with this, and set his mind at ease about Sten."

So it went with Sten. He strictly referred to himself in the third person. He frequently spoke in sentences with nothing but pronouns, and frequently reused the same pronoun to refer to different people. The vagueness was confounding, but Sten's skill was in Netware 2.x- a rare and difficult set of skills to find. So long as the code was clear, everything would be fine.

Everything was not fine. While Sten's code didn't have the empty vagueness of unclear pronouns, it also didn't have the clarity of meaningful variable names. Every variable and every method name was given a female first name. "Each of these is named for one of Sten's girlfriends." Given the number of names required, it was improbable that these were real girlfriends, but Sten gave no hint about this being fiction.

There was some consistency about the names. Instead of i, j, and k loop variables, you had Ingrid, Jane, and Katy. Zaria seemed to be only used as a parameter to methods. Karla seemed to be a temporary variable to hold intermediate results. None of these conventions were documented, obviously, and getting Sten to explain them was an exercise in confusion.

It led to some entertaining code reviews. "Michelle here talks to Nancy about Francine, and then Ingrid goes through Francine's purse to find Stacy." This described a method (Michelle) which called another method (Nancy), passing an array (Francine). Nancy iterates across the array (using Ingrid), to find a specific entry in the array (Stacy).

Sten lasted a few weeks at the job. It wasn't a very successful period of time for anyone. Peculiarities aside, the final straw wasn't the odd personal habits or the strange coding conventions- Sten just couldn't produce working code quickly enough to keep up with the rest of the team. Sten had to be let go.

A few weeks later, Barry got a call from a hiring manager at Initrode. Sten had applied, and they were checking the reference. "Yes, Sten worked here," Barry confirmed. After a moment's thought, he added, "I suggest that you bring him in for a second interview, and have him walk you through some code that he's written."

A few weeks after that, Barry got a gift basket from the manager at Initrode.

Thanks for the tip

Sten did not get hired at Initrode.

Karen was maintaining some specification tests that were flaky. Not extremely flaky, but three or four times out of a thousand, the tests would just fail. The tests were complicated, and some of the operations were timing sensitive, so it wasn't precisely surprising- but the problem was that they were actually generous with their timing windows. The unit tests passed consistently, it was only these functional, specification-based tests that failed.

So, for example, there were sections in the tests where they wanted to wait at least 2ms. Since the code and tests were in TypeScript, they used the setTimeout function, which per standard JavaScript documentation warns that it may wait longer. But again, Karen was fine with longer.

Unfortunately for Karen, the documentation for NodeJS is less specific, as it makes no guarantees about when the timeout function gets invoked. This means that it can fire the timeout before the time has elapsed.

After many, many hours of debugging, that was exactly the situation that Karen found herself in. Which is why her very simple wait function went from:

export const wait = (ms:number) => new Promise((complete) => setTimeout(complete, ms));

To the much more awkward:

export const wait = (ms: number) {
    const target = performance.now() + ms;
    return new Promise((complete) => {
        const checkReady = () => {
            if (performance.now() > target) {
                complete();
            } else {
                setTimeout(checkReady, 1);
            }
        }
        setTimeout(checkReady, 1);
    });
}

This version of the function checks the time every millisecond, and only completes the operation if we've waited at least as long as our target duration. This ensures that the timeout never fires too soon and it fixes the janky tests. But it's also terrible. Terrible that it exists. Terrible that this is the best solution. Terrible that our functional tests need to be so time sensitive. And terrible that the Node runtime actually breaks the one consistent scheduling guarantee that pretty much every other scheduler does: that it'll wait at least as long as you asked, but might wait much longer.

At best, we can say, "at least it's only testing code."

Today's submission is less a WTF and more a, "Yeah, that'd annoy me too."

Stevie works in a code-base that's largely C, which means function return values are usually used to communicate to status codes. The standard:

BOOL success = someFunc();
if (!success) {// handle the error

If someFunc returns TRUE, we succeeded, otherwise we failed.

There's nothing wrong with that convention. But there is something wrong with one of the long-time developers on the project, because they have their own idiom for doing this. And they've been around long enough that their approach is the convention other developers follow. It's not wrong, per se, just confusing:

BOOL error = someFunc();
if (error == FALSE)
{
    //handle the error
    errorCode = GetLastError();
    //…
}

Yes, pretty much anywhere an error can happen, they check if error == FALSE, and if that's true, they have an error.

I'd say, "at least they're consistent", but they're not. It's the convention, sure, but nobody wrote this down as the convention. New developers come in all the time, and they start out writing code in a more "normal" pattern. But the existing code has its pattern, and there is a lot of it. It has a mass and an inertia that is stronger than any developer. They don't change the code, the code changes them.

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

Jason M sends us some Ruby code.

def bv(prop, tv="Yes", fv="No", nv="Not Specified")
  v = self.send(prop)
  if v === true
    tv
  elsif v === false
    fv
  else
    nv
  end
 end

The obvious WTF here is the function name and parameter names. AAYFN(always abbreviate your function names) seems to be the convention here. But it also contains a more Ruby-specific WTF.

The function name bv is short for "boolean value", obviously. tv is the "true value", fv is the false value, and nv is the null value. So this is really about pretty-printing boolean values and converting them to strings. While the terrible names make it hard to understand, it's not that hard to figure out what's going wrong here. I hate it, don't get me wrong, but it just makes me sigh with disappointment, not groan.

No, the thing that makes me groan is v = self.send(prop). This is a very Ruby idiom that lets you access a member of the class by name; essentially it's like doing self.prop, but since prop is a variable containing a property name, we have to send it.

This is metaprogramming by strings, which is the main reason I end up hating it. But in this specific instance, it offers us a lot of potential issues. First, if prop is anything not boolean, we just return "Not Specified", which is incredibly misleading. I'd argue that if we attempt to use it on a non-boolean field we should throw an exception. Which opens the question: if prop doesn't exist, is that a non-boolean field, or is that a "enh, just call it null" situation? Because right now, that will throw an exception. It'll also throw an exception if the thing being accessed is a function that takes parameters. These behaviors may be surprising.

Now, I don't know the calling pattern. It's possible that whatever function calls this already has a good list of the allowed boolean values, and will never call this on a prop that doesn't exist. Certainly, that's what the Ruby docs recommend. But I'm going to hate it anyway, because this kind of runtime metaprogramming by passing strings around is eternally asking for trouble. And while I haven't done a huge amount of Ruby, I've done enough to know that any non-trivial codebase ends up like this once the metaprogramming band-aid is taken off.

Now, if you don't mind, I'll go back to doing my metaprogramming with C++ templates, which are simple, clear, and never result in wildly unmaintainable code because you ended up reimplementing LISP in template operations.

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

While a project manager is frequently called upon for their planning ability, the real skill we want from project managers is their ability to communicate. The job of a project manager is to align the team doing the work, with the organization goals driving the work, with the management and leadership teams trying to understand the work, while juggling all the constraints like budgets, timelines, and the endlessly changing expectations for the project. A good project manager is worth their weight in gold. A bad one will cost their weight in gold.

Mark was hired on as a contractor, reporting to Tegan. Tegan was fresh out of business school, complete with an MBA and a variety of project-management training certifications. Unfortunately for Mark and the rest of the team, and especially unfortunately for Tegan, she had absolutely no real world experience. To make matters worse, this wasn't just a software project: they were working on a system which matched newly developed software with newly designed mechanics and custom build control electronics. A group of experienced software engineers, mechanical engineers, and electrical engineers all found themselves reporting to a bright and shiny MBA. It's a role that she probably could have grown into, but management saw all the acronyms she continuously put after her name, and decided she could just take the whole thing over with no real guidance.

It went badly pretty much from the beginning. Tegan was not a talented communicator. For example, Mark's team needed to know: on what timeline were the electrical engineers going to deliver the first prototypes, so the software team could start running bench tests of their software? Tegan's response was a fortune cookie message about balancing the complicated pipelines and lanes on the Gantt chart and hitting all of their milestones; like a fortune cookie, it was vague, important sounding, but ultimately empty.

Of course, the natural reaction amongst the engineers was to just route around the damage: the various teams could talk to each other just fine without going through Tegan. That, unfortunately, did not go over well with management. Tegan, as the project manager, was their insight into the project. They needed her in the loop on everything. And she couldn't just be informed, she had an MBA. She needed to be making decisions. But she was unqualified to make those decisions, which meant the project gradually ground to a halt. Tegan's emails got more vague, her meetings got longer but accomplished less, and after a certain point, she just stopped replying to key email threads.

The first few days of radio silence seemed like a gift. But as time passed and Tegan seemed uninterested or unable to reply to any of the questions the team had for her, the project started to flounder. The engineering teams escalated this problem to management. Management presumably went back to Tegan. At some point, feeling the weight of everything going wrong around her, Tegan sent out this email, which is definitely the best and clearest communication she managed during the project. It's arguably the clearest, and most accurate communication one could make in this situation:

Team,
I understand all the issues but there are complex interrelations that must be worked out. I am currently constipated on each issue and will let you know when there is movement.

• Tegan
  MBA, CAPM, PMP

Her email cost the project many person-hours as all the engineering teams took a break to have a good laugh about the project manager admitting, in writing, that she was full of crap.

There was, eventually, movement. Tegan moved on to a new position at a different company. Her replacement, Pam, wasn't a new hire, but instead a transfer from another department. She wasn't a great project manager, certainly not worth her weight in gold, but she had enough experience to avoid the worst mistakes, and most important: she was good at regular communication in order to keep things moving.

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

TJ inherited a NestJS project. The original developers left the team many years ago, but they've left their mark in the codebase.

// ProjectsModule.ts
@Module({
  controllers: […],
  providers: […],
  exports: […],
})
export class ProjectsModule {}

NestJS is a dependency-injection oriented framework for TypeScript code. It offers "providers" (dependencies that can be injected), "controllers" (as one would expect), and lets you bundle them together into "modules". Modules can depend on other modules, letting you build a modular and flexible graph of dependencies. This means that the empty module isn't wrong here.

No, for it to be wrong, we need to write some tests:

// ProjectsModule.test.ts
describe("ProjectsModule", () => {
  it("can be created", () => {
    const projectsModule = new ProjectsModule()
    expect(projectsModule).toBeTruthy()
  })
})

Since modules are just containers for related code objects, there isn't much to test here. While "dynamic modules" which execute code are a thing, they don't execute that code at construction time anyway. This test will always pass. It isn't a test, it doesn't do anything. It likely doesn't even get their coverage up, since whatever providers or controllers it's referencing aren't covered by this test. It's a test that tests nothing but the framework it runs on top of.

"At least there are tests," TJ writes.

"Dragoncoder" supports a web application that has a "wait time" for access. I hate that that's a thing, but I recognize that there are real-world constraints where this might make sense. Still, I hate it. But that's not the WTF.

      var minutes = parseInt( 12 , 10);
      var time = document.getElementById('waitTime');

      if ( minutes < 2) {
        time.innerText = "Your estimated wait time is 12 minute."
      } else if (minutes < 60) {
        time.innerText = "Your estimated wait time is 12 minutes."
      } else if (minutes === 60) {
        time.innerText = "Your estimated wait time is 0 hour."
      } else if (minutes < 120 && (minutes % 60 === 1)) {
        time.innerText = "Your estimated wait time is 0 hour and 12 minute."
      } else if (minutes < 120) {
        time.innerText = "Your estimated wait time is 0 hour and 12 minutes."
      } else if (minutes > (60 * 4)) {
        time.innerText = "Your estimated wait time is more than 4 hours."
      } else if (minutes % 60 === 0) {
        time.innerText = "Your estimated wait time is 0 hours."
      } else {
        time.innerText = "Your estimated wait time is 0 hours and 12 minutes."
      }

This wait time page is initially rendered by their backend, but after that point, gets served up by a cache at the edge of their CDN. That makes sense, since "have the users hammer your backend while they're waiting" is a bad idea.

Note the line var minutes = parseInt( 12 , 10);. This is rendered from the backend, which is of course my least favorite way to send data from the server side to the client side.

But that's not the core problem here. The core problem is: what the hell are they outputting?

If your wait time is less than 2, or less than 60, we tell you that your wait time is 12 minutes. Or "12 minute", because who cares about pluralization? If your wait time is exactly 60 minutes, we tell you that your wait time is "0 hour", which I assume means you'll have enough time to watch the classic airplane disaster movie, Zero Hour, which you surely know Airplane! is a remake of.

I can only think that the text is also being generated by logic on the server side- though our submitter doesn't suggest that's the case. Though they do wonder why the code couldn't be something like: Your estimated wait time is: ${Math.floor(wait_time_minutes / 60)} hours and ${wait_time_minutes % 60} minutes, which is both fewer bytes to send from your cache and more useful to the end user.

Or maybe we just make this wait time go away. Again, I don't know why it's there, there may be a good real-world constraint that requires it, but… is there? Is there really?

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

As the US took this weekend to celebrate their complicated relationship with tyranny, we reach back through the archives for another story of tyrants. If you think about it, the Declaration of Independence is basically the same thing as quitting without notice. Original --Remy

It was Steve's first week on the job, and he had plenty of questions about the code base and the new features he was supposed to implement. He muddled through for most of the week, but Friday morning he hit a brick wall and needed to talk to Bill, the architect.

"Can I meet with you for like an hour to go over things?" Steve asked.

"No."

"Can I get half an hour then? I h-"

"No. Company meeting, every Friday, 12-5pm. It should be on your calendar. I'll forward the invite."

Bill also couldn't free up time in the morning, so that meant Steve was stuck until Monday afternoon. Still, it probably wasn't all bad. He assumed that since this was a small company, in startup mode, it was going to be one of those meetings that was less meeting and more party. He had heard about one company in town that had a kegger every Friday afternoon.

Steve really should have known better. During his interview, the actual technical questions were thin on the ground. It focused more on "soft skills", like time management. He fielded a lot of questions about how best to manage his time. The other question that really stuck out in his mind was the standard, "Have you ever had to deal with a difficult personality in the workplace? How did you deal with it?" It was memorable, less because the question itself was unusual, but because at least six variations of the same question showed up in the interview.

On his very first day, he learned who the difficult personality was: Frank, the boss and grand-high pooba of the dev team. Around 2PM Frank lumberghed himself into Steve's cube. "Yeah, we've got a little problem," Frank said. "I've noticed you spending a great deal of time in the break room."

"Oh, yeah, I was just going back for more coffee," Steve said with an awkward laugh. "You know how it is with programmers- we're fueled by caffeine."

"Yeah, well, if you could just go ahead and make sure you're at your desk doing work, that would be great."

As it turned out, Frank had gone easy on Steve because it was Steve's first day. The next day, Steve sat in on Bill's planning meeting- a 4-hour marathon to organize the development backlog and parcel out work. Halfway through, Bill called for a break. He and a few other co-workers darted outside to gradually commit suicide via cancer, while everybody else hung around the room committing suicide by donut. And then Frank walked in.

"What's happening?"

"It's um… just a little break," one of the devs replied. "Bill's outside."

"I see." Frank loitered in the room until Bill returned. The instant Bill's foot crossed the threshold of the meeting room, Frank's human facade was stripped away, and a spitting, slavering demon replaced him. He proceeded to dress Bill down, back up and right back down for disrespecting his team, disrespecting the company, disrespecting Frank and Frank's poor elderly mother with his attitude. He closed with, "They're developers and I want them sitting around and developing! Not waiting for you to finish your smoke breaks!"

On Thursday, Steve got to drive a meeting to show off the latest batch of features the dev team had completed. When he turned on the projector, Frank asked, "What's wrong with your computer?"

"Um… nothing?"

"The desktop is wrong! None of the icons are in the right place!"

Like most developers, Steve had changed the wallpaper and reorganized his desktop to suit his working style. Unfortunately, his transgression against the default desktop settings set Frank off on a long rant that consumed the entirety of the meeting. Steve was lucky, Frank claimed, that he wasn't fired on the spot. Standard work was vitally important, and personalization was frowned upon. "It's vitally important that any developer can use any other developer's computer- we can't afford to waste a minute of time just because you needed to be a special little snowflake!"

By the time the Friday afternoon meeting rolled around, Steve should have been expecting some kind of Frank-led time management course. Instead, Bill handed him a mop. "New blood gets mop duties. Start in the break room, and then hit the other common areas."

"Excuse me?"

"Frank's orders. Every Friday, we spend the afternoon cleaning the office, from top to bottom."

"There isn't a cleaning crew?"

"Oh, there is," Bill said. "Frank doesn't trust them to do a good job." That weekend, Steve decided to take Frank's lessons on time management to heart, and immediately left to seek employment that didn't involve wasting his time.

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

When Gary was called in for a meeting with a few of his managers- because of course he had several- he thought it was going to be for an "attaboy", because things had been going really well for the past few months.

Gary had inherited a mess, and taken over a nightmare application. It was the kind of application that should be a simple CRUD-style data-driven app, but somehow despite only having 20ish entities it managed, someone had generated 500+ controllers for managing them. Most of those controllers were copy/pasted code with minor changes in the WHERE clause of a SQL query.

And that was just the code. The infrastructure was similarly a mess, with duplicate resources provisioned in their cloud host. There was no CI/CD, no unit tests to speak of, no deployment process that wasn't "manually copy these files and pray". And uptime? You've heard about "five nines", but this product was lucky to get even one nine. Especially because the manual deployment process meant a few hours of downtime.

And that was just the infrastructure. The backlog was similarly messy. There were lots of tasks- many thousands- but not a single one had a priority. Most of the tasks were something like, "Fix database timeouts", or "Bug 531" with no description to explain what they were. At best, some of the "new feature" tasks linked to a Google Doc that explained a software roadmap that had been last updated in 2020.

So with no guidance, Gary and the rest of his team got to work. Cloud costs were massive. Just cutting the duplicate resources would help, but with actual planning it wasn't hard to find even bigger wins. In total, Gary got the cloud costs down 60%- essentially saving the company a small multiple of his salary every year.

With that out of the way, getting a CI/CD pipeline running was next. Within a few weeks, manual deployments were gone. Everything was automated. Downtime nearly vanished. And now, with all the cost savings in cloud resources, for a fraction of what they were paying, it was easy to automate provisioning test environments for each new feature.

So Gary was very ready for some congratulations when he sat down with management. He was prepared to discuss all the wins he and the rest of the developers on the project had gotten over the past few months.

"I'm sure you know why we're sitting down," Manager the First said when they settled into the conference room.

"I'm sure," said Manager the Second.

"We have some concerns about your performance," Manager the Third said.

"My performance?" Gary asked.

"Yes," said Manager the First. "Let me pull up the backlog."

"And the roadmap," said Manager the Second.

"Yes, I'm getting that up too, thank you." The trio of managers struggled with pulling up the appropriate pages, and after about 15 minutes, gave up. Instead, they discussed their complaint without visual aids. "You haven't completed any of the tasks on the roadmap. Bug 673 has been open since you started on the team. None of the roadmap milestones have been touched. There's absolutely no progress."

"Okay, but that document was wildly out of date," Gary said. "Instead I put cycles into solving the actual problems we're having. I've saved the company a huge amount of money. I've gotten our development cycle time down to a fraction of what it was. And we have basically no downtime!"

"That's all very nice, I'm sure," said Manager the Third. "But none of that was on our roadmap."

"Well, maybe we should set up a meeting to go over the roadmap," Gary said. "Because a lot of the tasks on there don't make much sense right now-"

"I don't think that's a good use of time," Manager the Second said. "Large meetings are expensive. Just stick to the roadmap, please."

With that, the meeting ended. Gary went back to work…

… updating his resume.

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

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.

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

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.

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

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')
end

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

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

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_total

In 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?

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

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

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

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

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

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 result

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

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

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.

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

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.

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

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.

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

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.

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.

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.

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

Are there any files to send? That's the question that Chris C's predecessor had. So they asked it. Again. And again. And again.

Chris writes:

I'm occasionally called upon to troubleshoot an ecommerce application that was built in the PHP 5.x days and has been running largely untroubled by maintenance or modernity (aside from the backported security patches to its binaries) ever since.

if(sizeof($files) > 0){
		if(sizeof($files) > 0){
				foreach($files as $file){
						$mime->addAttachment($file);
		}
		}
}

Indentation as per the original.

If the files array contains items, then if the files array contains items, then we iterate across the files array, which hopefully contains items, and add them as an attachment to an email.

I feel like the way this got indented, the developer responsible knew, deep down, that this was wrong. They lacked the reading comprehension to understand why, but deep down in their spleen, something was screaming at them. And thus those stacked curly brackets at the end there.

Of course, none of the conditionals are needed: a foreach on an empty object just does nothing.

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

Delilah works in a Python shop. Despite Python's "batteries included" design, that doesn't stop people from trying to make their own batteries from potatoes. For example, her co-worker wrote this function:

def key_exists(element, key):
    if isinstance(element, dict):
        try:
            element = element[key]
        except KeyError:
            return False
        return True

Python, of course, has an in operator. key in dictionary is an extremely common idiom. There's no reason to implement your own. Certainly, there's no reason to re-implement it by catching and throwing exceptions.

This is ugly, stupid, and bad. It gets worse, though, when you see how it gets used.

for key in old_yaml_data:
    if key in new_yaml_data:
        if old_yaml_data[key] != new_yaml_data[key]:
            temp = new_yaml_data[key]
            new_yaml_data[key] = merge(new_yaml_data[key], old_yaml_data[key])

            if key_exists(new_yaml_data[key], 'image') and key_exists(old_yaml_data[key], 'image'):
                new_yaml_data[key]['image'] = temp['image']
            elif key == "databases":
                revert_db_tags(new_yaml_data[key], temp)

This code is attempting to upgrade "old" YAML data with "new" data. So it's basically merging dictionaries, which is a great case for the in operator.

And they use the correct idiom on the second line there! This was written by one developer! They do the standard key in new_yaml_data check. And they also use key_exists. I can only assume that they had a stroke between starting and finishing this script, which I'll note is, in total, 48 lines long.

Here's the whole short script, which is just generally a mess. Slapped together Python code that's trying to be a "smarter" shell script, but is definitely written with the elegance of hacked-together-bash.

import sys
import yaml
from jsonmerge import merge

appHomePath = sys.argv[1]
oldValuesYAML = appHomePath + "values.yaml"
newValuesYAML = appHomePath + "/upgrade_version/values.yaml"
with open(newValuesYAML, 'r') as f:
    new_yaml_data = yaml.load(f, Loader=yaml.loader.FullLoader)
with open(oldValuesYAML, 'r') as f:
    old_yaml_data = yaml.load(f, Loader=yaml.loader.FullLoader)
def key_exists(element, key):
    if isinstance(element, dict):
        try:
            element = element[key]
        except KeyError:
            return False
        return True

def revert_db_tags(old_yaml_data, new_yaml_data):
    dbList = ["mongoDB", "postgresDB"]
    mongoDbTagsToRevert = ["mongoRestore"]
    mongodbKeysToDelete = []
    postgresDbTagsToRevert = []


    for db in dbList:
        old_yaml_data[db]['image'] = new_yaml_data[db]['image']
    for mongoDbTag in mongoDbTagsToRevert:
        old_yaml_data['mongoDB'][mongoDbTag]['image'] = new_yaml_data['mongoDB'][mongoDbTag]['image']
    for mongoDbTag in mongoKeysToDelete:
        del old_yaml_data['mongoDB'][mongoDbTag]

    for postgresDbTag in postgresDbTagsToRevert:
        old_yaml_data['postgresDB'][postgresDbTag]['image'] = new_yaml_data['postgresDB'][postgresDbTag]['image']

for key in old_yaml_data:
    if key in new_yaml_data:
        if old_yaml_data[key] != new_yaml_data[key]:
            temp = new_yaml_data[key]
            new_yaml_data[key] = merge(new_yaml_data[key], old_yaml_data[key])

            if key_exists(new_yaml_data[key], 'image') and key_exists(old_yaml_data[key], 'image'):
                new_yaml_data[key]['image'] = temp['image']
            elif key == "databases":
                revert_db_tags(new_yaml_data[key], temp)

with open(newValuesYAML, 'w') as f:
    data = yaml.dump(new_yaml_data, f, sort_keys=False)

A depressing quantity of software is what I would call a "data pump". I have some data over here, and I need it over there. Maybe I'm integrating into a legacy app. Or into an ERP. Or into a 3rd party API. At the end of the day, I have data in one place, and I want it in another place.

Sally has a Java application written in the Quarkus framework, which has a nightly batch that works to keep a table of Bar entities in sync with a table of Foo entities. (This anonymization comes from Sally) These exist in the same database. There is also a Bar webservice, which provides information about the Bar entities. The workflow, such as it is, is that the software needs to find all of the Foo entities that do not currently have associated Bar entities, and then call the Bar webservice to get the required information to create those Bar entities.

Let's see how that works.

@Inject UserTransaction transaction
// If this is annotated with @Transaction the usage in the Message function down below will have some Thread exception
public List<FooData> getAllFoos() {
  try{
    return fooDataRepository.findAllFoos();
  } catch (Exception e) {
    throw new RuntimeException(e);
  }
}

We'll worry about that comment in a second, but this function returns a list of all of the Foo objects in the database. It does not return a list of all the Foo objects without associated Bar entities. It's just the whole giant list of everything. The underlying database is a standard relational database; it'd be trivially easy to write that query, even going through the ORM.

Well, that's bad, but it's all pretty minor. How does the actual update go?

// Can't be annotated with @Transaction because Oracle DB can handle the given Amount of dataEntities in one Transaction '\._./'
Message updateBarsWithFoos() {
  List<FooData> foos = getAllFoos();
  if(!foos.isEmpty()){
    foos.forEach(foo -> {
      try{
        transaction.begin();
        if(barRepository.findByName(foo.getName()) == null){
          if(barDataService.searchByName(foo.getName()) != null && barDataService.searchByName(foo.getName()).marker() != null){
            barRepository.createBar(barDataService.searchByName(foo.getName()));
          }
        }
        transaction.commit();
      } catch (Exception e) {
        try {
          transaction.rollback();
        } catch (Exception ex) {
          throw new RuntimeException(ex);
        }
      }
    });
  }
  return new Message(MessageLevel.INFO, "Created bars")
};

Ah, the real WTF is that it's an Oracle database. That's always a WTF.

But let's trace through this code.

We get all of our Foo entities. We check for emptiness and then do a forEach, which seems to make the empty check superfluous: a forEach on an empty list would be a no-op anyway.

We start a transaction, then check the database: if there are no Bar objects that link to Foo, then we call into the barDataService to find data. If there is, we call into the service again, to see if the marker property is not null. If it is, we call into the service again to get the actual data we're putting into the database. Then we close the transaction. If anything goes wrong, we rollback the transaction and chuck an exception up the chain.

That is three web service calls inside of a database transaction. Three calls which could easily be one, and that call could easily also happen outside of a transaction if you're mindful about confirming your constraints. And of course, because they're not mindful at all, they need to manage the transaction directly, and can't use the @Transaction annotation provided by their framework, which would at least cut down on some of the boilerplate.

Now, I'm sure you'll be shocked - shocked - to learn that the webservice is actually a bit flaky, and thus times out from time to time. And this isn't the only batch job running, which means the long-lived transactions cause all sorts of contention and terrible performance across the various batches. And this app doesn't have its connection pool properly configured, so the entire software stack can exhaust all of its database connections surprisingly quickly, causing yet more failures.

The root of the WTF, of course, is doing this as a batch job. A well engineered application would do everything it could to not create data in the database that isn't referentially sound. There, Sally gives us the one bit of good news:

My current project will do away with the batch processing altogether, so we can say, "RIP, transactional wholesale triple caller!"

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

JB has a database table that, at first glance, looks like one of those data warehouse tables that exists to make queries performant. You know the sort, the table that contains every date between 1979 and 2050, or every number out to 1,000,000 or something. It looks dumb, but it helps make certain joins and queries performant.

The database table is called three_alpha_numerics. It has two columns: digit, which contains three characters, and is_numeric, which is a a single character: 'Y' or 'N'. It looks roughly like this:

+-------+------------+
| digit | is_numeric |
+-------+------------+
| 009   | Y          |
+-------+------------+
| 00A   | N          |
+-------+------------+

So, for example, if you wanted all the possible numeric triples, you could SELECT digit FROM three_alpha_numerics WHERE is_numeric = 'Y', which is obviously the easiest thing one can imagine.

So what is this for? Well, it's used by a stored procedure that generates unique IDs. That stored procedure does a left join against another table to find all the unused digits. And here's the real gotcha: that stored procedure only ever uses the rows where is_numeric is Y, meaning the vast majority of the data in this table is never used.

Unique IDs, of course, are an incredibly difficult task for databases to do, so it absolutely makes sense that we create a system that allows us to only have 1,000 unique IDs. That's more than 640, which should be enough for anyone. Having many thousands of unusable alphanumeric triplets is just the cost we have to pay.

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

Another representative line, and this one comes from an Excel spreadsheet. But, per Remy's Law of Requirements gathering ("No matter what the requirements doc says, what your users wanted was Excel"), this one was actually written by a developer. A developer who didn't understand how Excel works, but more important, didn't understand how dates worked either.

This comes from Ulysse J.

=CONCATENER(SI(MOIS($A18)>9;ANNEE($A18)-2000;(ANNEE($A18)-2000)*10);SI(JOUR($A18)>9;MOIS($A18);MOIS($A18)*10);JOUR($A18))

Now, the first thing: Excel function names are locale specific. This was written in France, so the functions are French. CONCATENER is "concatenate", SI is "if", MOIS is "month", and so on.

The purpose of this function is to convert a field (cell A18) in DD/MM/YYYY into YYMMDD. So how does it do this?

Well, we check the month. If it's greater than 9, we output the year minus 2000. If it's less than 9, then, we output the year minus 2000, multiplied by 10. That is to say, August, 2026 would start by outputting 260. We repeat this logic for the days: if the day is larger than 9, we output the month, otherwise we output the month times 10. Finally, we output the day.

This is attempting to do padding. There's just a problem. Imagine February 1st, 2009- an actual date in the document. We convert the year into 90, the month into 20, rendering the date as 90210. That's incorrect. And once we get to 2100, if there is still an Excel in 2100 (I joke: of course Excel will still exist in 2100. Humanity won't, but the robots will use Excel), this will also break. Not that it matters- I mean, YYMMDD doesn't make sense by that point.

Obviously, the correct solution is to use Excel's rich, built-in formatting functions to convert between date formats. It's easy! But Ulysse raises another point:

Extra points: even if you do not know how to do proper [formatting], the input format is guaranteed to have correct padding. I would just concatenate parts of it (treating dates as text is bad, but still less bad than treating them as integer triplets).

I will say this: I know a software developer wrote this, because your average Excel user could easily write bad formulas, but never bad in this kind of convoluted way. You need a real expert to do something this bad.

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

Today's anonymous submitter sends us two blocks. The first is a perfectly normal line of React code:

const [width, setWidth] = useState(false)

This creates a width variable, defaulting it to false, and a setWidth function, which lets React detect when you change the variable, and trigger a re-render. Importantly, this mutation only happens on the next render, which means if you call setWidth and then check width, you won't see your change happen.

As I said, this is perfectly normal React code. Well, almost. First, I have to ask: why on Earth is width being set to a boolean value? "How wide are you?" "Yes." It's possible that there's a good reason for this, though I suspect that it's unlikely.

The second issue, however, is that the linter complained that the setter was never actually used. That was odd, because if our submitter grepped the codebase, there were two calls to setWidth. Let's see what that looked like:

const show = (show) => {
    setWidth(show)
    setWidth(!show)
}

We create a function show, where we expect a boolean value, and then we setWidth with that value, and then with the negation of that value. So show(true) will set width to be false. To make matters more confusing, we set width both ways, and I assume this is someone trying to get around React's state management. React won't trigger a re-render if you set the state to a value it already has. So I suspect they're twiddling to try and force it to re-render, and I also suspect that this might not work? Even if it does, this isn't how you should be using React. As I said, I'm no React expert, but as the saying goes: "I don't have to be a helicopter pilot to know that when I see a helicopter hanging upside down from a tree someone messed up."

Our submitter writes:

Got hired to cleanup a mission critical website for a company that had just learned that offshore teams might not be worth the cost saving measures.

"Pay me now or pay me later."

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

As previously discussed, C++ took a surprisingly long time to get a "starts with" function for strings. It took even longer to get a function called "contains". In part, that's simply because string::find solves that problem.

Nancy sends us a… different approach to solving this problem.

bool substringInString(string str, string::iterator &it)
{
  string tmp;
  bool result = false;
  int size = str.length();

  int count = 0;
  while (count < size)
  {
    tmp += *it;
    it++;
    count++;
    if (tmp.find(str) != string::npos)
    {
      result = true;
      it -= size;
      break;
    }
  }

  if ( !result)
  {
    it -= size;
  }

  return result;
}

This function iterates across a string, character by character. In this iteration, we copy one character at a time into tmp. Then we see if tmp contains our search str. If it does, we break out of the loop after rewinding the iterator. Outside of the loop, we check if we found the substring, and if we did, we rewind the iterator. Then we return true or false based on whether on not we found the substring.

So wait a second. str is our search string. it is where we're searching. And we copy from it up to our search string's length into a temporary string. We then do a find in that temporary string- hey! This is just a startsWith check written in the most insane way possible.

Why even bother with the while loop? While tmp is shorter than the search string, the answer is always "no, we haven't found it". And the developers knew that- that's why they always rewind size characters on the iterator. They're always searching exactly that many characters. Of course, since we always rewind the same amount, we can also just move the it -= size statement out of the loop and out of the if statement and do it once.

Nancy calls this "a little gem" in a "large codebase". Yeah, a real gem.

Today's anonymous submitter sends us some code that just makes your mind go… blank when you look at it.

	public static boolean isNull(String value) {
		return StringUtils.isBlank(value);
	}

StringUtils.isBlank comes from the Apache Commons library. It's a helper function for Java which returns true if a string is, well, blank. "Blank" in this case is: empty, null, or only whitespace. So it's important to note that isBlank may return true on a null, but it isn't truly a null-check, so wrapping it in isNull is just confusing.

But imagine I've got another problem. Let's say I have a database that's been poorly normalized and maintained. And so I have a bunch of fields that maybe are null, but some also maybe contain the string "null". What am I going to do then? I need another function.

	public static boolean isNullAndNull(String value) {
		return isNull(value) && "null".equalsIgnoreCase(value);
	}

Ah yes, isNullAndNull, the clearest and easiest name I could imagine for this. It tells me exactly what the function is checking: is it null, and is it also null? We add a second check to our isNull call- we check if the input value matches the string "null". Except we're &&ing the conditions together. So this function will always return false. It can't both be blank and contain the string "null".

Which means Jennifer Null, who is a real person, can breathe easy. This version of a null check won't think she's nothing.

If you've seen one developer recounting how their AI agent deleted production, you've seen them all. They're mostly not interesting stories. It's like watching someone speeding through traffic on a motorcycle without a helmet: the eventual tragedy is sad, but it's unsurprising and not an interesting story to tell. It's not even interesting as a warning: the kind of person who speeds on a motorcycle without a helmet isn't doing so because they don't understand the danger. They've just decided it doesn't apply to them.

But the founder of PocketOS, Jer, recently shared how- whoopsie!- their AI agent deleted production. There's a lot of ingredients that go into this particular disaster, which I think makes it interesting, because the use of a poorly supervised AI agent is only one ingredient in this absolute trainwreck of a story.

PocketOS is a small company that makes software for rental companies to manage reservations. Car rentals are a big customer, but the tool is more general than that. They manage all of their infrastructure via a service called Railway. Railway is a pretty-looking GUI tool for automating your deployments and the target environments.

PocketOS also is heavily adopting Cursor wrapping around the Claude model. They've paid big bucks for the top-end model offered. Many of their components, like Railway, offer MCP services so that their LLM can do useful things. They're using the Claude LLM to automate as much as they can.

So far, this is all a pretty typical setup. They pointed Claude at their code and gave it a "routine" task, and sent it to work. It toddled through the problem and encountered a credential issue. It "decided" that the fix for this issue was to delete a storage volume and recreate it. It scanned through the code to find a file containing an API key, found it, and then sent a POST request via cURL to delete the volume in question.

Jer writes:

To execute the deletion, the agent went looking for an API token. It found one in a file completely unrelated to the task it was working on. That token had been created for one purpose: to add and remove custom domains via the Railway CLI for our services. We had no idea — and Railway's token-creation flow gave us no warning — that the same token had blanket authority across the entire Railway GraphQL API, including destructive operations like volumeDelete. Had we known a CLI token created for routine domain operations could also delete production volumes, we would never have stored it.

Wait, the tokens you create in Railway all have god-level privileges? That sounds like a terrible idea. And you were storing the token in your code? We'll come back to this in a moment, but sure, this is bad, but you can just restore from backup, right?

The volume was deleted. Because Railway stores volume-level backups in the same volume — a fact buried in their own documentation that says "wiping a volume deletes all backups" — those went with it. Our most recent recoverable backup was three months old.

Oh. Oh no.

Now, I don't think it's literally true that Railway is storing your backups literally in the same volume as the thing they're backing up. I certainly hope not. But they do apparently delete your backups when you delete the volume associated with them. Which is a choice, certainly. A bad one. And one that they documented, according to Jer. It was, in his words, "buried" in the docs.

But let's go back to the tokens for a moment. I am not a Railway user, but I checked out the tool and went through the process of creating a project token. And while no, Railway does not give you big red flags warning you "Hey, this token can do ABSOLUTELY ANYTHING", it also never gives you an opportunity to scope the token. Which, I don't know about you, but the first thing I do when I create an authentication entity is try and figure out how to control its authorizations, because I assume at the start it doesn't have any. That'd be sane.

The scoping happens when you create the token, depending on what context you're in when you do it. It's only a handful of scopes, and no fine grained permissions on API keys at all. The lowest level is "Project" which can do anything to a single environment- which does mean that even if you, like Jer's team, wanted to have a script that changed some DNS settings in production, that same key could be used to delete volumes in production. Which means you really really want to take care of that key, and you certainly don't want to leave it where some junior developer or bumbling AI agent can find it.

Jer also complains that Railway shouldn't allow an API call to take destructive actions without more protections, like forcing someone to type in the name of the thing being deleted or sending a confirmation email, or something. This, I'm more skeptical of. Most cloud providers don't offer anything like this in their APIs, at least that I've seen, because on a certain level, if you're invoking the API with the proper credentials, that's a big enough hill to climb that we can assume you've intended your action. The correct way to protect against this is properly scoped keys and keeping those keys secure and not just lying around in plain text. There's a certain aspect of understanding that you're using a potentially dangerous tool and need to take the responsibility for safety into your own hands; while a table saw can easily take some fingers off, it's perfectly safe when used correctly.

This is all bad, but how can we make it worse? Well, Jer demanded that Claude "explain itself". In a section called "The Agent's Confession", Jer highlights that the agent is able to identify the explict rules that it failed to follow.

Read that again. The agent itself enumerates the safety rules it was given and admits to violating every one. This is not me speculating about agent failure modes. This is the agent on the record, in writing.

No, it is not the agent on record. I see this kind of thing a lot when people talk about LLMs. An LLM cannot explain its reasoning. It cannot go on "the record". It cannot confess to anything. While what it plops out when asked might be interesting, it is not an explanation. The only explanation is that it's a powerful statistical model trying to create a plausible string of tokens! It's simply looking at its context window and your prompt and trying to predict what it should say. It can tell you what rules it violated not because it understands the rules or knows it violated any rules, but because those rules are in its context window. If you ask it right, it'll confess to killing JFK and framing Oswald for the crime.

Jer then tries to ensure that Cursor takes some of the blame, pointing to Cursor's "guardrails" documentation. Except, here, the documentation is actually quite explicit about what those guardrails guarantee. If you're using a first-party tool, it will prohibit unsafe operations. When using 3rd party MCPs, like Railway's, the only guardrail is that it requires human approval for every action- unless you update your allowlist for that MCP. If you put them in your allowlist, the guardrails go away. Jer argues that tools should enforce more protection against LLM behaviors, but the problem with that is people- like the PocketOS team- turn those protections off. And like a lot of safety mistakes, they can get away with it all the way up until the point where they can't.

Jer follows this by listing off a pile of other times using Cursor has caused disasters, which isn't making the argument he thinks it is: yes, Cursor is dangerous, but those dangers are well known. It makes the choice to turn Cursor loose without strict supervision seem even more foolish.

Jer writes:

For now I want this incident understood on its own terms: as a Cursor failure, a Railway failure, and a backup-architecture failure that all happened to one company in one Friday afternoon.

It's also a PocketOS failure. It's a failure to properly assess the tools and environments you chose to use for your product. A failure to read and understand the docs for vital features, like *backups*. A failure to employ even the most basic safeguards. A failure to put a second's thought into key management- even if that key was only for DNS entries, you still shouldn't chuck it in source control. A failure to have a competent backup strategy. It's worth noting that they did restore from a three month old backup, which means they were at one point taking backups outside of Railway's volume setup. That was a wise decision. That they stopped is a failure.

The first rule of disaster retrospectives is that it's never one piece that's the failure. It's never one person's fault, one tool's fault, one vendor's fault. It's a systemic failure. Railway's keys should be finer grained. But also, you shouldn't leave keys lying around. Deleting backups when you delete the volume is a terrible idea, but having only one service for backups (that's also your primary site) is a terrible idea. Claude's ability to enforce its own guardrails should be better, but LLMs are notoriously dangerous about this: you should know better, and by your own words you did.

This is not an anti-AI post, or even a "get a load of this asshole" post. It is a "understand the damn tools you're using" post. Be critical of them. Don't trust them. Ever. Especially LLMs, because the worst part of an LLM is that it takes away the one thing computers used to be good at: predictable, deterministic behavior. But not just LLMs: don't trust your cloud provider, don't trust your infrastructure manager. Dig into them and understand how they work, and if they seem to complicated to understand, than they may be too complicated to trust.

Update: As pointed out in the featured comment below, Railway did finally get a backup restored. So they got their data back. Yay? From the post, Jer remains committed to making this a Railway issue and not a PocketOS issue.

"This WTF is in Matlab" almost feels like cheating. At one place I worked, somebody's job was struggling through a mountain of Matlab code and porting it into C. "This Matlab code looks like it was written by an alien," also doesn't really get much traction- all Matlab code looks like it was written by an alien. This falls into the realm of "Researchers use Matlab, researchers may be very smart about their domain, but generally don't know the first thing about writing maintainable code, because that's not their job."

But let's take a look at some MatLab Carl W found:

    try
        if (~isempty(fieldnames(bigStruct)) && isfield(bigStruct,'pathName'))
            [FileName, PathName] = uigetfile(bigStruct.pathName);
        else
            [FileName, PathName] = uigetfile(lastPath); %lastPath holds previous path
        end
    catch
        bigStruct = struct;
    end

The uigetfile function opens a file dialog box. When the user selects a file, FileName holds the filename, PathName holds the containing path. If the user doesn't select a valid file, or clicks "Cancel", both of those variables get set to 0. It's then up to the caller to check the return value and decide what happens next.

Which is not what happens here, obviously. The developer responsible seems to believe that it maybe throws an exception? And they can just catch it? Carl's best guess is that this is a "weird" way to catch the cancel button. But it does mean that FileName and PathName get set to 0, and those zeros propagate until something finally tries to open those files, at which point everything blows up and the user doesn't know why.

A few years back, C# added the concept of "primary constructors". Instead of declaring the storage for class members and then initializing them in the constructor, you can annotate the class itself with the required fields, and C# automatically generates a constructor for you. It's all very TypeScript and very Microsoft, and certainly cuts down on some boilerplate.

Esben B's team isn't really using them in many places, but they are using a linter which is opinionated about them. So this in-line constructor causes the linter to complain:

    public DocumentNetworkController(ILookupClient service)

The linter wants you to switch this to a primary constructor. Esben didn't want to do that, and didn't want to change the global linter configuration, and so added a pragma to disable that particular warning:

#pragma warning disable IDE0290 // Use primary constructor
    public DocumentNetworkController(ILookupClient service)
#pragma warning restore IDE0290

The linter didn't like this. It threw a new warning: that this suppression wasn't needed. Which was news to Esben, as clearly the suppression was needed if you wanted to make the warnings go away. The obvious solution was to disable the warning that you didn't need to disable the warning:

#pragma warning disable IDE0079, IDE0290 // Use primary constructor
    public DocumentNetworkController(ILookupClient service)
#pragma warning restore IDE0290, IDE0079

Except this doesn't work. These pragmas take effect on the next line, which means you can't disable IDE0079 on the same line as IDE0290 and expect it to work. Which means the final version of the code looked like this:

#pragma warning disable IDE0079 // Disable warning about not needed supression
#pragma warning disable IDE0290 // Use primary constructor
    public DocumentNetworkController(ILookupClient service)
#pragma warning restore IDE0290, IDE0079

Esben writes:

So the nice recommendation to use a primary ctor ended up with 3 lines of annoying boilerplate code. Good times \o/

While yes, this is frustrating, I will say there's an element of "when the table saw keeps taking fingers off, that may be more of a you problem." I don't know the details, so I can't say, "just change the linter config or adopt its recommendation" and claim that the problem goes away, but when the tool hurts you, it's a definite sign of one of two things: it's either the wrong tool, or you're using it wrong.

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

We rip on PHP a lot, but I am willing to admit that the language and ecosystem have evolved over the years. What started as an ugly templating language is now just an ugly regular language.

But what happens when you still really want to do things with templates? Allison has inherited a Python-based, WSGI application which rejects any sort of formal routing or basic web development best practices. Their way of routing requests is simply long chains of "if condition then invokeA elif otherCondition then invokeB". Sometimes, those conditions will directly set the MIME type on the HTTP response.

They do use a templating library called Mako for generating their responses. They use it for their HTML responses, obviously. They also use it for their JSON responses, generating code like this:

{
    "success": true,
    "items": {
        %for item in items_available.keys():
        "${item}": ${items_available[item]}${',' if not loop.last else ''} 
        %endfor
        }   
}

The %for and matching %endfor mark the Python code off, which generates JSON via string-munging, complete with the check to make sure we're not on the last iteration of the loop.

Like so much bad code, this offers a degree of fractal wrongness. Instead of iterating over the keys and fetching the items inside the loop, you could iterate for key,value in items_available.items()- and according to the Mako docs, that for is just a regular Python for loop. That we're just outputting the contents of the dictionary is itself potentially a problem- sure, if we know the types of the dictionary, we'll know that whatever it is can be output in the body of a JSON document, but do we really think this code is using type annotations? I don't. And for a RESTful web service, I'm always going to feel weird about using a success field when ideally the HTTP status code could convey most of that information (and yes, I know there are reasons to still put status in the body, I just hate it).

Of course, the real issue is just: Python's built in JSON serialization is actually pretty advanced. And performant! You don't need any of this, you could just do something like:

return json.dumps({"success": true, "items": items_available})

No templates. No formatting. No worries about how the data gets represented. Well, still worries, because JSON serialier will throw exceptions if it doesn't know what to do with a type. But then at least you get that exception on the server side and aren't sending the client a malformed document.

In any case, this is a good demonstration that you can write bad PHP in any language.

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

Henrik H (previously) sends us a simple representative C# line:

static void GenerateCommercilaInvoice()

This is a static method which takes no parameters and returns nothing. Henrik didn't share the implementation, but this static function likely does something that involves side effects, maybe manipulating the database (to generate that invoice?). Or, possibly worse, it could be doing something with some global or static state. It's all side effects and no meaningful controls, so enjoy debugging that when things go wrong. Heck, good luck testing it. Our best case possibility is that it's just a wrapper around a call to a stored procedure.

This method signature is basically a commercila for refactoring.

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

Today, we look at a representative comment, sent to us by Nona. This particular comment was in a pile of code delivered by an offshore team.

// https://stackoverflow.com/questions/46744740/lodash-mongoose-object-id-difference/46745169

"Wait," you say, "what's the WTF about a comment pointing to a Stack Overflow page. I do that all the time?"

In this case, it's because this particular comment wasn't given any further explanation. It also wasn't in a block of code that was doing anything with either lodash, Mongoose, or set differences. It was, however, repeated multiple times throughout the codebase, because the entire codebase was a pile of copy-pasta glued together with the bare minimum code to make it work.

In at least one place, the comment was probably correct and helpful. But it got swept up as part of a broader copy/paste exercise, and now is scattered through the code without any true purpose.

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

Eric O worked for a medical device company. The medical device industry moves slowly, relative to other technical industries. Medical science and safety have their own cadence, and at a certain point, iterating faster doesn't matter much.

Eric was working on a new feature on a system that had been in use for thirteen years. This new feature interacted with a database which stored information about racks of test tubes, and Eric's tests meant creating several entries for racks of test tubes. And that's when Eric discovered that the database only allowed thirty racks. Add any more, it would just roll right back over to one.

This was odd. The database was small- less than 40MB, even in production- and there were automatic tasks to purge old data for compliance purposes. Why a hard limit of thirty?

Eric had only been at the company for a year, so he asked one of the more senior team members, Lester. "Oh yeah, that was before my time. You should probably ask Carl."

Later that day, Eric happened to bump into Carl around the coffee maker, and asked the question. "Oh, yeah, I do vaguely remember something about that. It was in the requirements for the product. I thought it was weird, but didn't think too much about it. You should probably ask Elise, she's been here like twenty years."

Well, now it was getting curious. Eric went over to the "old building", as it was named, the original office for the company on the other side of the parking lot. Most of the offices had moved to the new building a decade earlier, and it mostly served as fabrication and storage, but a few offices remained.

Elise was on the third floor, down a poorly lit hallway, sitting in an office with water-stained acoustical tile in its ceiling. "Oh, yeah, I put that into the requirements document. It's funny, I thought it was weird too, but the system you're working on was a replacement for an older system. Our requirements were derived from those. Let me think… Irving worked on that, but he's dead, god rest him. Penny is retired. Oh, you know, Humbert is still around. He didn't work on that, but he worked on some of the systems that came before that. He's upstairs and on the other side of the building."

Eric went upstairs and to the other side of the building. The fourth floor had been last remodeled circa 1985, and the ugly industrial paint on the wall was made even uglier by the fact that someone had replaced most of the flourescent tubes with LEDs. Most. The mismatched color temperature started Eric down the path of a headache.

Humbert was in an office similar to Elise's. On his desk was a plaque commemerating 40 years of service with the company. Eric asked about the limitation, and Humbert laughed.

"You're working on the latest version of a product that initially started on an old PDP-11 running MUMPS. I mean, the first versions, anyway. We ran to desktop computers as fast as we could. I wrote a version for DOS in… oh… '86? I knew none of the facilities we worked with had more than ten or fifteen racks of tubes, and I needed somehow to limit the size of the database so it all fit on a single 5 1/4" floppy disk. I picked thirty, because it seemed like a good round number. Honestly, I'm shocked that the limit still exists."

So was Eric. There had been several ground-up-rewrites since 1986, before the one Eric maintained had been released thirteen years ago. Each one of them had chosen to maintain the same limitation, without ever considering why it existed. The rule had simply been copied, mindlessly, for 40 years.

"I'm kind of impressed," Eric said to Humbert, "in a horrified way."

"Me too, kid, me too."

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

"Here, you're a programmer, take this over. It's business critical."

That's what Felicity's boss told her when he pointed her to a network drive containing an Excel spreadsheet. The Excel spreadsheet contained a pile of macros. The person who wrote it had left, and nobody knew how to make it work, but the macros in question were absolutely business vital.

Also, it's in French.

We'll take this one in chunks. The indentation is as in the original.

Public Sub ExporToutVersBaseDonnées(ClasseurEnCours As Workbook)
Call AffectionVariables(ToutesLesCellulesNommées)
Call AffectationBaseDonnées(BaseDonnées)
BaseDonnées.Activate

The procedures AffectionVariables and AffectationBaseDonnées populate a pile of global variables. "base de données" is French for database, but don't let the name fool you- anything referencing "base de données" is referencing another Excel file located on a shared server. There are, in total, four Excel files that must live on a shared server, and two more which must be in a hard-coded path on the user's computer.

Oh, and the shared server is referenced not by a hostname, but by IP address- which is why the macros were breaking on everyone's computer; the IP address changed.

Let's continue.

'Vérifier si la ligne existe déjà.
        If ClasseurEnCours.Sheets("DATA").Range("Num_Fichier") = 0 Then
        Num_Fichier = BaseDonnées.Sheets(1).Range("Dernier_Fichier").Value + 1
Insérer_Ligne: '(étiquette Goto) insérer une ligne
    Application.GoTo Reference:="Dernière_Ligne"
            Selection.EntireRow.Insert
'Copie les cellules (colonne A à colonne FI) de la ligne au-dessus de la ligne insérée.
            With ActiveCell
                    .Offset(-1, 0).Range("A1:FM1").Copy
'Colle le format de la cellule précédemment copiée à la cellule active puis libère les données du presse papier
                    .PasteSpecial
                    .Range("A1:FM1").Value = ""
'Se repositionne au début de la ligne insérée.
                    .Range("A1").Select
            End With
            Application.CutCopyMode = False

Uh oh, Insérer_Ligne is a label for a Goto target. Not to be confused by the Application.GoTo call on the next line- that just selects a range in the spreadsheet.

After that little landmine, we copy/paste some data around in the sheet.

That's the If side of the conditional, let's look at the else clause:

        Else
Cherche_Numéro_Fichier: ' Chercher la ligne ou le numéro de fichier est égale à NumFichier.
                        While ActiveCell.Value <> Num_Fichier
                If ActiveCell.Row = Range("Etiquettes").Row Then
                    GoTo Insérer_Ligne
                End If
                ActiveCell.Offset(-1, 0).Range("a1:a1").Select
            Wend
            'Vérifier le numéro d'indice de la ligne active.
                If Cells(ActiveCell.Row, 165).Value <> ClasseurEnCours.Sheets("DATA").Range("Dernier_Indice") Then
                    ActiveCell.Offset(-1, 0).Range("A1:A1").Select
                    GoTo Cherche_Numéro_Fichier
                End If
            ActiveCell.Offset(0, 0).Range("A1:FM1").Value = ""
        End If

We start with another label, and… then we have a Goto. A Goto which jumps us back into the If side of the conditional. A Goto inside of a while loop, a while loop that's marching around the spreadsheet to search for certain values in the cell.

After the loop, we have another Goto which will possibly jump us up to the start of the else block.

The procedure ends with some cleanup:

'----- 
' Do some stuff on the active cell and the following cells on the column
.-----
BaseDonnées.Close True
Set BaseDonnées = Nothing
End Sub

I do not know what this function does, and the fact that the code is largely in a language I don't speak isn't the obstacle. I have no idea what the loops and the gotos are trying to do. I'm not even a "never use Goto ever ever ever" person; in a language like VBA, it's sometimes the best way to handle errors. But this bizarre time-traveling flow control boggles me.

"Etiquettes" is French for "labels", and it may be bad etiquette but I've got some four letter labels for this code.

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

Nona writes: "this is the beginning of a 2100 line function."

That's bad. Nona didn't send us the entire JavaScript function, but sent us just the three early lines, which definitely raise concerns:

if (res.length > 0) {
  await (function () {
    return new Promise((resolve, reject) => {

We await a synchronous function which retuns a promise, passing a function to the promise. As a general rule, you don't construct promises directly, you let asynchronous code generate them and pass them around (or await them). It's not a thing you never do, but it's certainly suspicious. It gets more problematic when Nona adds:

This function happens to contain multiple code repetition snippets, including these three lines.

That's right, this little block appears multiple times in the function, inside of anonymous function getting passed to the Promise.

No, the code does not work in its current state. It's unclear what the 2100 line function was supposed to do. And yes, this was written by lowest-bidder third-party contractors.

Nona adds:

I am numb at this point and know I gotta fix it or we lose contracts

Management made the choice to "save money" by hiring third parties, and now Nona's team gets saddled with all the crunch to fix the problems created by the "savings".

Today's Anonymous submitter sends us some React code. We'll look at the code and then talk about the WTF:

// inside a function for updating checkboxes on a page
if (!e.target.checked) {
  const removeIndex = await checkedlist.findIndex(
    (sel) => sel.Id == selected.Id,
  )
  const removeRowIndex = await RowValue.findIndex(
    (sel) => sel == Index,
  )

// checkedlist and RowValue are both useState instances.... they should never be modified directly
  await checkedlist.splice(removeIndex, 1)
  await RowValue.splice(removeRowIndex, 1)

// so instead of doing above logic in the set state, they dont
  setCheckedlist(checkedlist)
  setRow(RowValue)
} else {
  if (checkedlist.findIndex((sel) => sel.Id == selected.Id) == -1) {
    await checkedlist.push(selected)
  }
// same, instead of just doing a set state call, we do awaits and self updates
  await RowValue.push(Index)
  setCheckedlist(checkedlist)
  setRow(RowValue)
}

Comments were added by our submitter.

This code works. It's the wrong approach for doing things in React: modifying objects controlled by react, instead of using the provided methods, it's doing asynchronous push calls. Without the broader context, it's hard to point out all the other ways to do this, but honestly, that's not the interesting part.

I'll let our submitter explain:

This code is black magic, because if I update it, it breaks everything. Somehow, this is working in perfect tandem with the rest of the horrible page, but if I clean it up, it breaks the checkboxes; they're no longer able to be clicked. Its forcing React somehow to update asynchronously so it can use these updated values correctly, but thats the neat part, they aren't even being used anywhere else, but somehow the re-rendering page only accepts awaits. I've tried refactoring it 5 different ways to no avail

That's what makes truly bad code. Code so bad that you can't even fix it without breaking a thousand other things. Code that you have to carefully, slowly, pick through and gently refactor, discovering all sorts of random side-effects that are hidden. The code so bad that you actually have to live with it, at least for awhile.

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

Aankhen has a peer who loves writing Python scripts to automate repetitive tasks. We'll call this person Ernest.

Ernest was pretty proud of some helpers he wrote to help him manage his Docker containers. For example, when he wanted to stop and remove all his running Docker containers, he wrote this script:

#!/usr/bin/env python
import subprocess

subprocess.run("docker kill $(docker ps -q)", shell=True)
subprocess.run("docker rm $(docker ps -a -q)", shell=True)

He aliased this script to docker-stop, so that with one command he could… run two.

"Ernest," Aankhen asked, "couldn't this just be a bash script?"

"I don't really know bash," Ernest replied. "If I just do it in bash, if the first command fails, the second command doesn't run."

Aankhen pointed out that you could make bash not do that, but Ernest replied: "Yeah, but I always forget to. This way, it handles errors!"

"It explicitly doesn't handle errors," Aankhen said.

"Exactly! I don't need to know when there are no containers to kill or remove."

"Okay, but why not use the Docker library for Python?"

"What, and make the software more complicated? This has no dependencies!"

Aankhen was left with a sinking feeling: Ernest was either the worst developer he was working with, or one of the best.

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

Every once in awhile, we get a bit of terrible code, and our submitter also shares, "this isn't called anywhere," which is good, but also bad. Ernesto sends us a function which is called in only one place:

///
/// Shutdown server
///
private void shutdownServer()
{
    shutdownServer();
}

The "one place", obviously, is within itself. This is the Google Search definition of recursion, where each recursive call is just the original call, over and over again.

This is part of a C# service, and this method shuts down the server, presumably by triggering a stack overflow. Unless C# has added tail calls, anyway.

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

We mostly don't pick on bad SQL queries here, because mostly the query optimizer is going to fix whatever is wrong, and the sad reality is that databases are hard to change once they're running; especially legacy databases. But sometimes the code is just so hamster-bowling-backwards that it's worth looking into.

Jim J has been working on a codebase for about 18 months. It's a big, sprawling, messy project, and it has code like this:

AND CASE WHEN @c_usergroup = 50 AND NOT EXISTS(SELECT 1 FROM l_appl_client lac WHERE lac.f_application = fa.f_application AND lac.c_linktype = 840 AND lac.stat = 0 AND CASE WHEN ISNULL(lac.f_client,0) <> @f_client_user AND ISNULL(lac.f_c_f_client,0) <> @f_client_user THEN 0 ELSE 1 END = 1 ) THEN 0 ELSE 1 END = 1 -- 07.09.2022

We'll come back to what it's doing, but let's start with a little backstory.

This code is part of a two-tier application: all the logic lives in SQL Server stored procedures, and the UI is a PowerBuilder application. It's been under development for a long time, and in that time has accrued about a million lines of code between the front end and back end, and has never had more than 5 developers working on it at any given time. The backlog of feature requests is nearly as long as the backlog of bugs.

You may notice the little date comment in the code above. That's because until Jim joined the company, they used Visual Source Safe for version control. Visual Source Safe went out of support in 2005, and let's be honest: even when it was in support it barely worked as a source control system. And that's just the Power Builder side- the database side just didn't use source control. The source of truth was the database itself. When going from development to test to prod, you'd manually export object definitions and run the scripts in the target environment. Manually. Yes, even in production. And yes, environments did drift and assumptions made in the scripts would frequently break things.

You may also notice the fields above use a lot of Hungarian notation. Hungarian, in the best case, makes it harder to read and reason about your code. In this case, it's honestly fully obfuscatory. c_ stands for a codetable, f_ for entities. l_ is for a many-to-many linking table. z_ is for temporary tables. So is x_. And t_. Except not all of those "temporary" tables are truly temporary, a lesson Jim learned when trying to clean up some "junk" tables which were not actually junk.

I'll let Jim add some more detail around these prefixes:

an "application" may have a link to a "client", so there is an f_client field; but also it references an "agent" (which is also in the f_client table, surpise!) - this is how you get an f_c_f_client field. I have no clue why the prefix is f_c_ - but I also found c_c_c_channel and fc4_contact columns. The latter was a shorthand for f_c_f_c_f_c_f_contact, I guess.

"f_c_f_c_f_c_f_c" is also the sound I'd make if I saw this in a codebase I was responsible for. It certainly makes me want to change the c_c_c_channel.

With all this context, let's turn it back over to Jim to explain the code above:

And now, with all this background in mind, let's have a look at the logic in this condition. On the deepest level we check that both f_client and f_c_f_client are NOT equal to @f_client_user, and if this is the case, we return 0 which is NOT equal to 1 so it's effectively a negation of the condition. Then we check that records matching this condition do NOT EXIST, and when this is true - also return 0 negating the condition once more.

Honestly, the logic couldn't be clearer, when you put it that way. I jest, I've read that twelve times and I still don't understand what this is for or why it's here. I just want to know who we can prosecute for this disaster. The whole thing is a quadruple negative and frankly, I can't handle that kind of negativity.

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

Today's snippet from Rich D is short and sweet, and admittedly, not the most TFs of WTFs out there. But it made me chuckle, and sometimes that's all we need. This Java snippet shows us how to delete a file:

if (Files.exists(filePath)) {
    Files.deleteIfExists(filePath);
}

If the file exists, then if it exists, delete it.

This commit was clearly submitted by the Department of Redundancy Department. One might be tempted to hypothesize that there's some race condition or something that they're trying to route around, but if they are, this isn't the way to do it, per the docs: "Consequently this method may not be atomic with respect to other file system operations." But also, I fail to see how this would do that anyway.

The only thing we can say for certain about using deleteIfExists instead of delete is that deleteIfExists will never throw a NoSuchFileException.

Agatha has inherited some Windows Forms code. This particular batch of such code falls into that delightful category of code that's wrong in multiple ways, multiple times. The task here is to disable a few panels worth of controls, based on a condition. Or, since this is in Spanish, "bloquear controles". Let's see how they did it.

private void BloquearControles()
{
	bool bolBloquear = SomeConditionTM; // SomeConditionTM = a bunch of stuff. Replaced for clarity.

	// Some code. Removed for clarity.
	
	// private System.Windows.Forms.Panel pnlPrincipal;
	foreach (Control C in this.pnlPrincipal.Controls)
	{
		if (C.GetType() == typeof(System.Windows.Forms.TextBox))
		{
			C.Enabled = bolBloquear;
		}
		if (C.GetType() == typeof(System.Windows.Forms.ComboBox))
		{
			C.Enabled = bolBloquear;
		}
		if (C.GetType() == typeof(System.Windows.Forms.CheckBox))
		{
			C.Enabled = bolBloquear;
		}
		if (C.GetType() == typeof(System.Windows.Forms.DateTimePicker))
		{
			C.Enabled = bolBloquear;
		}
		if (C.GetType() == typeof(System.Windows.Forms.NumericUpDown))
		{
			C.Enabled = bolBloquear;
		}
	}
	
	// private System.Windows.Forms.GroupBox grpProveedor;
	foreach (Control C1 in this.grpProveedor.Controls)
	{
		if (C1.GetType() == typeof(System.Windows.Forms.TextBox))
		{
			C1.Enabled = bolBloquear;
		}
		if (C1.GetType() == typeof(System.Windows.Forms.ComboBox))
		{
			C1.Enabled = bolBloquear;
		}
		if (C1.GetType() == typeof(System.Windows.Forms.CheckBox))
		{
			C1.Enabled = bolBloquear;
		}
		if (C1.GetType() == typeof(System.Windows.Forms.DateTimePicker))
		{
			C1.Enabled = bolBloquear;
		}
		if (C1.GetType() == typeof(System.Windows.Forms.NumericUpDown))
		{
			C1.Enabled = bolBloquear;
		}
	}

	// private System.Windows.Forms.GroupBox grpDescuentoGeneral;
	foreach (Control C2 in this.grpDescuentoGeneral.Controls)
	{
		if (C2.GetType() == typeof(System.Windows.Forms.TextBox))
		{
			C2.Enabled = bolBloquear;
		}
		if (C2.GetType() == typeof(System.Windows.Forms.ComboBox))
		{
			C2.Enabled = bolBloquear;
		}
		if (C2.GetType() == typeof(System.Windows.Forms.CheckBox))
		{
			C2.Enabled = bolBloquear;
		}
		if (C2.GetType() == typeof(System.Windows.Forms.DateTimePicker))
		{
			C2.Enabled = bolBloquear;
		}
		if (C2.GetType() == typeof(System.Windows.Forms.NumericUpDown))
		{
			C2.Enabled = bolBloquear;
		}
	}

	// Some more code. Removed for clarity.
}

This manages two group boxes and a panel. It checks a condition, then iterates across every control beneath it, and sets their enabled property on the control. In order to do this, it checks the type of the control for some reason.

Now, a few things: every control inherits from the base Control class, which has an Enabled property, so we're not doing this check to make sure the property exists. And every built-in container control automatically passes its enabled/disabled state to its child controls. So there's a four line version of this function where we just set the enabled property on each container.

This leaves us with two possible explanations. The first, and most likely, is that the developer responsible just didn't understand how these controls worked, and how inheritance worked, and wrote this abomination as an expression of that ignorance. This is extremely plausible, extremely likely, and honestly, our best case scenario.

Because our worse case scenario is that this code's job isn't to disable all of the controls. The reason they're doing type checking is that there are some controls used in these containers that don't match the types listed. The purpose of this code, then, is to disable some of the controls, leaving others enabled. Doing this by type would be a terrible way to manage that, and is endlessly confusing. Worse, I can't imagine how this behavior is interpreted by the end users; the enabling/disabling of controls following no intuitive pattern, just filtered based on the kind of control in use.

The good news is that Agatha can point us towards the first option. She adds:

They decided to not only disable the child controls one by one but to check their type and only disable those five types, some of which aren't event present in the containers. And to make sure this was WTF-worthy the didn't even bother to use else-if so every type is checked for every child control

She also adds:

At this point I'm not going to bother commenting on the use of GetType() == typeof() instead of is to do the type checking.

Bad news, Agatha: you did bother commenting. And even if you didn't, don't worry, someone would have.

Python is (in)famous for its "batteries included" approach to a standard library, but it's not that notable that it has plenty of standard data structures, like dicts. Nor is in surprising that dicts have all sorts of useful methods, like pop, which removes a key from the dict and returns its value.

Because you're here, reading this site, you'll also be unsurprised that this doesn't stop developers from re-implementing that built-in function, badly. Karen sends us this:

def parse_message(message):
    def pop(key):
        if key in data:
            result = data[key]
            del data[key]
            return result
        return ''

    data = json.loads(message)
    some_value = pop("some_key")
    # <snip>...multiple uses of pop()...</snip>

Here, they create an inner method, and they exploit variable hoisting. While pop appears in the code before data is declared, all variable declarations are "hoisted" to the top. When pop references data, it's getting that from the enclosing scope. Which while this isn't a global variable, it's still letting a variable cross between two scopes, which is always messy.

Also, this pop returns a default value, which is also something the built-in method can do. It's just the built-in version requires you to explicitly pass the value, e.g.: some_value = data.pop("some_key", "")

Karen briefly wondered if this was a result of the Python 2 to 3 conversion, but no, pop has been part of dict for a long time. I wondered if this was just an exercise in code golf, writing a shorthand function, but even then- you could just wrap the built-in pop with your shorthand version (not that I'd recommend such a thing). No, I think the developer responsible simply didn't know the function was there, and just reimplemented a built-in method badly, as so often happens.

Industrial machines are generally accompanied by "Human Machine Interfaces", HMIs. This is industrial slang for a little computerized box you use to control the industrial machine. All the key logic and core functionality and especially the safety functionality is handled at a deeper computer layer in the system. The HMI is just buttons users can push to interact with the machine.

Purchasers of those pieces of industrial equipment often want to customize that user interface. They want to guide users away from functions they don't need, or make their specific workflow clear, or even just brand the UI. This means that the vendor needs to publish an API for their HMI.

Which brings us to Wendy. She works for a manufacturing company which wants to customize the HMI on a piece of industrial equipment in a factory. That means Wendy has been reading the docs and poking at the open-sourced portions of the code, and these raise more questions than they answer.

For example, the HMI's API provides its own set of collection types, in C#. We can wonder why they'd do such a thing, which is certainly a WTF in itself, but this representative line raises even more questions than that:

Int32 Count { get; set; }

What happens if you use the public set operation on the count of items in a collection? I don't know. Wendy doesn't either, as she writes:

I'm really tempted to set the count but I fear the consequences.

All I can hear in my head when I think about "setting the Count" is: "One! One null reference exception! Two! TWO null reference exceptions! HA HA HA HA!"

By http://muppet.wikia.com/wiki/Count_von_Count

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

I've had the misfortune of working in places which did source-control via comments. Like one place which required that, with each section of code changed, you needed to add a comment with your name, the ticket number, and the reason the change was made. You know, the kind of thing you can just get from your source control service.

In their defense, that policy was invented for mainframe developers and then extended to everyone else, and their source control system was in Visual Source Safe. VSS was a) terrible, and b) a perennial destroyer of history, so maybe they weren't entirely wrong and VSS was the real WTF. I still hated it.

In any case, Alice's team uses more modern source control than that, which is why she's able to explain to us the story of this function:

public function calculateMassGrossPay(array $employees, Payroll $payroll): array
{
    // it shouldn't enter here, but if it does by any change, do nth
    return [];
}

Once upon a time, this function actually contained logic, a big pile of fairly complicated logic. Eventually, a different method was created which streamlined the functionality, but had a different signature and logic. All the callers were updated to use that method instead- by commenting out the line which called this one. This function had a comment added to the top: // it shouldn't enter here.

Then, the body of this function got commented out, and the return was turned into an empty array. The comment was expanded to what you see above. Then, eventually, the commented-out callers were all deleted. Years after that, the commented out body of this function was also deleted, leaving behind the skeleton you see here.

This function is not referenced anywhere else, not even in a comment. It's truly impossible for code to "enter here".

Alice writes: "Version control by commented out code does not work very well."

Indeed, it does not.

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

Henrik H's employer thought they could save money by hiring offshore, and save even more money by hiring offshore junior developers, and save even more money by basically not supervising them at all.

Henrik sends us just one representative line:

if (System.DateTime.Now.AddDays(-365) <= f.ReleaseDate) // 365 days means one year 

I appreciate the comment, that certainly "helps" explain the magic number. There's of course, just one little problem: It's wrong. I mean, ~75% of the time, it works every time, but it happily disregards leap years. Which may or may not be a problem in this case, but if they got so far as learning about the AddDays method, they were inches from using AddYears.

It feels like ages ago, when document databases like Mongo were all the rage. That isn't to say that they haven't stuck around and don't deliver value, but gone is the faddish "RDBMSes are dead, bro." The "advantage" they offer is that they turn data management problems into serialization problems.

And that's where today's anonymous submission takes us. Our submitter has a long list of bugs around managing lists of usernames. These bugs largely exist because the contract developer who wrote the code didn't write anything, and instead "vibe coded too close to the sun", according to our submitter.

Here's the offending C# code:

   [JsonPropertyName("invitedTraders")]
   [BsonElement("invitedTraders")]
   [BsonIgnoreIfNull]
   public InvitedTradersV2? InvitedTraders { get; set; }

   [JsonPropertyName("invitedTradersV2")]
   [BsonElement("invitedTradersV2")]
   [BsonIgnoreIfNull]
   public List<string>? InvitedTradersV2 { get; set; }

Let's start with the type InvitedTradersV2. This type contains a list of strings which represent usernames. The field InvitedTradersV2 is a list of strings which represent usernames. Half of our submitter's bugs exist simply because these two lists get out of sync- they should contain the same data, but without someone enforcing that correctly, problems accrue.

This is made more frustrating by the MongoDB attribute, BsonIgnoreIfNull, which simply means that the serialized object won't contain the key if the value is null. But that means the consuming application doesn't know which key it should check.

For the final bonus fun, note the use of JsonPropertyName. This comes from the built-in class library, which tells .NET how to serialize the object to JSON. The problem here is that this application doesn't use the built-in serializer, and instead uses Newtonsoft.JSON, a popular third-party library for solving the problem. While Newtonsoft does recognize some built-in attributes for serialization, JsonPropertyName is not among them. This means that property does nothing in this example, aside from add some confusion to the code base.

I suspect the developer responsible, if they even read this code, decided that the duplicated data was okay, because isn't that just a normal consequence of denormalization? And document databases are all about denormalization. It makes your queries faster, bro. Just one more shard, bro.

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

Darren is supporting a Delphi application in the current decade. Which is certainly a situation to be in. He writes:

I keep trying to get out of doing maintenance on legacy Delphi applications, but they keep pulling me back in.

The bit of code Darren sends us isn't the largest WTF, but it's a funny mistake, and it's a funny mistake that's been sitting in the codebase for decades at this point. And as we all know, jokes only get funnier with age.

FileName := DestDir + ExtractFileName(FileName);
if FileExists(DestDir + ExtractFileName(FileName)) then
begin
  ...
end;

This code is inside of a module that copies a file from a remote server to the local host. It starts by sanitizing the FileName, using ExtractFileName to strip off any path components, and replace them with DestDir, storing the result in the FileName variable.

And they liked doing that so much, they go ahead and do it again in the if statement, repeating the exact same process.

Darren writes:

As Homer Simpson said "Lather, rinse, and repeat. Always repeat."

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

Remy's Law of Requirements Gathering states "No matter what the requirements document says, what your users really wanted was Excel." This has a corrolary: "Any sufficiently advanced Excel file is indistingushable from software."

Given enough time, any Excel file whipped up by any user can transition from "useful" to "mission critical software" before anyone notices. That's why Nemecsek was tasked with taking a pile of Excel spreadsheets and converting them into "real" software, which could be maintained and supported by software engineers.

Nemecsek writes:

This is just one of the formulas they asked me to work on, and not the longest one.

Nemecsek says this is a "formula", but I suspect it's a VBA macro. In reality, it doesn't matter.

InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).InitechNeoDTActivePart(0).
InitechNeoDTActivePartPartContainer(0).InitechNeoDTActivePartPart(iPart).Losses = 
calcLossesInPart(InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).
InitechNeoDTActivePart(0).RatedFrequency, InitechNeoDTMachineDevice.
InitechNeoDTActivePartContainer(0).InitechNeoDTActivePart(0).InitechNeoDTActivePartPartContainer(0).
InitechNeoDTActivePartPart(iPart).RadialPositionToMainDuct, InitechNeoDTMachineDevice.
InitechNeoDTActivePartContainer(0).InitechNeoDTActivePart(0).InitechNeoDTActivePartPartContainer(0).
InitechNeoDTActivePartPart(iPart).InitechNeoDTActivePartPartSectionContainer(0).
InitechNeoDTActivePartPartSection(0).InitechNeoDTActivePartPartConductorComposition(0).IsTransposed, 
InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).InitechNeoDTActivePart(0).
InitechNeoDTActivePartPartContainer(0).InitechNeoDTActivePartPart(iPart).
InitechNeoDTActivePartPartSectionContainer(0).InitechNeoDTActivePartPartSection(0).
InitechNeoDTActivePartPartConductorComposition(0).ParallelRadialCount, InitechNeoDTMachineDevice.
InitechNeoDTActivePartContainer(0).InitechNeoDTActivePart(0).InitechNeoDTActivePartPartContainer(0).
InitechNeoDTActivePartPart(iPart).InitechNeoDTActivePartPartSectionContainer(0).
InitechNeoDTActivePartPartSection(0).InitechNeoDTActivePartPartConductorComposition(0).
ParallelAxialCount, InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).
InitechNeoDTActivePart(0).InitechNeoDTActivePartPartContainer(0).InitechNeoDTActivePartPart(iPart).
InitechNeoDTActivePartPartSectionContainer(0).InitechNeoDTActivePartPartSection(0).
InitechNeoDTActivePartPartConductorComposition(0).InitechNeoDTActivePartPartConductor(0).Type, 
InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).InitechNeoDTActivePart(0).
InitechNeoDTActivePartPartContainer(0).InitechNeoDTActivePartPart(iPart).
InitechNeoDTActivePartPartSectionContainer(0).InitechNeoDTActivePartPartSection(0).
InitechNeoDTActivePartPartConductorComposition(0).InitechNeoDTActivePartPartConductor(0).
DimensionRadialElectric, InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).
InitechNeoDTActivePart(0).InitechNeoDTActivePartPartContainer(0).InitechNeoDTActivePartPart(iPart).
InitechNeoDTActivePartPartSectionContainer(0).InitechNeoDTActivePartPartSection(0).
InitechNeoDTActivePartPartConductorComposition(0).InitechNeoDTActivePartPartConductor(0).
DimensionAxialElectric + InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).
InitechNeoDTActivePart(0).InitechNeoDTActivePartPartContainer(0).InitechNeoDTActivePartPart(iPart).
InitechNeoDTActivePartPartSectionContainer(0).InitechNeoDTActivePartPartSection(0).
InitechNeoDTActivePartPartConductorComposition(0).InitechNeoDTActivePartPartConductor(0).InsulThickness, 
getElectricConductivityAtTemperatureT1(InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).
InitechNeoDTActivePart(0).InitechNeoDTActivePartPartContainer(0).InitechNeoDTActivePartPart(iPart).
InitechNeoDTActivePartPartSectionContainer(0).InitechNeoDTActivePartPartSection(0).
InitechNeoDTActivePartPartConductorComposition(0).InitechNeoDTActivePartPartConductor(0).
InitechNeoDTActivePartPartConductorRawMaterial(0).ElectricConductivityT0, InitechNeoDTMachineDevice.
InitechNeoDTActivePartContainer(0).InitechNeoDTActivePart(0).InitechNeoDTActivePartPartContainer(0).
InitechNeoDTActivePartPart(iPart).InitechNeoDTActivePartPartSectionContainer(0).
InitechNeoDTActivePartPartSection(0).InitechNeoDTActivePartPartConductorComposition(0).
InitechNeoDTActivePartPartConductor(0).InitechNeoDTActivePartPartConductorRawMaterial(0).MaterialFactor, 
InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).InitechNeoDTActivePart(0).
InitechNeoDTActivePartPartContainer(0).InitechNeoDTActivePartPart(iPart).
InitechNeoDTActivePartPartSectionContainer(0).InitechNeoDTActivePartPartSection(0).
InitechNeoDTActivePartPartConductorComposition(0).InitechNeoDTActivePartPartConductor(0).
InitechNeoDTActivePartPartConductorRawMaterial(0).ReferenceTemperatureT0, InitechNeoDTMachineDevice.
ReferenceTemperature), LayerNumberRatedVoltage, InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).
InitechNeoDTActivePart(0).InitechNeoDTActivePartPartContainer(0).InitechNeoDTActivePartPart(iPart).
InitechNeoDTActivePartPartLayerContainer(0),InitechNeoDTMachineDevice.InitechNeoDTActivePartContainer(0).
InitechNeoDTActivePart(0).RFactor)

Line breaks added to try and keep horizontal scrolling sane. This arguably hurts readability, in the same way that beating a dead horse arguably hurts the horse.

This may not be the longest one, but it's certainly painful. I do not know exactly what this is doing, and frankly, I do not want to.

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

As we enter into the latter portion of the year, folks are traveling to visit family, logging off of work in hopes that everything can look after itself for a month, and somewhere, someone, is going to make the choice "yes, I can push to prod on Christmas Eve, and it'll totally work out for me!"

Over the next few weeks, I'm hoping to get a chance to get some holiday support horrors up on the site, in keeping with the season. Whether it's the absurd challenges of providing family tech support, the last minute pushes to production, the five alarm fires caused by a pointy-haired-bosses's incompetence, we want your tales of holiday IT woe.

So hit that submit button on the side bar, and tell us who's on Santa's naughty list this year.

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

Today, Reginald approaches us for a confession.

He writes:

I've no idea where I "copied" this code from five years ago. The purpose of this code was to filter out Maps and Collections Maybe the intention was to avoid a recursive implementation by an endless loop? I am shocked that I wrote such code.

Well, that doesn't bode well, Reginald. Let's take a look at this Java snippet:

/**
 * 
 * @param input
 * @return
 */
protected Map rearrangeMap(Map input) {
	Map retMap = new HashMap();

	if (input != null && !input.isEmpty()) {

		Iterator it = input.keySet().iterator();
		while (true) {
			String key;
			Object obj;
			do {
				do {
					if (!it.hasNext()) {
					}
					key = (String) it.next();

				} while (input.get(key) instanceof Map);

				obj = input.get(key);

			} while (obj instanceof Boolean && ((Boolean) obj).equals(Boolean.FALSE));

			if (obj != null) {
				retMap.put(key, obj);
				return retMap;
			}
		}
	} else {
		return retMap;
	}
}

The first thing that leaps out is that this is a non-generic Map, which is always a code smell, but I suspect that's the least of our problems.

We start by verifying that the input Map exists and contains data. If the input is null or empty, we return it. In our main branch, we create an iterator across the keys, before ethering a while(true) loop. So far so bad

Then we enter a pair of nested do loops. Which definitely hints that we've gone off the edge of the map here. In the inner most loop, we do a check- if there isn't a next element in the iterator, we… do absolutely nothing. Whether there is or isn't an element, we advance to the next element, risking a NoSuchElementException. We do this while the key points to an instance of Map. As always, an instanceof check is a nauseating code stench.

Okay, so the inner loop skips across any keys that point to maps, and throws an exception when it gets to the end of the list.

The surrounding loop skips over every key that is a boolean value that is also false.

If we find anything which isn't a Map and isn't a false Boolean and isn't null, we put it in our retMap and return it.

This function finds the first key that points to a non-map, non-false value and creates a new map that contains only that key/value. Which it's a hard thing to understand why I'd want that, especially since some Map implementations make no guarantee about order. And even if I did want that, I definitely wouldn't want to do that this way. A single for loop could have solved this problem.

Reginald, I don't think there's any absolution for this. Instead, my advice would be to install a carbon monoxide detector in your office, because I have some serious concerns about whether or not your brain is getting enough oxygen.

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

Part of the "fun" of JavaScript is dealing with code which comes from before sensible features existed. For example, if you wanted to clone an object in JavaScript, circa 2013, that was a wheel you needed to invent for yourself, as this StackOverflow thread highlights.

There are now better options, and you'd think that people would use them. However, the only thing more "fun" than dealing with code that hasn't caught up with the times is dealing with developers who haven't, and still insist on writing their own versions of standard methods.

  const objectReplace = (oldObject, newObject) => {
    let keys = Object.keys(newObject)
    try {
      for (let key of keys) {
        oldObject[key] = newObject[key]
      }
    } catch (err) {
      console.log(err, oldObject)
    }     

    return oldObject
  }

It's worth noting that Object.entries returns an array containing both the keys and values, which would be a more sensible for this operation, but then again, if we're talking about using correct functions, Object.assign would replace this function.

There's no need to handle errors here, as nothing about this assignment should throw an exception.

The thing that really irks me about this though is that it pretends to be functional (in the programming idiom sense) by returning the newly modified value, but it's also just changing that value in place because it's a reference. So it has side effects, in a technical sense (changing the value of its input parameters) while pretending not to. Now, I probably shouldn't get too hung up on that, because that's also exactly how Object.assign behaves, but dammit, I'm going to be bothered by it anyway. If you're going to reinvent the wheel, either make one that's substantially worse, or fix the problems with the existing wheel.

In any case, the real WTF here is that this function is buried deep in a 15,000 line file, written by an offshore contract team, and there are at least 5 other versions of this function, all with slightly different names, but all basically doing the same thing, because everyone on the team is just copy/pasting until they get enough code to submit a pull request.

Our submitter wonders, "Is there a way to train an AI to not let people type this?"

No, there isn't. You can try rolling that boulder up a hill, but it'll always roll right back down. Always and forever, people are going to write bad code.

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

Someone wanted to make sure that invalid routes logged an error in their Go web application. Artem found this when looking at production code.

if (requestUriPath != "/config:system") &&
    (requestUriPath != "/config:system/ntp") &&
    (requestUriPath != "/config:system/ntp/servers") &&
    (requestUriPath != "/config:system/ntp/servers/server") &&
    (requestUriPath != "/config:system/ntp/servers/server/config") &&
    (requestUriPath != "/config:system/ntp/servers/server/config/address") &&
    (requestUriPath != "/config:system/ntp/servers/server/config/key-id") &&
    (requestUriPath != "/config:system/ntp/servers/server/config/minpoll") &&
    (requestUriPath != "/config:system/ntp/servers/server/config/maxpoll") &&
    (requestUriPath != "/config:system/ntp/servers/server/config/version") &&
    (requestUriPath != "/config:system/ntp/servers/server/state") &&
    (requestUriPath != "/config:system/ntp/servers/server/state/address") &&
    (requestUriPath != "/config:system/ntp/servers/server/state/key-id") &&
    (requestUriPath != "/config:system/ntp/servers/server/state/minpoll") &&
    (requestUriPath != "/config:system/ntp/servers/server/state/maxpoll") &&
    (requestUriPath != "/config:system/ntp/servers/server/state/version") {
    log.Info("ProcessGetNtpServer: no return of ntp server state for ", requestUriPath)
    return nil
}

The most disturbing part of this, for Artem, isn't that someone wrote this code and pushed it to production. It's that, according to git blame, two people wrote this code, because the first developer didn't include all the cases.

For the record, the application does have an actual router module, which can trigger logging on invalid routes.

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

Today's representative line comes from Capybara James (most recently previously). It's representative, not just of the code base, but of Goodhart's Law: when a measure becomes a target, it ceases to be a good measure. Or, "you get what you measure".

If, for example, you decide that code coverage metrics are how you're going to judge developers, then your developers are going to ensure that the code coverage looks great. If you measure code coverage, then you will get code coverage- and nothing else.

That's how you get tests like this:

Mockito.verify(exportRequest, VerificationModeFactory.atLeast(0)).failedRequest(any(), any(), any());

This test passes if the function exportRequest.failedRequest is called at least zero times, with any input parameters.

Which, as you might imagine, is a somewhat useless thing to test. But what's important is that there is a test. The standards for code coverage are met, the metric is satisfied, and Goodhart marks up another win on the board.

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

One of the things that makes legacy code legacy is that code, over time, rots. Some of that rot comes from the gradual accumulation of fixes, hacks, and kruft. But much of the rot also comes from the tooling going unsupported or entirely out of support.

For example, many years ago, I worked in a Visual Basic 6 shop. The VB6 IDE went out of support in April, 2008, but we continued to use it well into the next decade. This made it challenging to support the existing software, as the IDE frequently broke in response to OS updates. Even when we started running it inside of a VM running an antique version of Windows 2000, we kept running into endless issues getting projects to compile and build.

A fun side effect of that: the VB6 runtime remains supported. So you can run VB6 software on modern Windows. You just can't modify that software.

Greta has inherited an even more antique tech stack. She writes, "I often wonder if I'm the last person on Earth encumbered with this particular stack." She adds, "The IDE is long-deprecated from a vendor that no longer exists- since 2002." Given the project started in the mid 2010s, it may have been a bad choice to use that tech-stack.

It's not as bad as it sounds- while the technology and tooling is crumbling ruins, the team culture is healthy and the C-suite has given Greta wide leeway to solve problems. But that doesn't mean that the tooling isn't a cause of anguish, and even worse than the tooling- the code itself.

"Some things," Greta writes, "are 'typical bad'" and some things "are 'delightfully unique' bad."

For example, the IDE has a concept of "designer" files, for the UI, and "code behind" files, for the logic powering the UI. The IDE frequently corrupts its own internal state, and loses the ability to properly update the designer files. When this happens, if you attempt to open, save, or close a designer file, the IDE pops up a modal dialog box complaining about the corruption, with a "Yes" and "No" option. If you click "No", the modal box goes away- and then reappears because you're seeing this message because you're on a broken designer file. If you click "Yes", the IDE "helpfully" deletes pretty much everything in your designer file.

Nothing about the error message indicates that this might happen.

The language used is a dialect of C++. I say "dialect" because the vendor-supplied compiler implements some cursed feature set between C++98 and C++11 standards, but doesn't fully conform to either. It's only capable of outputting 32-bit x86 code up to a Pentium Pro. Using certain C++ classes, like std::fstream, causes the resulting executable to throw a memory protection fault on exit.

Worse, the vendor supplied class library is C++ wrappers on top of an even more antique Pascal library. The "class" library is less an object-oriented wrapper and more a collection of macros and weird syntax hacks. No source for the Pascal library exists, so forget about ever updating that.

Because the last release of the IDE was circa 2002, running it on any vaguely modern environment is prone to failures, but it also doesn't play nicely inside of a VM. At this point, the IDE works for one session. If you exit it, reboot your computer, or try to close and re-open the project, it breaks. The only fix is to reinstall it. But the reinstall requires you to know which set of magic options actually lets the install proceed. If you make a mistake and accidentally install, say, CORBA support, attempting to open the project in the IDE leads to a cascade of modal error boxes, including one that simply says, "ABSTRACT ERROR" ("My favourite", writes Greta). And these errors don't limit themselves to the IDE; attempting to run the compiler directly also fails.

But, if anything, it's the code that makes the whole thing really challenging to work with. While the UI is made up of many forms, the "main" form is 18,000 lines of code, with absolutely no separation of concerns. Actually, the individual forms don't have a lot of separation of concerns; data is shared between forms via global variables declared in one master file, and then externed into other places. Even better, the various sub-forms are never destroyed, just hidden and shown, which means they remember their state whether you want that or not. And since much of the state is global, you have to be cautious about which parts of the state you reset.

Greta adds:

There are two files called main.cpp, a Station.cpp, and a Station1.cpp. If you were to guess which one owns the software's entry point, you would probably be wrong.

But, as stated, it's not all as bad as it sounds. Greta writes: "I'm genuinely happy to be here, which is perhaps odd given how terrible the software is." It's honestly not that odd; a good culture can go a long way to making wrangling a difficult tech stack happy work.

Finally, Greta has this to say:

We are actively working on a .NET replacement. A nostalgic, perhaps masochistic part of me will miss the old stack and its daily delights.

Wolferitza sends us a large chunk of a C# class. We'll take it in chunks because there's a lot here, but let's start with the obvious problem:

    private int iID0;
    private int iID1;
    private int iID2;
    private int iID3;
    private int iID4;
    private int iID5;
    private int iID6;
    private int iID7;
    private int iID8;
    private int iID9;
    private int iID10;
    private int iID11;
    private int iID12;
    private int iID13;

If you say, "Maybe they should use an array," you're missing the real problem here: Hungarian notation. But sure, yes, they should probably use arrays. And you might think, "Hey, they should use arrays," would be an easy fix. Not for this developer, who used an ArrayList.

private void Basculer(DataTable dtFrom, DataTable dtTo)
{
    ArrayList arrRows = new ArrayList();

    int index;

    DataRow drNew1 = dtTo.NewRow();
    DataRow drNew2 = dtTo.NewRow();
    DataRow drNew3 = dtTo.NewRow();
    DataRow drNew4 = dtTo.NewRow();
    DataRow drNew5 = dtTo.NewRow();
    DataRow drNew6 = dtTo.NewRow();
    DataRow drNew7 = dtTo.NewRow();
    DataRow drNew8 = dtTo.NewRow();
    DataRow drNew9 = dtTo.NewRow();
    DataRow drNew10 = dtTo.NewRow();
    DataRow drNew11 = dtTo.NewRow();
    DataRow drNew12 = dtTo.NewRow();
    DataRow drNew13 = dtTo.NewRow();
    DataRow drNew14 = dtTo.NewRow();
    DataRow drNew15 = dtTo.NewRow();

    arrRows.Add(drNew1);
    arrRows.Add(drNew2);
    arrRows.Add(drNew3);
    arrRows.Add(drNew4);
    arrRows.Add(drNew5);
    arrRows.Add(drNew6);
    arrRows.Add(drNew7);
    arrRows.Add(drNew8);
    arrRows.Add(drNew9);
    arrRows.Add(drNew10);
    arrRows.Add(drNew11);
    arrRows.Add(drNew12);
    arrRows.Add(drNew13);
    arrRows.Add(drNew14);
    arrRows.Add(drNew15);
    // more to come…

Someone clearly told them, "Hey, you should use an array or an array list", and they said, "Sure." There's just one problem: arrRows is never used again. So they used an ArrayList, but also, they didn't use an ArrayList.

But don't worry, they do use some arrays in just a moment. Don't say I didn't warn you.

    if (m_MTTC)
    {
        if (m_dtAAfficher.Columns.Contains("MTTCRUB" + dr[0].ToString()))
        {
            arrMappingNames.Add("MTTCRUB" + dr[0].ToString());
            arrHeadersTexte.Add(dr[4]);
            arrColumnsFormat.Add("");
            arrColumnsAlign.Add("1");

Ah, they're splitting up the values in their data table across multiple arrays; the "we have object oriented programming at home" style of building objects.

And that's all the setup. Now we can get into the real WTF here.

            if (iCompt == Convert.ToInt16(0))
            {
                iID0 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(1))
            {
                iID1 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(2))
            {
                iID2 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(3))
            {
                iID3 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(4))
            {
                iID4 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(5))
            {
                iID5 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(6))
            {
                iID6 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(7))
            {
                iID7 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(8))
            {
                iID8 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(9))
            {
                iID9 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(10))
            {
                iID10 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(11))
            {
                iID11 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(12))
            {
                iID12 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(13))
            {
                iID13 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
        }
    }

Remember those private iID* values? Here's how they get populated. We check a member variable called iCompt and pull the first value out of a dr variable (a data reader, also a member variable). You may have looked at the method signature and assumed dtFrom and dtTo would be used, but no- they have to purpose in this method at all.

And if you liked what happened in this branch of the if, you'll love the else:

    else
    {
        if (m_dtAAfficher.Columns.Contains("MTTHRUB" + dr[0].ToString()))
        {
            arrMappingNames.Add("MTTHRUB" + dr[0].ToString());
            arrHeadersTexte.Add(dr[4]);
            arrColumnsFormat.Add("");
            arrColumnsAlign.Add("1");

            if (iCompt == Convert.ToInt16(0))
            {
                iID0 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(1))
            {
                iID1 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(2))
            {
                iID2 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(3))
            {
                iID3 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(4))
            {
                iID4 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(5))
            {
                iID5 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(6))
            {
                iID6 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(7))
            {
                iID7 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(8))
            {
                iID8 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(9))
            {
                iID9 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(10))
            {
                iID10 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(11))
            {
                iID11 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(12))
            {
                iID12 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
            else if (iCompt == Convert.ToInt16(13))
            {
                iID13 = Convert.ToInt32(dr[0]);
                iCompt = iCompt + 1;
            }
        }
    }
}

I can only assume that this function is called inside of a loop, though I have to wonder about how that loop exits? Maybe I'm being too generous, this might not be called inside of a loop, and the whole class gets to read 13 IDs out before it's populated. Does iCompt maybe get reset somewhere? No idea.

Honestly, does this even work? Wolferitza didn't tell us what it's actually supposed to do, but found this code because there's a bug in there somewhere that needed to be fixed. To my mind, "basically working" is the worst case scenario- if the code were fundamentally broken, it could just be thrown away. If it mostly works except for some bugs (and terrible maintainability) no boss is going to be willing to throw it away. It'll just fester in there forever.

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

Handling non-existent values always presents special challenges. We've (mostly) agreed that NULL is, in some fashion, the right way to do it, though it's still common to see some sort of sentinel value that exists outside of the expected range- like a function returning a negative value when an error occurred, and a zero (or positive) value when the operation completes.

Javier found this function, which has a… very French(?) way of handling invalid dates.

 Private Function CheckOraDate(ByVal sDate As String) As String
        Dim OraOValDate As New DAL.PostGre.DataQuery()
        Dim tdate As Date
        If IsDate(sDate) Then
            Return IIf(OraOValDate.IsOracle, CustomOracleDate(Convert.ToDateTime(sDate).ToString("MM-dd-yyyy")), "'" & sDate & "'")
        Else
            '~~~ No Date Flag of Bastille Day
            Return CustomOracleDate(Convert.ToDateTime("07/14/1789").ToString("MM-dd-yyyy"))
        End If

    End Function

Given a date string, we check if it is a valid date string using IsDate. If it is, we check if our data access layer thinks the IsOracle flag is set, and if it is, we do some sort of conversion to a `CustomOracleDate", otherwise we just return the input wrapped in quotes.

All that is sketchy- any function that takes dates as a string input and then returns the date in a new format as a string always gets my hackles up. It implies loads of stringly typed operations.

But the WTF is how we handle a bad input date: we return Bastille Day.

In practice, this meant that their database system was reporting customers' birthdays as Bastille Day. And let me tell you, those customers don't look a day over 200, let alone 236.

For an extra bonus WTF, while the "happy path" checks if we should use the custom oracle formatting, the Bastille Day path does not, and just does whatever the Oracle step is every time.

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

Alicia recently moved to a new country and took a job with a small company willing to pay well and help with relocation costs. Overall, the code base was pretty solid. Despite the overall strong code base, one recurring complaint was that running the test suite was painfully long.

While Alicia doesn't specify what the core business is, but says: "in this company's core business, random numbers were the base of everything."

As such, they did take generating random numbers fairly seriously, and mostly used strong tools for doing that. However, whoever wrote their test suite was maybe a bit less concerned, and wrote this function:

public static Long generateRandomNumberOf(int length) {
    while (true) {
            long numb = (long)(Math.random() * 100000000 * 1000000); // had to use this as int's are to small for a 13 digit number.
            if (String.valueOf(numb).length() == length)
                return numb;		
        }		
}

They want many digits of random number. So they generate a random floating point, and then multiply it a few times to get a large number. If the length of the resulting number, in characters, is the desired length, we return it. Otherwise, we try again.

The joy here, of course, is that this function is never guaranteed to exit. In fact, if you request more than 15 digits, it definitely won't exit. In practice, most of the time, the function is able to hit the target length in a relative handful of iterations, but there's no guarantee for that.

Alicia was tracking down a bug in a test which called this function. So she went ahead and fixed this function so that it use a sane way to generate the appropriate amount of entropy that actually guaranteed a result. She included that change in her pull request, nobody had any comments, and it got merged in.

The unit tests aren't vastly faster than they were, but they are faster. Who knows what other surprises the test suite has in store?

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

Brian (previously)found himself contracting for an IoT company, shipping thermostats and other home automation tools, along with mobile apps to control them.

Brian was hired because the previous contractor had hung around long enough for the product to launch, cashed the check, and vanished, never to be heard from again.

And let's just say that Brian's predecessor had a unique communication style.

    private class NoOpHandler extends AsyncCharisticHandler {
        public NoOpHandler(Consumer<BluetoothGattCharacteristic> consumer) {
            super(null, consumer);
        }

        @Override
        public void Execute() {
            // Don't do any actual BLE Communication here, and just go straight to the callback, This
            // handler is just to allow people to get a callback after after a bunch of Async IO Operations
            // have happened, without throwing all the completion logic into the "last" async callback of your batch
            // since the "last" one changes.
            InvokeCallback();
            // After this callback has been handled, recheck the queue to run any subsequent Async IO Operations
            OnBLEAsyncOperationCompleted();
            // I'm aware this is recursive. If you get a stack overflow here, you're using it wrong.
            // You're not supposed to queue thousands of NoOp handlers one after the other, Stop doing it!
            // If you need to have code executed sequentially just, er... write a fu*king function there is
            // nothing special about this callback, or the thread it is called on, and you don't need to use
            // it for anything except getting a callback after doing a batch of async IO, and then, it runs 
            // in the context of the last IO Completion callback, which shouldn't take ages. If you use 
            // AsyncRunWhenCompleted() to create more of these within the callback of AsyncRunWhenCompleted
            // it just keeps the IO completion thread busy, which also breaks shit. 
            // Basically, you shouldn't be using AsyncRunWhenCompleted() at all if you're not me.
        }
    }

Who said bad programmers don't write comments? This bad programmer wrote a ton of comments. What's funny about this is that, despite the wealth of comments, I'm not 100% certain I actually know what I'm supposed to do, aside from not use AsyncRunWhenCompleted.

The block where we initialize the Bluetooth system offers more insight into this programmer's style.

    @SuppressLint("MissingPermission")
    private void initializeBluetooth() {
        _bluetoothManager = (BluetoothManager) getSystemService(BLUETOOTH_SERVICE);
        _bluetoothAdapter = _bluetoothManager != null ? _bluetoothManager.getAdapter() : null;
        if (_bluetoothAdapter != null && _bluetoothAdapter.isEnabled()) {
            /* #TODO: I don't know if cancelDiscovery does anything... either good, or bad. It seems to make BLE discovery faster after 
            *         the service is restarted by android, but I don't know if it screws anything else up in the process. Someone should check into that */
            _bluetoothAdapter.cancelDiscovery();
            _bluetoothScanner = _bluetoothAdapter.getBluetoothLeScanner();
            _scanFilters = Collections.singletonList(new ScanFilter.Builder().setServiceUuid(new ParcelUuid(BLE_LIVELINK_UUID)).build());
            CreateScanCallback();
        } else {
            // #TODO: Handle Bluetooth not available or not enabled
            stopSelf();
        }
    }

This is a clear example of "hacked together till it works". What does cancelDiscovery do? No idea, but we call it anyway because it seems to be faster. Should we look it up? Because yes, it sounds like calling it is correct, based on the docs. Which took me 15 seconds to find. "Someone should check into that," and apparently I am that someone.

Similarly, the second TODO seems like an important missing feature. At least a notification which says, "Hey, you need bluetooth on to talk to bluetooth devices," would go a long way.

All this is in service of an IoT control app, which seems to double as a network scanner. It grabs the name of every Bluetooth and WiFi device it finds, and sends them and a location back to a web service. That web service logs them in a database, which nobody at the company ever looks at. No one wants to delete the database, because it's "valuable", though no one can ever specify exactly how they'd get value from it. At best, they claim, "Well, every other app does it." Mind you, I think we all know how they'd get value: sell all this juicy data to someone else. It's just no one at the company is willing to say that out loud.

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

Dotan was digging through vendor supplied documentation to understand how to use an API. To his delight, he found a specific function which solved exactly the problem he had, complete with examples of how it was to be used. Fantastic!

He copied one of the examples, and hit compile, and reviewed the list of errors. Mostly, the errors were around "the function you're calling doesn't exist". He went back to the documentation, checked it, went back to the code, didn't find any mistakes, and scratched his head.

Now, it's worth noting the route Dotan took to find the function. He navigated there from a different documentation page, which sent him to an anchor in the middle of a larger documentation page- vendorsite.com/docs/product/specific-api#specific-function.

This meant that as the page loaded, his browser scrolled directly down to the specific-function section of the page. Thus, Dotan missed the gigantic banner at the top of the page for that API, which said this:

/!\ NOTE /!\ NOTE /!\ NOTE /!\ NOTE /!\ NOTE /!\ NOTE /!\ NOTE /!
This doc was written to help flesh out a user API. The features described here are all hypothetical and do not actually exist yet, don't assume anything you see on this page works in any version /!\ NOTE /!\ NOTE /!\ NOTE /!\ NOTE /!\ NOTE /!\ NOTE /!\ NOTE /!\

On one hand, I think providing this kind of documentation is invaluable, both to your end users and for your own development team. It's a great roadmap, a "documentation driven development" process. And I can see that they made an attempt to be extremely clear about it being incomplete and unimplemented- but they didn't think about how people actually used their documentation site. A banner at the top of the page only works if you read the page from top to bottom, but documentation pages you will frequently skip to specific sections of the page.

But there was a deeper issue with the way this particular approach was executed: while the page announced that one shouldn't assume anything works, many of the functions on the page did work. Many did not. There was no rhyme or reason, to version information or other indicators to help a developer understand what was and was not actually implemented.

So while the idea of a documentation-oriented roadmap specifying features that are coming is good, the execution here verged into WTF territory. It was a roadmap, but with all the landmarks erased, so you had no idea where you actually were along the length of that road. And the one warning sign that would help you was hidden behind a bush.

Dotan asks: "WTF is that page doing on the official documentation wiki?"

And I'd say, I understand why it's there, but boy it should have been more clear about what it actually was.

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

Years ago, Brian had a problem: their C# application would crash sometimes. What was difficult to understand was why it was crashing, because it wouldn't crash in response to a user action, or really, any easily observable action.

The basic flow was that the users used a desktop application. Many operations that the users wanted to perform were time consuming, so the application spun up background tasks to do them, thus allowing the user to do other things within the application. And sometimes, the application would just crash, both when the user hadn't done anything, and when all background jobs should have been completed.

The way the background task was launched was this:

seeder.RunSeeder();

It didn't take too much head scratching to realize what was running every time the application crashed: the garbage collector.

RunSeeder returned a Task object, but since Brian's application treated the task as "fire and forget", they didn't worry about the value itself. But C# did- the garbage collector had to clean up that memory.

And this was running under .Net 4.0. This particular version of the .Net framework was a special, quantum realm, at least when it came to tasks. You see, if a Task raises an exception, nothing happens. At least, not right away. No one is notified of the exception unless they inspect the Task object directly. There's a cat in the box, and no one knows the state of the cat unless they open the box.

The application wasn't checking the Task result. The cat remained in a superposition of "exception" and "no exception". But the garbage collector looked at the task. And, in .Net 4.0, Microsoft made a choice about what to do there: when they opened the box and saw an exception (instead of a cat), they chose to crash.

Microsoft's logic here wasn't entirely bad; an uncaught exception means something has gone wrong and hasn't been handled. There's no way to be certain the application is in a safe state to continue. Treating it akin to undefined behavior and letting the application crash was a pretty sane choice.

The fix for Brian's team was simple: observe the exception, and choose not to do anything with it. They truly didn't care- these tasks were fire-and-forget, and failure was acceptable.

seeder.RunSeeder().ContinueWith(t => { var e = t.IsFaulted ? t.Exception : null; }); // Observe exceptions to prevent quantum crashes

This code merely opens the box and sees if there's an exception in there. It does nothing with it.

Now, I'd say as a matter of programming practice, Microsoft was right here. Ignoring exceptions blindly is a definite code smell, even for a fire-and-forget task. Writing the tasks in such a way as they catch and handle any exceptions that bubble up is better, as is checking the results.

But I, and Microsoft, were clearly on the outside in this argument. Starting with .Net 4.5 and moving forward, uncaught exceptions in background tasks were no longer considered show-stoppers. Whether there was a cat or an exception in the box, when the garbage collector observed it, it got thrown away either way.

In the end, this reminds me of my own failing using background tasks in .Net.

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

The first time Z hit the captcha on his company's site, he didn't think much of it. And to be honest, the second time he wasn't paying that much attention. So it wasn't until the third time that he realized that the captcha had showed him the same image every single time- a "5" with lines scribbled all over it.

That led Z to dig out the source and see how the captcha was implemneted.

<Center>Click a number below to proceed to the next page. <br>Some browsers do not like this feature and will try to get around it. If you are having trouble<br> seeing the image, empty your internet cache and temporary internet files may help. <br>Please ensure you have no refresher add-ons installed on your browser.<br />
<table border=1><Tr><td colspan='3' align='center'>
<font style='font-size:36px;'><img width='150' title='5' alt='5' src='valimages/5.gif'> </font></td></tr>
<Tr>
<Td align=center><font size='6'><A href='valid.php?got=crimied&linknum=1'>1</a></font></td>
<Td align=center><font size='6'><A href='valid.php?got=crimied&linknum=2'>2</a></font></td>
<Td align=center><font size='6'><A href='valid.php?got=crimied&linknum=3'>3</a></font></td>
</tr>
<Tr>
<Td align=center><font size='6'><A href='valid.php?got=crimied&linknum=4'>4</a></font></td>
<Td align=center><font size='6'><A href='valid.php?got=crimied&linknum=5'>5</a></font></td>
<Td align=center><font size='6'><A href='valid.php?got=crimied&linknum=6'>6</a></font></td>
</tr>
<tr>
<Td align=center><font size='6'><A href='valid.php?got=crimied&linknum=7'>7</a></font></td>
<Td align=center><font size='6'><A href='valid.php?got=crimied&linknum=8'>8</a></font></td>
<Td align=center><font size='6'><A href='valid.php?got=crimied&linknum=9'>9</a></font></td>
</tr></table>
</Center>

Look, I know there's a joke about how hard it is to center things in CSS, but I think we've gone a little overboard with our attempt here.

Now, the PHP driving this page could have easily been implemented to randomly select an image from the valimages directory, and there was some commented out code to that effect. But it appears that whoever wrote it couldn't quite understand how to link the selected image to the behavior in valid.php, so they just opted to hard code in five as the correct answer.

The bonus, of course, is that the image for five is named 5.gif, which means if anyone really wanted to bypass the captcha, it'd be trivial to do so by scraping the code. I mean, not more trivial than just realizing "it's the same answer every time", but still, trivial.

Of course, out here in the real world, captchas have never been about keeping bots out of sites, and instead are just a way to trick the world into training AI. Pretty soon we'll roll out the Voight-Kampf test, but again, the secret purpose won't be to find the replicants, but instead gather data so that the next generation of replicants can pass the test.

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

Way back in 1964, people were starting to recgonize that computers were going to have a large impact on the world. There was not, at the time, very much prepackaged software, which meant if you were going to use a computer to do work, you were likely going to have to write your own programs. The tools to do that weren't friendly to non-mathematicians.

Thus, in 1964, was BASIC created, a language derived from experiments with languages like DOPE (The Dartmouth Oversimplified Programming Experiment). The goal was to be something easy, something that anyone could use.

In 1977, the TRS-80, the Commodore PET, and the Apple II all launched, putting BASIC into the hands of end users. But it's important to note that BASIC had already been seeing wide use for a decade on "big iron" systems, or more hobbyist systems, like the Altair 8800.

Today's submitter, Coyne, was but a humble student in 1977, and despite studying at a decent university, brand spanking new computers were a bit out of reach. Coyne was working with professors to write code to support papers, and using some dialect of BASIC on some minicomputer.

One of Coyne's peers had written a pile of code, and one simple segment didn't work. As it was just a loop to print out a series of numbers, it seemed like it should work, and work quite easily. But the programmer writing it couldn't get it to work. They passed it around to other folks in the department, and those folks also couldn't get it to work. What could possibly be wrong with this code?

3010 O = 45
3020 FOR K = 1 TO O
3030   PRINT K
3040 NEXT K

Now, it's worth noting, this particular dialect of BASIC didn't support long variable names- you could use a single letter, or you could use a letter and a number, and that was it. So the short variable names are not explicitly a problem here- that's just the stone tools which were available to programmers at the time.

For days, people kept staring at this block, trying to figure out what was wrong. Finally, Coyne took a glance, and in a moment was able to spot the problem.

I've done something nasty here, because I posted the correct block first. What the programmer had actually written was this:

3010 O = 45
3020 FOR K = 1 TO 0
3030   PRINT K
3040 NEXT K

The difference is subtle, especially when you're staring at a blurry CRT late at night in the computer lab, with too little coffee and too much overhead lighting. I don't know what device they were using for display; most terminals made sure to make O look different from 0, but I couldn't be bold enough to say all of them did. And, in this era, you'd frequently review code on printed paper, so who knows how it was getting printed out?

But that, in the end, was the problem- the programmer accidentally typed a zero where they meant the letter "O". And that one typo was enough to send an entire computer science department spinning for days when no one could figure it out.

In any case, it's interesting to see how an "easy" to use language once restricted variable names to such deep inscrutability.

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

For C programmers of a certain age (antique), booleans represent a frustrating challenge. But with the addition of stdbool.h, we exited the world of needing to work hard to interact with boolean values. While some gotchas are still in there, your boolean code has the opportunity to be simple.

Mark's predecessor saw how simple it made things, and decided that wouldn't do. So that person went and wrote their own special way of comparing boolean values. It starts with an enum:

typedef enum exop_t {
    EXOP_NONE,
    EXOP_AND,
    EXOP_OR,
    EXOP_EQUAL,
    EXOP_NOTEQUAL,
    EXOP_LT,
    EXOP_GT,
    EXOP_LEQUAL,
    EXOP_GEQUAL,
    EXOP_ADD,
    EXOP_SUBTRACT,
    EXOP_MULTIPLY,
    EXOP_DIV,
    EXOP_MOD,
    EXOP_NEGATE,
    EXOP_UNION,
    EXOP_FILTER1,
    EXOP_FILTER2
};

Yes, they did write an enum to compare booleans. They also wrote not one, but two functions. Let's start with the almost sane one.

static bool compare_booleans (bool bool1,
                              bool bool2,
                              exop_t  exop)
{

    int32_t  cmpresult;

    if ((bool1 && bool2) || (!bool1 && !bool2)) {
        cmpresult = 0;
    } else if (bool1) {
        cmpresult = 1;
    } else {
        cmpresult = -1;
    }

    return convert_compare_result(cmpresult, exop);

}

This function takes two boolean values, and a comparison we wish to perform. Then, we test if they're equal, though the way we do that is by and-ing them together, then or-ing that with the and of their negations. If they're equal, cmpresult is set to zero. If they're not equal, and the first boolean is true, we set cmpresult to one, and finally to negative one.

Thus, we're just invented strcmp for booleans.

But then we call another function, which is super helpful, because it turns that integer into a more normal boolean value.

static boolean
    convert_compare_result (int32_t cmpresult,
                            exop_t exop)
{
    switch (exop) {
    case EXOP_EQUAL:
        return (cmpresult) ? FALSE : TRUE;
    case EXOP_NOTEQUAL:
        return (cmpresult) ? TRUE : FALSE;
    case EXOP_LT:
        return (cmpresult < 0) ? TRUE : FALSE;
    case EXOP_GT:
        return (cmpresult > 0) ? TRUE : FALSE;
    case EXOP_LEQUAL:
        return (cmpresult <= 0) ? TRUE : FALSE;
    case EXOP_GEQUAL:
        return (cmpresult >= 0) ? TRUE : FALSE;
    default:
        printf( "ERR_INTERNAL_VAL\n" );
        return TRUE;
    }
} 

We switch based on the requested operation, and each case is its own little ternary. For equality comparisons, it requires a little bit of backwards logic- if cmpresult is non-zero (thus true), we need to return FALSE. Also note how our expression enum has many more options than convert_compare_result supports, making it very easy to call it wrong- and worse, it returns TRUE if you call it wrong.

At least they made booleans hard again. Who doesn't want to be confused about how to correctly check if two boolean values are the same?

It's worth noting that, for all this code, the rest of the code base never used anything but EXOP_EQUAL and EXOP_NOTEQUAL, because why would you do anything else on booleans? Every instance of compare_booleans could have been replaced with a much clearer == or != operator. Though what should really have been replaced was whoever wrote this code, preferably before they wrote it.

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

Back in the antediluvian times, when I was in college, people still used floppy disks to work on their papers. This was a pretty untenable arrangement, because floppy disks lost data all the time, and few students had the wherewithal to make multiple copies. Half my time spent working helldesk was breaking out Norton Diskutils to try and rescue people's term papers. To avoid this, the IT department offered network shares where students could store documents. The network share was backed up, tracked versions, and could be accessed from any computer on campus, including the VAX system (in fact, it was stored on the VAX).

I bring this up because we have known for quite some time that companies and governments need to store documents in centrally accessible locations so that you're not reliant on end users correctly managing their files. And if you are a national government, you have to make a choice: either you contract out to a private sector company, or you do it yourself.

South Korea made the choice to do it themselves, with their G-Drive system (short for Government Drive, no relation to Google Drive), a government file store hosted primarily out of a datacenter in Daejeon. Unfortunately, "primarily" is a bit too apropos- last month, a fire in that datacenter destroyed data.

The Interior Ministry explained that while most systems at the Daejeon data center are backed up daily to separate equipment within the same center and to a physically remote backup facility, the G-Drive’s structure did not allow for external backups. This vulnerability ultimately left it unprotected.

Someone, somehow, designed a data storage system that was structurally incapable of doing backups? And then told 750,000 government employees that they should put all their files there?

Even outside of that backup failure, while other services had backups, they did not have a failover site, so when the datacenter went down, the government went down with it.

In total, it looks like about 858TB of data got torched. 647 distinct services were knocked out. At least 90 of them were reported to be unrecoverable (that last link is from a company selling Lithium Ion safety products, but is a good recap). A full recovery was, shortly after the accident, predicted to take a month, but as of October 22, only 60% of services had been restored.

Now, any kind of failure of this scale means heads must roll, and police investigations have gone down the path of illegal subcontracting. The claim is that the government hired a contractor broke the law by subcontracting the work, and that those subcontractors were unqualified for the work they were doing- that while they were qualified to install or remove a li-ion battery, they were not qualified to move one, which is what they were doing and resulted in the fire.

I know too little about Korean laws about government contracting and too little about li-ion battery management to weigh in on this. Certainly, high-storage batteries are basically bombs, and need to be handled with great care and protected well. Though, if one knows how to install and uninstall a battery, moving a battery seems covered in those steps.

But if I were doing a root cause analysis here, while that could be the root cause of the fire, it is not the root cause of the outage. If you build a giant datacenter but can't replicate services to another location, you haven't built a reliable cloud storage system- you've just built an expensive floppy disk that is one trip too close to a fridge magnet away from losing all of your work. In this case, the fridge magnet was made of fire, but the result is the same.

I'm not going to say this problem would have be easy to avoid; actually building resilient infrastructure that fails gracefully under extreme stress is hard. But while it's a hard problem, it's also a well-understood problem. There are best practices, and clearly not one of them was followed.

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

A ticket came in marked urgent. When users were entering data in the header field, the spaces they were putting in kept getting mangled. This was in production, and had been in production for sometime.

Mike P picked up the ticket, and was able to track down the problem to a file called Strings.java. Yes, at some point, someone wrote a bunch of string helper functions and jammed them into a package. Of course, many of the functions were re-implementations of existing functions: reinvented wheels, now available in square.

For example, the trim function.

    /**
     * @param str
     * @return The trimmed string, or null if the string is null or an empty string.
     */
    public static String trim(String str) {
        if (str == null) {
            return null;
        }

        String ret = str.trim();

        int len = ret.length();
        char last = '\u0021';    // choose a character that will not be interpreted as whitespace
        char c;
        StringBuffer sb = new StringBuffer();
        for (int i = 0; i < len; i++) {
            c = ret.charAt(i);
            if (c > '\u0020') {
                if (last <= '\u0020') {
                    sb.append(' ');
                }
                sb.append(c);
            }
            last = c;
        }
        ret = sb.toString();

        if ("".equals(ret)) {
            return null;
        } else {
            return ret;
        }
    }