This week, a double dose of Daniel D.

First he shared a lesson he titled "Offer you can't refuse a.k.a. Falsehood programmers believe about prices" explaining "Some programmers believe that new prices per month (when paid annually) are always better then the old ones (when paid monthly). Only this time they have forgotten their long-time clients on legacy packages."

Then he found a few more effs. "This e-shop required to create an account to download an invoice for order already delivered. Which is kind of WTF on its own. But when I pasted a generated 62 mixed character (alphanumeric+special) password, their form still insisted on entering 8+ characters. not correct. Well, because their programmers didn't expect somebody to paste a password. Once I did another JS event - e.g. clicked a submit button, it fixed itself."

And our Best Beastie in Black discovered "Anomalies in the causal structure of our particular 4-dimensional Lorentzian manifold have apparently caused this secure message portal belonging to a tax prep/audit company to count emails that haven't yet been sent by sender."

Traveler Tim R. struggled to pay for a visa, and reports this result. Rather than an error reported as success, we appear to have here a success reported as an error. "We're all familiar with apps that throw up an eror dialog with the error message as success but it's particularly irritating when trying to submit a payment. This is what happened when I tried to pay for an Indian visa with Paypal. To add insult to injury, when you try to pay again, it says that due to errors and network problems, you must check back in 2 hours before attempting a repeat payment."

Finally Robert H. is all charged up about Chevy shenanigans. "I thought one of the advantages of EV vehicles was they don't need oil changes?"

Eli sends us something that's not quite a code sample, despite coming from code. It's not a representative line, because it's many lines. But it certainly is representative.

Here's the end of one of their code files:

													});
												}
											}
										);
									});
								}
							)
						);
					}
				});
			}
		}
	);
});

I feel like someone heard that JavaScript could do functional programming and decided to write LISP. That's a lot of nested blocks. I don't know what the code looks like, but also I don't want to know what the code looks like.

Also, as someone who programs with a large font size, this is a special kind of hell for me.

As often happens, Luka started some work but didn't get it across the finish line before a scheduled vacation. No problem: just hand it off to another experienced developer.

Luka went off for a nice holiday, the other developer hammered away at code, and when Luka came back, there was this lovely method already merged to production, sitting and waiting:

vvv(x, y)
{
	return typeof x[y] !== 'undefined';
}

"What is this?" Luka asked.

"Oh, it's a helper function to check if a property is defined on an object."

Luka could see that much, but that didn't really answer the question.

First, it wasn't the correct way to check if a property existed. Mind you, actually doing those checks in JavaScript is a complicated minefield because of prototype inheritance, but between the in operator, the hasOwn and hasOwnProperty methods, there are simpler and cleaner ways to get there.

But of course, that wasn't what got anyone's attention. What caught Luka up was the name of the function: vvv. And not only was it a terrible name, thanks to the other dev's industriousness, it was now called all over the codebase. Even places where a more "correct" call had been used had been refactored to use this method.

"But it's so brief, and memorable," the developer said.

Luka was vvvery upset by that attitude.

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

"We use a three tier architecture," said the tech lead on Cristian's new team. "It helps us keep concerns separated."

This statement, as it turned out, was half true. They did divide the application into three tiers- a "database layer", a "business layer", and a "presentation layer". The "database layer" was a bunch of Java classes. The "business layer" was a collection of Servlets. And the "presentation layer" was a pile of JSP files.

What they didn't do, however, was keep the concerns separated.

Here's some code from their database layer:

public synchronized StringBuffer getStocTotGest(String den, String gest) {
		StringBuffer sb = new StringBuffer("<table width=\"100%\"  border=\"1\" cellspacing=\"1\" cellpadding=\"1\">" + "<tr bgcolor=\"#999999\">" + "<td>Denumire</td>" + "<td>Cant</td>"
				+ "<td>PretVanz</td>" + "</tr>");
		try {
			ResultSet rs = connectionManager
					.executeQuery("select (if(length(SUBSTRING(den,1,instr(den,'(')-1))>0,SUBSTRING(den,1,instr(den,'(')-1),den)) as den,um,pret_vinz,sum(stoc) as stoc from stmarfzi_poli where den like '"
							+ den + "%' " + gest + "  group by den  order by den");
			while (rs.next()) {
				sb.append("<tr><td>" + rs.getString("den") + "</td>");
				sb.append("<td><div align=\"right\">" + threeDecimalPlacesFormat.format(rs.getDouble("stoc")) + " " + rs.getString("um") + "</div></td>");
				sb.append("<td><div align=\"right\">" + teoDecimalPlacesFormat.format(rs.getDouble("pret_vinz")) + "</div></td></tr>");
			}
			sb.append("</table>");
		} catch (Exception ex) {
			ex.printStackTrace();
		}
		return sb;
	}

I guess a sufficiently motivated programmer can write PHP in any language.

This just has a little bit of everything in it, doesn't it? There's the string-munged HTML generation in the database layer. The HTML is also wrong, as header fields are output with td tags, instead of th. There's the SQL injection vulnerability. There's the more-or-less useless exception handler. It's synchronized even though it's not doing anything thread unsafe. It's truly a thing of beauty, at least if you don't know what beauty is and think it means something horrible.

This function was used in a few places. It was called from a few servlets in the "business layer", where the resulting StringBuffer was dumped into a session variable so that JSP files could access it. At least, that was for the JSP files which didn't invoke the function themselves- JSP files which mixed all the various layers together.

Cristian's first task in the code base was changing the background colors of all of the rendered table headers. Since, as you can see, they weren't using CSS to make this easy, that involved searching through the entire codebase, in every layer, to find all the places where maybe a table was generated.

Changing those colors was Cristian's first task in the code base. I assume that Cristian is still working on that, and will be working on that for some time to come.

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

Alexandra inherited a codebase that, if we're being kind, could be called "verbose". Individual functions routinely cross into multiple thousands of lines, with the longest single function hitting 4,000 lines of code.

Very little of this is because the problems being solved are complicated, and much more of it is because people don't understand how anything works.

For example, in this C++ code, they have a vector of strings. The goal is to create a map where the keys are the strings from the vector, and the values are more strings, derived from a function call.

Essentially, what they wanted was:

for (std::string val : invec)
{
    umap[val] = lookupValue(val);
}

This would have been the sane, obvious way to do things. That's not what they did.

unordered_map<string, string> func(vector<string> invec)
{
    unordered_map<string, string> umap;
    vector<pair<string, string*> idxvec;
    for(string name : invec)
    {
        umap[name] = "";
        idxvec.push_back(make_pair(name, &umap[name]));
    }   

    for(auto thingy : idxvec)
    {
        //actual work, including assigning the string
        thingy.get<1>() = lookupValue(thingy.get<0>()); 
    }
    return umap;
}

I won't pick on names here, as they're clearly anonymized. But let's take a look at the approach they used.

They create their map, and then create a new vector- a vector which is a pair<string, string*>- a string and a pointer to a string. Already, I'm confused by why any of this is happening, but let's press on and hope it becomes clear.

We iterate across our input vector, which this I get. Then we create a key in the map and give it an empty string as a value. Then we create a pair out of our key and our pointer to that empty string. That's how we populate our idxvec vector.

Once we've looped across all the values once, we do it again. This time, we pull out those pairs, and set the value at the pointer equal to the string returned by lookup value.

Which leads us all to our favorite letter of the alphabet: WHY?

I don't know. I also am hesitant to comment to much on the memory management and ownership issues here, as with the anonymization, there may be some reference management that got lost. But the fact that we're using bare pointers certainly makes this code more fraught than it needed to be. And, given how complex the STL data structures can be, I think we can also agree that passing around bare pointers to memory inside those structures is a recipe for disaster, even in simple cases like this.

What I really enjoy is that they create a vector of pairs, without ever seeming to understand that a list of pairs is essentially what a map is.

In conclusion: can we at least agree that, from now on, we won't iterate across the same values twice? I think about 15% of WTFs would go away if we all followed that rule.

Oh, wait, no. People who could understand rules like that aren't the ones writing this kind of code. Forget I said anything.

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

...or actually, it doesn't. A few fans found figures that just didn't add up. Here they are.

Steven J Pemberton deserves full credit for this finding. "My bank helpfully reminds me when it's time to pay my bill, and normally has no problem getting it right. But this month, the message sent Today 08:02, telling me I had to pay by tomorrow 21-Nov was sent on... 21-Nov. The amount I owed was missing the decimal point. They then apologised for freaking me out, but got that wrong too, by not replacing the placeholder for the amount I really needed to pay. "

Faithful Michael R. levels a charge of confusion against what looks like.. Ticketmaster, maybe? "My card indeed ends with 0000. Perhaps they do some weird math with their cc numbers to store them as numerics." It's not so much weird math as simply reification. Your so called "credit card number" is not actually a number; it is a digit string. And the last four digits are also a digit string.

Marc Würth, who still uses Facebook, gripes that their webdevs also don't understand the difference between numbers and digit strings. "Clicking on Mehr dazu (Learn more), tells me:
> About facebook.com on older versions of mobile browsers
> [...]
> Visit facebook.com from one of these browsers, if it’s available to download on your mobile device:
> [...]
> Firefox (version 48 or higher)
> [...]
Um... Facebook, guess what modern mobile web browser I'm viewing you, right now? [132.0.2 from 2024-11-10] "

Self-styled dragoncoder047 is baffled by what is probably a real simple bug in some display logic reporting the numerator where it should display the denominator (2). Grumbles DC "Somebody please explain to me how 5+2+2+2+2+2+2+0.75+2+2=23. If WebAssign itself can't even master basic arithmetic, how can I trust it teaching me calculus? "

Finally Andrew C. has a non-mathematical digit or two to share, assuming you're inclined to obscure puns. "As well as having to endure the indignity of job seeking, now I get called names too!" This probably requires explanation for those who are not both native speakers of the King's English and familiar with cryptographic engineering.

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

Today is holiday in the US, where we celebrate a cosplay version of history with big meals and getting frustrated with our family. It's also a day where we are thankful- usually to not be at work, but also, thankful to not work with Brad. Original --Remy

Anita parked outside the converted garage, the printed graphic reading Global Entertainment Strategies (GES) above it. When the owner, an old man named Brad, had offered her a position after spotting her in a student computer lab, she thought he was crazy, but a background check confirmed everything he said. Now she wondered if her first intuition was correct.

“Anita, welcome!” Brad seemed to bounce like a toddler as he showed Anita inside. The walls of the converted garage were bare drywall; the wall-mounted AC unit rattled and spat in the corner. In three corners of the office sat discount computer desks. Walls partitioned off Brad’s office in the fourth corner.

He practically shoved Anita into an unoccupied desk. The computer seemed to be running an unlicensed version of Windows 8, with no Office applications of any kind. “Ross can fill you in!” He left the office, slamming the door shut behind him.

“Hi.” Ross rolled in his chair from his desk to Anita’s. “Brad’s a little enthusiastic sometimes.”

“I noticed. Uh, he never told me what game we’re working on, or what platform. Not even a title.”

Ross’s voice lowered to a whisper. “None of us know, either. We’ve been coding in Unity for now. He hired you as a programmer, right? Well, right now we just need someone to manage our documentation. I suggest you prepare yourself.”

Ross led Anita into Brad’s office. Above a cluttered desk hung a sagging whiteboard. Every square inch was covered by one, sometimes several, overlapping sticky notes. Each had a word or two written in Brad’s scrawl.

“We need more than just random post-its with ‘big guns!’ and ‘more action!’” Ross said. “We don’t even know what the title is! We’re going crazy without some kind of direction.”

Anita stared at the wall of sticky notes, feeling her sanity slipping from her mind like a wet noodle. “I’ll try.”

Sticky Escalation

Brad, can we switch to Word for our documentation? It’s getting harder
to read your handwriting, and there’s a lot of post-its that have
nothing to do with the game. This will make it easier to proceed with
development. -Anita

Two minutes after she sent the email, Brad barged out of his office. “Anita, why spend thousands of dollars on software licenses when this works just fine? If you can’t do your job with the tools you have, what kind of a programmer does that make you?”

“Brad, this isn’t going to work forever. Your whiteboard is almost out of room, and you won’t take down any of your non-game stickies!”

“I can’t take any of them down, Anita! Any of them!” He slammed the door to his office behind him.

The next day, Anita was greeted at the door by the enthusiastic Brad she had met before the interview. “I listened to reason, Anita. I hope this is enough for you to finish this documentation and get coding again!”

Brad led Anita into his office. On every wall surface, over the door, even covering part of the floor, were whiteboards. Sticky notes dotted nearly a third of the new whiteboard space.

“Now, Anita, if I don’t see new code from you soon, I may just have to let you go! Now get to work!”

Anita went to sit at her desk, then stopped. Instead, she grabbed a bright red sticky note, wrote the words “I QUIT” with a sharpy, barged into Brad’s office, and stuck it to his monitor. Brad was too stunned to talk as she left the converted garage.

The Avalanche

“Are you doing better?” Jason called Anita a few weeks later. Their short time together at GES has made them comrades-in-arms, and networking was crucial in the business.

“Much,” she said. “I got a real job with an indie developer in Santa Monica. We even have a wiki for our framework!”

“Well, listen to this. The day after you quit, the AC unit in the garage broke. I came into work to see Brad crying in a corner in his office. All of the sticky notes had curled in the humidity and fallen to the floor. The day after he got us all copies of Word.

“Too bad we still don’t know what the title of the game is.”

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

Today, we're going to start with the comment before the method.

    /**
     * The topology type of primitives to render. (optional)<br>
     * Default: 4<br>
     * Valid values: [0, 1, 2, 3, 4, 5, 6]
     *
     * @param mode The mode to set
     * @throws IllegalArgumentException If the given value does not meet
     * the given constraints
     *
     */

This comes from Krzysztof. As much as I dislike these JavaDoc style comments (they mostly repeat information I can get from the signature!), this one is promising. It tells me the range of values, and what happens when I exceed that range, what the default is, and it tells me that the value is optional.

In short, from the comment alone I have a good picture of what the implementation looks like.

With some caveats, mind you- because that's a set of magic numbers in there. No constants, no enum, just magic numbers. That's worrying.

Let's look at the implementation.

    public void setMode(Integer mode) {
        if (mode == null) {
            this.mode = mode;
            return ;
        }
        if (((((((mode!= 0)&&(mode!= 1))&&(mode!= 2))&&(mode!= 3))&&(mode!= 4))&&(mode!= 5))&&(mode!= 6)) {
            throw new IllegalArgumentException((("Invalid value for mode: "+ mode)+ ", valid: [0, 1, 2, 3, 4, 5, 6]"));
        }
        this.mode = mode;
    }

This code isn't terrible. But there are all sorts of small details which flummox me.

Now, again, I want to stress, had they used enums this method would be much simpler. But fine, maybe they had a good reason for not doing that. Let's set that aside.

The obvious ugly moment here is that if condition. Did they not understand that and is a commutative operation? Or did they come to Java from LISP and miss their parentheses?

Then, of course, there's the first if statement- the null check. Honestly, we could have just put that into the chain of the if condition below, and the behavior would have been the same, or they could have just used an Optional type, which is arguably the "right" option here. But now we're drifting into the same space as enums- if only they'd used the core language features, this would be simpler.

Let's focus, instead, on one last odd choice: how they use whitespace. mode!= 0. This, more than anything, makes me think they are coming to Java from some other language. Something that uses glyphs in unusual ways, because why else would the operator only get one space on one side of it? Which also makes me think the null check was written by someone else- because they're inconsistent with it there.

So no, this code isn't terrible, but it does make me wonder a little bit about how it came to be.

Robert was diagnosing a problem in a reporting module. The application code ran a fairly simple query- SELECT field1, field2, field3 FROM report_table- so he foolishly assumed that it would be easy to understand the problem. Of course, the "table" driving the report wasn't actually a table, it was a view in the database.

Most of our readers are familiar with how views work, but for those who have had been corrupted by NoSQL databases: database views are great- take a query you run often, and create it as an object in the database:

CREATE VIEW my_report
AS
SELECT t1.someField as someField, t2.someOtherField as someOtherField
FROM table1 t1 INNER JOIN table2 t2 ON t1.id = t2.id

Now you can query SELECT * FROM my_report WHERE someField > 5.

Like I said: great! Well, usually great. Well, sometimes great. Well, like anything else, with great power comes great responsibility.

Robert dug into the definition of the view, only to find that the tables it queried were themselves views. And those were in turn, also views. All in all, there were nineteen layers of nested views. The top level query he was trying to debug had no real relation to the underlying data, because 19 layers of abstraction had been injected between the report and the actual data. Even better- many of these nested views queried the same tables, so data was being split up and rejoined together in non-obvious and complex ways.

The view that caused Robert to reach out to us was this:

ALTER VIEW [LSFDR].[v_ControlDate]
AS
SELECT
GETDATE() AS controlDate
--GETDATE() - 7 AS controlDate

This query is simply invoking a built-in function which returns today's date. Why not just call the function? We can see that once upon a time, it did offset the date by seven days, making the control date a week earlier. So I suppose there's some readability in mytable m INNER JOIN v_ControlDate cd ON m.transactionDate > cd.controlDate, but that readability also hides the meaning of control date.

That's the fundamental problem of abstraction. We lose details and meaning, and end up with 19 layers of stuff to puzzle through. A more proper solution may have been to actually implement this as a function, not a view- FROM mytable m WHERE m.transactionDate > getControlDate(). At least here, it's clear that I'm invoking a function, instead of hiding it deep inside of a view called from a view called from a view.

In any case, I'd argue that the actual code we're looking at isn't the true WTF. I don't like this view, and I wouldn't implement it this way, but it doesn't make me go "WTF?" The context the view exists in, on the other hand, absolutely does. 19 layers! Is this a database or a Russian Honey Cake?

The report, of course, didn't have any requirements defining its data. Instead, the users had worked with the software team to gradually tweak the output over time until it gave them what they believed they wanted. This meant actually changing the views to be something comprehensible and maintainable wasn't a viable option- changes could break the report in surprising and non-obvious ways. So Robert was compelled to suffer through and make the minimally invasive changes required to fix the view and get the output looking like what the users wanted.

The real WTF? The easiest fix was to create another view, and join it in. Problems compound themselves over time.

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

"Magic bytes" are a common part of a file header. The first few bytes of a file can often be used to identify what type of file it is. For example, a bitmap file starts with "BM", and a PGM file always starts with "PN" where "N" is a number between 1 and 6, describing the specific variant in use, and WAV files start with "RIFF".

Many files have less human-readable magic bytes, like the ones Christer was working with. His team was working on software to manipulate a variety of different CAD file types. One thing this code needed to do is identify when the loaded file was a CAD file, but not the specific UFF file type they were looking for. In this case, they need to check that the file does not start with 0xabb0, 0xabb1, or 0xabb3. It was trivially easy to write up a validation check to ensure that the files had the correct magic bytes. And yet, there is no task so easy that someone can't fall flat on their face while doing it.

This is how Christer's co-worker solved this problem:

const uint16_t *id = (uint16_t*)data.GetBuffer();
if (*id == 0xabb0 || *id == 0xABB0 || *id == 0xabb1 || *id == 0xABB1 || *id == 0xabb3 || *id == 0xABB3)
{
    return 0;
}

Here we have a case of someone who isn't clear on the difference between hexadecimal numbers and strings. Now, you (and the compiler) might think that 0xABB0 and 0xabb0 are, quite clearly, the same thing. But you don't understand the power of lowercase numbers. Here we have an entirely new numbering system where 0xABB0 and 0xabb0 are not equal, which also means 0xABB0 - 0xabb0 is non-zero. An entirely new field of mathematics lies before us, with new questions to be asked. If 0xABB0 < 0xABB1, is 0xABB0 < 0xabb1 also true? From this little code sample, we can't make any inferences, but these questions give us a rich field of useless mathematics to write papers about.

The biggest question of all, is that we know how to write lowercase numbers for A-F, but how do we write a lowercase 3?

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

Our second anonymous helper tells us "While looking up how to find my banks branch using a blank check, I came across this site that seems to have used AI to write their posts. Didn't expect to learn about git while reading about checks. I included the navbar because its just as bad."

Our third anonymous helper snickered "I guess I was just a bit over quota." Nicely done.

Our fourth anonymous helper isn't actually anonymous, alas. He signed off as the plausibly-named Vincent R, muttering "I dunno, it's all Greek to me. Or at least it *was* Greek until Firefox thoughtfully translated all the lambdas and mus and sigmas in these probability formulas..."

Finally for Friday, the fifth from Dan W. "On my way to the airport, I checked my route on the Trainline app. I think I'll have just enough time to make this connection in Wolverhampton." Walk, don't run.

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

Charles is supporting a PHP based application. One feature of the application is a standard "Contact Us" form. I'll let Charles take on the introduction:

While it looks fine on the outside, the code is a complete mess. The entire site is built with bad practices, redundant variables, poor validation, insecure cookie checks, and zero focus on maintainability or security. Even the core parts of the platform are a nightmare

We're going to take this one in chunks, because it's big and ugly.

try {
    if (isset($_POST)) {
        $name = $_POST['objName'];
        $lst_name = $_POST['objLstName'];
        $email = $_POST['objEmail'];
        $phone = $_POST['objGsm'];
        $message = $_POST['objMsg'];
        $verifycode = $_POST['objVerifyCode'];
        /******************************************************/
        $objCmpT = $_POST['objCmpT'];
        $objCmpS = $_POST['objCmpS'];
        $objCountry = $_POST['objCountry'];
        $objCity = $_POST['objCity'];
        $objName2 = $_POST['objName2'];
        $objLstName2 = $_POST['objLstName2'];
        $objQuality = $_POST['objQuality'];
        $objEmail = $_POST['objEmail'];
        $objMsg2 = $_POST['objMsg2'];
        $objVerifyCode2 = $_POST['objVerifyCode2'];

I don't love that there's no structure or class here, to organize these fields, but this isn't bad, per se. We have a bunch of form fields, and we jam them into a bunch of variables. I am going to, with no small degree of willpower, not comment on the hungarian notation present in the field names. Look at me not commenting on it. I'm definitely not commenting on it. Look at me not commenting that some, but not all, of the variables also get the same hungarian prefix.

What's the point of hungarian notation when everything just gets the same thing anyway; like hungarian is always bad, but this is just USELESS

Ahem.

Let's continue with the code.

        $ok = 0;
        $ok2 = 0;
        $sendTo = "example@example.com";
        $golableMSG = '
        -First Name & Last Name :' . $name . ' ' . $lst_name . '
        -email :' . $email . '
        -Phone Number : 0' . $phone . '
        -Message : ' . $message;
        $globaleMSG2 = '
        -First Name & Last Name :' . $objName2 . ' ' . $objLstName2 . '
        -Email :' . $objEmail . '
        -Type of company : ' . $objCmpT . '
        -Sector of activity : ' . $objCmpS . '
        -Country : ' . $objCountry . '
        -City : ' . $objCity . '
        -Your position within the company : ' . $objQuality . '
        -Message : ' . $objMsg2;

We munge all those form fields into strings. These are clearly going to be the bodies of our messages. Only now I'm noticing that the user had to supply two different names- $name and $objName2. Extra points here, as I believe they meant to name both of these message variables globaleMSG but misspelled the first one, golableMSG.

Well, let's continue.

        if (!$name) {
            $data['msg1'] = '*';
        } else {
            $ok++;
            $data['msg1'] = '';
        }
        if (!$lst_name) {
            $data['msg2'] = '*';
        } else {
            $ok++;
            $data['msg2'] = '';
        }
        if (!$email) {
            $data['msg3'] = '*';
        } else {
            $ok++;
            $data['msg3'] = '';
        }
        if ($phone <= 0) {
            $data['msg4'] = '*';
        } else {
            $ok++;
            $data['msg4'] = '';
        }
        if (!$message) {
            $data['msg5'] = '*';
        } else {
            $ok++;
            $data['msg5'] = '';
        }
        if (!$verifycode) {
            $data['msg6'] = '*';
        } else {
            $ok++;
            $data['msg6'] = '';
        }
        /*********************************************************************************/
        if (!$objCmpS) {
            $data['msg7'] = '*';
        } else {
            $ok2++;
            $data['msg7'] = '';
        }
        if (!$objCountry) {
            $data['msg8'] = '*';
        } else {
            $ok2++;
            $data['msg8'] = '';
        }
        if (!$objCity) {
            $data['msg9'] = '*';
        } else {
            $ok2++;
            $data['msg9'] = '';
        }
        if (!$objName2) {
            $data['msg10'] = '*';
        } else {
            $ok2++;
            $data['msg10'] = '';
        }
        if (!$objLstName2) {
            $data['msg11'] = '*';
        } else {
            $ok2++;
            $data['msg11'] = '';
        }
        if (!$objQuality) {
            $data['msg12'] = '*';
        } else {
            $ok2++;
            $data['msg12'] = '';
        }
        if (!$objMsg2) {
            $data['msg13'] = '*';
        } else {
            $ok2++;
            $data['msg13'] = '';
        }
        if (!$objVerifyCode2) {
            $data['msg14'] = '*';
        } else {
            $ok2++;
            $data['msg14'] = '';
        }

What… what are we doing here? I worry that what I'm looking at here is some sort of preamble to verification code. But why is it like this? Why?

        /********************************************************************************/
        if ($ok == 6) {
            if (preg_match("/^[ a-z,.+!:;()-]+$/", $name)) {
                $data['msg1_1'] = '';
                if (preg_match("/^[ a-z,.+!:;()-]+$/", $lst_name)) {
                    $data['msg2_2'] = '';
                    $subject = $name . " " . $lst_name;
                    if (filter_var($email, FILTER_VALIDATE_EMAIL)) {
                        $data['msg3_3'] = '';
                        $from = $email;
                        if (preg_match("/^[6-9][0-9]{8}$/", $phone)) {
                            $data['msg4_4'] = '';
                            if (intval($verifycode) == intval($_COOKIE['nmbr1']) + intval($_COOKIE['nmbr2'])) {
                                $data['msg6_6'] = '';
                                $headers = 'From: ' . $from . "\r\n" .
                                    'Reply-To: ' . $sendTo . "\r\n" .
                                    'X-Mailer: PHP/' . phpversion();
                                mail($sendTo, $subject, $golableMSG, $headers);
                                $data['msgfinal'] = 'Votre Messsage est bien envoyer';
                                /*$data = array('success' => 'Votre Messsage est bien envoyer', 'postData' => $_POST);*/
                            } else {
                                $data['msg6_6'] = 'votre resultat est incorrect';
                            }
                        } else {
                            $data['msg4_4'] = 'Votre Numéro est incorrect';
                        }
                    } else {
                        $data['msg3_3'] = 'Votre Email est incorrect';
                    }
                } else {
                    $data['msg2_2'] = 'Votre Prénom est Incorrect';
                }
            } else {
                $data['msg1_1'] = 'Votre Nom est Incorrect';
            }
        }

Oh look, it is validation code. Their verification code system, presumably to prevent spamming messages, is not particularly secure or useful. The real thing I see here, though, is the namespaced keys. Earlier, we set $data['msg1'], and now we're setting $data['msg1_1'] which is a code stench that could kill from a hundred yards.

And don't worry, we do the same thing for the other message we send:

        /**************************************************************/
        if ($ok2 == 8) {
            if (preg_match("/^[ a-z,.+!:;()-]+$/", $objName2)) {
                $data['msg10_10'] = '';
                if (preg_match("/^[ a-z,.+!:;()-]+$/", $objLstName2)) {
                    $data['msg11_11'] = '';
                    $subject2 = $objName2 . " " . $objLstName2;
                    if (intval($objVerifyCode2) == intval($_COOKIE['nmbr3']) + intval($_COOKIE['nmbr4'])) {
                        $from2 = $objEmail;
                        $data['msg14_14'] = '';
                        $headers2 = 'From: ' . $from2 . "\r\n" .
                            'Reply-To: ' . $sendTo . "\r\n" .
                            'X-Mailer: PHP/' . phpversion();
                        mail($sendTo, $subject2, $globaleMSG2, $headers2);
                        $data['msgfinal'] = 'Votre Messsage est bien envoyer';
                    } else {
                        $data['msg14_14'] = 'votre resultat est incorrect';
                    }
                } else {
                    $data['msg11_11'] = 'Votre Prénom est Incorrect';
                }
            } else {
                $data['msg10_10'] = 'Votre Nom est Incorrect';
            }
        }

Phew. Hey, remember way back at the top, when we checked to see if the $_POST variable were set? Well, we do have an else clause for that.

    } else {
        throw new \Exception($mot[86]);
    }

Who doesn't love throwing messages by hard-coded array indexes in your array of possible error messages? Couldn't be bothered with a constant, could we? Nope, message 86 it is.

But don't worry about that exception going uncaught. Remember, this whole thing was inside of a try:

} catch (\Exception $e) {
    $data['msgfinal'] = "Votre Messsage n'est pas bien envoyer";
    /*$data = array('danger' => 'Votre Messsage pas bien envoyer', 'postData' => $_POST);*/
}

Yeah, it didn't matter what message we picked, because we just catch the exception and hard-code out an error message.

Also, I don't speak French, but is "message" supposed to have an extra "s" in it?

Charles writes:

It’s crazy to see such sloppy work on a platform that seems okay at first glance. Honestly, this platform is the holy grail of messy code—it could have its own course on how not to code because of how bad and error-prone it is. There are also even worse scenarios of bad code, but it's too long to share, and honestly, they're too deep and fundamentally ingrained in the system to even begin explaining.

Oh, I'm sure we could explain it. The explanation may be "there was a severe and fatal lack of oxygen in the office, and this is what hypoxia looks like in code," but I'm certain there'd be an explanation.

Once upon a time, web browsers weren't the one-stop-shop for all kinds of possible content that they are today. Aside from the most basic media types, your browser depended on content plugins to display different media types. Yes, there was an era where, if you wanted to watch a video in a web browser, you may need to have QuickTime or… (shudder) Real Player installed.

As a web developer, you'd need to write code to check which plugins were installed. If they don't have Adobe Acrobat Reader installed, there's no point in serving them up a PDF file- you'll need instead to give them an install link.

Which brings us to Ido's submission. This code is intended to find the Acrobat Reader plugin version.

acrobatVersion: function GetAcrobatVersion() {
	// Check acrobat is Enabled or not and its version
	acrobatVersion = 0;
	if (navigator.plugins && navigator.plugins.length) {
		for (intLoop = 0; intLoop <= 15; intLoop++) {
			if (navigator.plugins[intLoop] != -1) {
				acrobatVersion = parseFloat(navigator.plugins[intLoop].version);
				isAcrobatInstalled = true;
				break;
			}
		}
	}
	else {...}
}

So, we start by checking for the navigator.plugins array. This is a wildly outdated thing to do, as the MDN is quite emphatic about, but I'm not going to to get hung up on that- this code is likely old.

But what I do want to pay attention to is that they check navigator.plugins.length. Then they loop across the set of plugins using a for loop. And don't use the length! They bound the loop at 15, arbitrarily. Why? No idea- I suspect it's for the same reason they named the variable intLoop and not i like a normal human.

Then they check to ensure that the entry at plugins[intLoop] is not equal to -1. I'm not sure what the expected behavior was here- if you're accessing an array out of bounds in JavaScript, I'd expect it to return undefined. Perhaps some antique version of Internet Explorer did something differently? Sadly plausible.

Okay, we've found something we believe to be a plugin, because it's not -1, we'll grab the version property off of it and… parseFloat. On a version number. Which ignores the fact that 1.1 and 1.10 are different versions. Version numbers, like phone numbers, are not actually numbers. We don't do arithmetic on them, treat them like text.

That done, we can say isAcrobatInstalled is true- despite the fact that we didn't check to see if this plugin was actually an Acrobat plugin. It could have been Flash. Or QuickTime.

Then we break out of the loop. A loop that, I strongly suspect, would only ever have one iteration, because undefined != -1.

So there we have it: code that doesn't do what it intends to, and even if it did, is doing it the absolute wrong way, and is also epically deprecated.

Sometimes, there's code so bad you simply know it's unused and never called. Bernard sends us one such method, in Java:

  /**
   * Finds a <code>GroupEntity</code> by group number.
   *
   * @param  group the group number.
   * @return the <code>GroupEntity</code> object.
   */
  public static GroupEntity find(String group) {
    return GroupEntity.find(group);
  }

This is a static method on the GroupEntity class called find, which calls a static method on the GroupEntity class called find, which calls a static method on the GroupEntity class called find and it goes on and on my friend.

Clearly, this is a mistake. Bernard didn't supply much more context, so perhaps the String was supposed to be turned into some other type, and there's an overload which would break the recursion. Regardless, there was an antediluvian ticket on the backlog requesting that the feature to allow finding groups via a search input that no one had yet worked on.

I'm sure they'll get around to it, once the first call finishes.

Simon recently found himself working alongside a "very senior" developer- who had a whopping 5 years of experience. This developer was also aggrieved that in recent years, Object Oriented programming had developed a bad reputation. "Functional this, functional that, people really just don't understand how clean and clear objects make your code."

For example, here are a few Java objects which they wrote to power a web scraping tool:

class UrlHolder {

    private String url;

    public UrlHolder(String url) {
        this.url = url;
    }
}

class UrlDownloader {

    private UrlHolder url;
    public String downloadPage;

    public UrlDownLoader(String url) {
        this.url = new UrlHolder(Url);
    }
}

class UrlLinkExtractor {

   private UrlDownloader url;

   public UrlLinkExtractor(UrlDownloader url) {
        this.url = url;
   }

   public String[] extract() {
       String page = Url.downloadPage;
       ...
   }
}

UrlHolder is just a wrapper around string, but also makes that string private and provides no accessors. Anything shoved into an instance of that may as well be thrown into oblivion.

UrlDownloader wraps a UrlHolder, again, as a private member with no accessors. It also has a random public string called downloadPage.

UrlLinkExtractor wraps a UrlDownloader, and at least UrlLinkExtractor has a function- which presumably downloads the page. It uses UrlDownloader#downloadPage- the public string property. It doesn't use the UrlHolder, because of course it couldn't. The entire goal of this code is to pass a string to the extract function.

I guess I don't understand object oriented programming. I thought I did, but after reading this code, I don't.

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

Horror movies, as of late, have gone to great lengths to solve the key obstacle to horror movies- cell phones. When we live in a world where help is a phone call away, it's hard to imagine the characters not doing that. So screenwriters put them in situations where this is impossible: in Midsommar they isolate them in rural Sweden, in Get Out calling the police is only going to put our protagonist in more danger. But what's possibly more common is making the film a period piece- like the X/Pearl/Maxxxine trilogy, Late Night with the Devil, or Netflix's continuing series of R.L. Stine adaptations.

I bring this up, because today's horror starts in 1993. A Norwegian software company launched its software product to mild acclaim. Like every company, it had its ups and downs, its successes and missteps. On the surface, it was a decent enough place to work.

Over the years, the company tried to stay up to date with technology. In 1993, the major languages one might use for launching a major software product, your options are largely C or Pascal. Languages like Python existed, but weren't widely used or even supported on most systems. But the company stayed in business and needed to update their technology as time passed, which meant the program gradually grew and migrated to new languages.

Which meant, by the time Niklas F joined the company, they were on C#. Even though they'd completely changed languages, the codebase still derived from the original C codebase. And that meant that the codebase had many secrets, dark corners, and places a developer should never look.

Like every good horror movie protagonist, Niklas heard the "don't go in there!" and immediately went in there. And lurking in those shadows was the thing every developer fears the most: homebrew date handling code.

/// <summary>
/// 
/// </summary>
/// <param name="dt"></param>
/// <returns></returns>
public static DateTime LastDayInMonth(DateTime dt)
{
	int day = 30;
	switch (dt.Month)
	{
		case 1:
			day = 31;
			break;
		case 2:
			if (IsLeapYear(dt))
				day = 29;
			else
				day = 28;
			break;
		case 3:
			day = 31;
			break;
		case 4:
			day = 30;
			break;
		case 5:
			day = 31;
			break;
		case 6:
			day = 30;
			break;
		case 7:
			day = 31;
			break;
		case 8:
			day = 31;
			break;
		case 9:
			day = 30;
			break;
		case 10:
			day = 31;
			break;
		case 11:
			day = 30;
			break;
		case 12:
			day = 31;
			break;
	}
	return new DateTime(dt.Year, dt.Month, day, 0, 0, 0);
}

/// <summary>
/// 
/// </summary>
/// <param name="dt"></param>
/// <returns></returns>
public static bool IsLeapYear(DateTime dt)
{
	bool ret = (((dt.Year % 4) == 0) && ((dt.Year % 100) != 0) || ((dt.Year % 400) == 0));
	return ret;
}

For a nice change of pace, this code isn't incorrect. Even the leap year calculation is actually correct (though my preference would be to just return the expression instead of using a local variable). But that's what makes this horror all the more insidious: there are built-in functions to handle all of this, but this code works and will likely continue to work, just sitting there, like a demon that we've made a pact with. And suddenly we realize this isn't Midsommar but Ari Aster's other hit film, Hereditary, and we're trapped being in a lineage of monsters, and can't escape our inheritance.

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

Once again, we take a look at the traditional "if (boolean) return true; else return false;" pattern. But today's, from RJ, offers us a bonus twist.

public override bool IsValid
{
   get
   {
      if (!base.IsValid)
         return false;

      return true;
   }
}

As promised, this is a useless conditional. return base.IsValid would do the job just as well. Except, that's the twist, isn't it. base is our superclass. We're overriding a method on our superclass to… just do what the base method does.

This entire function could just be deleted. No one would notice. And yet, it hasn't been. Everyone agrees that it should be, yet it hasn't been. No one's doing it. It just sits there, like a pimple, begging to be popped.

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

Jon recently started a new project. When setting up his dev environment, one of his peers told him, "You can disable verbose logging by setting DEBUG_LOG=false in your config file."

Well, when Jon did that, the verbose logging remained on. When he asked his peers, they were all surprised to see that the flag wasn't turning off debug logging. "Hunh, that used to work. Someone must have changed something…" Everyone had enough new development to do that tracking down a low priority bug fell to Jon. It didn't take long.

const DEBUG_LOG = process.env.DEBUG_LOG || true

According to the blame, the code had been like this for a year, the commit crammed with half a dozen features, was made by a developer who was no longer with the company, and the message was simply "Debugging". Presumably, this was intended to be a temporary change that accidentally got committed and no one noticed or cared.

Jon fixed it, and moved on. There was likely going to be plenty more to find.

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

Quality control is an important business function for any company. When your company is shipping devices with safety concerns, it's even more important. In some industries, a quality control failure is bound to be national headlines.

When the quality control software tool stopped working, everyone panicked. At which point, GRH stepped in.

Now, we've discussed this software and GRH before, but as a quick recap, it was:

written by someone who is no longer employed with the company, as part of a project managed by someone who is no longer at the company, requested by an executive who is also no longer at the company. There are no documented requirements, very few tests, and a lot of "don't touch this, it works".

And this was a quality control tool. So we're already in bad shape. It also had been unmaintained for years- a few of the QC engineers had tried to take it over, but weren't programmers, and it had essentially languished.

Specifically, it was a quality control tool used to oversee the process by about 50 QC engineers. It automates a series of checks by wrapping around third party software tools, in a complex network of "this device gets tested by generating output in program A, feeding it to program B, then combining the streams and sending them to the device, but this device gets tested using programs D, E, and F."

The automated process using the tool has a shockingly low error rate. Without the tool, doing things manually, the error rate climbs to 1-2%. So unless everyone wanted to see terrifying headlines in the Boston Globe about their devices failing, GRH needed to fix the problem.

GRH was given the code, in this case a a zip file on a shared drive. It did not, at the start, even build. After fighting with the project configuration to resolve that, GRH was free to start digging in deeper.

Public Sub connect2PCdb()
        Dim cPath As String = Path.Combine(strConverterPath, "c.pfx")
        Dim strCN As String

        ' JES 12/6/2016: Modify the following line if MySQL server is changed to a different server.  A dump file will be needed to re-create teh database in the new server.
        strCN = "metadata=res://*/Model1.csdl|res://*/Model1.ssdl|res://*/Model1.msl;provider=MySql.Data.MySqlClient;provider connection string='server=REDACTED;user id=REDACTED;database=REDACTED;sslmode=Required;certificatepassword=REDACTED;certificatefile=REDACTED\c.pfx;password=REDACTED'"
        strCN = Regex.Replace(strCN, "certificatefile=.*?pfx", "certificatefile=" & cPath)
        pcContext = New Entities(strCN)
        strCN = "metadata=res://*/Model1.csdl|res://*/Model1.ssdl|res://*/Model1.msl;provider=MySql.Data.MySqlClient;provider connection string='server=REDACTED;user id=REDACTED;persistsecurityinfo=True;database=REDACTED;password=REDACTED'"
        strCN = Regex.Match(strCN, ".*'(.*)'").Groups(1).Value

        Try
            strCN = pcContext.Database.Connection.ConnectionString
            cnPC.ConnectionString = "server=REDACTED;user id=REDACTED;password=REDACTED;database=REDACTED;"
            cnPC.Open()
        Catch ex As Exception

        End Try
    End Sub

This is the code which connects to the backend database. The code is in the category of more of a trainwreck than a WTF. It's got a wonderful mix of nonsense in here, though- a hard-coded connection string which includes plaintext passwords, regex munging to modify the string, then hard-coding a string again, only to use regexes to extract a subset of the string. A subset we don't use.

And then, for a bonus, the whole thing has a misleading comment- "modify the following line" if we move to a different server? We have to modify several lines, because we keep copy/pasting the string around.

Oh, and of course, it uses the pattern of "open a database connection at application startup, and just hold that connection forever," which is a great way to strain your database as your userbase grows.

The good news about the hard-coded password is that it got GRH access to the database. With that, it was easy to see what the problem was: the database was full. The system was overly aggressive with logging, the logs went to database tables, the server was an antique with a rather small hard drive, and the database wasn't configured to even use all of that space anyway.

Cleaning up old logs got the engineers working again. GRH kept working on the code, though, cleaning it up and modernizing it. Updating to latest version of the .NET Core framework modified the data access to be far simpler, and got rid of the need for hard-coded connection strings. Still, GRH left the method looking like this:

    Public Sub connect2PCdb()
        'Dim cPath As String = Path.Combine(strConverterPath, "c.pfx")
        'Dim strCN As String

        ' JES 12/6/2016: Modify the following line if MySQL server is changed to a different server.  A dump file will be needed to re-create teh database in the new server.
        'strCN = "metadata=res://*/Model1.csdl|res://*/Model1.ssdl|res://*/Model1.msl;provider=MySql.Data.MySqlClient;provider connection string='server=REDACTED;user id=REDACTED;database=REDACTED;sslmode=Required;certificatepassword=REDACTED;certificatefile=REDACTED\c.pfx;password=REDACTED'"
        'strCN = Regex.Replace(strCN, "certificatefile=.*?pfx", "certificatefile=" & cPath)
        'pcContext = New Entities(strCN)
        'strCN = "metadata=res://*/Model1.csdl|res://*/Model1.ssdl|res://*/Model1.msl;provider=MySql.Data.MySqlClient;provider connection string='server=REDACTED;user id=REDACTED;persistsecurityinfo=True;database=REDACTED;password=REDACTED'"
        'strCN = Regex.Match(strCN, ".*'(.*)'").Groups(1).Value

        'GRH 2021-01-15.  Connection information moved to App.Config
        'GRH 2021-08-13.  EF Core no longer supports App.Config method
        pcContext = New PcEntities

        Try
            ' GRH 2021-08-21  This variable no longer exists in .NET 5
            'strCN = pcContext.Database.Connection.ConnectionString
            ' GRH 2021-08-20  Keeping the connection open causes EF Core to not work
            'cnPC.ConnectionString = "server=REDACTED;user id=REDACTED;password=REDACTED;database=REDACTED;SslMode=none"
            'cnPC.Open()
        Catch ex As Exception

        End Try
    End Sub

It's now a one-line method, with most of the code commented out, instead of removed. Why on Earth is the method left like that?

GRH explains:

Yes, I could delete the function as it is functionally dead, but I keep it for the same reasons that a hunter mounts a deer's head above her mantle.

No obvious pattern fell out of last week's submissions for Error'd, but I did especially like Caleb Su's example.

Michael R. , apparently still job hunting, reports "I have signed up to outlier.ai to make some $$$ on the side. No instructions necessary."

Peter G. repeats a recurring theme of lost packages, saying "(Insert obligatory snark about Americans and geography. No, New Zealand isn't located in Washington DC)." A very odd coincidence, since neither the lat/long nor the zip code are particularly interesting.

"The Past Is Mutable," declares Caleb Su , explaining "In the race to compete with Gmail feature scheduling emails to send in the *future*, Outlook now lets you send emails in the past! Clearly, someone at Microsoft deserves a Nobel Prize for defying the basic laws of unidirectional time." That's thinking different.

Explorer xOneca explains this snapshot: "Was going to watch a Youtube video in DuckDuckGo, and while diagnosing why it wasn't playing I found this. It seems that youtube-nocookie.com actually *sets* cookies..?"

Morgan either found or made a funny. But it is a funny. "Now when I think about it I do like Option 3 more…" I rate this question a 👎

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

As a general rule, if you're using an RDBMS and can solve your problem using SQL, you should solve your problem using SQL. It's how we avoid doing joins or sorts in our application code, which is always a good thing.

But this is a general rule. And Jasmine sends us one where solving the problem as a query was a bad idea.

ALTER   FUNCTION [dbo].[GetName](@EntityID int)

RETURNS varchar(200)

AS

BEGIN

declare @Name varchar(200)

select @Name =
  case E.EntityType
    when 'Application'  then A.ApplicationName
    when 'Automation'   then 'Automated Process'
    when 'Group'        then G.GroupName
    when 'Organization' then O.OrgName
    when 'Person'       then P.FirstName + ' ' + P.LastName
    when 'Resource'     then R.ResourceName
    when 'Batch'        then B.BatchComment
  end
from Entities E
left join AP_Applications A   on E.EntityID = A.EntityID
left join CN_Groups G         on E.EntityID = G.EntityID
left join CN_Organizations O  on E.EntityID = O.EntityID
left join CN_People P         on E.EntityID = P.EntityID
left join Resources R         on E.EntityID = R.EntityID
left join AR_PaymentBatches B on E.EntityID = B.EntityID
where E.EntityID = @EntityID

return @Name

END

The purpose of this function is to look up the name of an entity. Depending on the kind of entity we're talking about, we have to pull that name from a different table. This is a very common pattern in database normalization- a database equivalent of inheritance. All the common fields to all entities get stored in an Entities table, while specific classes of entity (like "Applications") get their own table which joins back to the Entities table.

On the surface, this code doesn't even really look like a WTF. By the book, this is really how you'd write this kind of function- if we were going by the book.

But the problem was that these tables were frequently very large, and even with indexes on the EntityID fields, it simply performed horribly. And since "showing the name of the thing you're looking at" was a common query, that performance hit added up.

The fix was easy- write out seven unique functions- one for each entity type- and then re-write this function to use an IF statement to decide which one to execute. The code was simpler to understand and read, and performed much faster.

In the end, perhaps not really a WTF, or perhaps the root WTF is some of the architectural decisions which allow this to exist (why a function for getting the name, and the name alone, which means we execute this query independently and not part of a more meaningful join?). But I think it's an interesting example of how "this is the right way to do it" can lead to some unusual outcomes.

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

When processing HTTP requests, you frequently need to check the parameters which were sent along with that request. Those parameters are generally passed as stringly-typed key/value pairs. None of this is news to anyone.

What is news, however, is how Brodey's co-worker indexed the key/value pairs.

For i As Integer = 0 To (Request.Params().Count - 1)
    If (parameters.GetKey(i).ToString() <> "Lang") Then
        If (parameters.GetKey(i).Equals("ID")) OrElse (parameters.GetKey(i).Equals("new")) OrElse _
             (parameters.GetKey(i).Equals("open")) OrElse (parameters.GetKey(i).Equals("FID")) _
         OrElse (parameters.GetKey(i).Equals("enabled")) OrElse (parameters.GetKey(i).Equals("my")) OrElse _
         (parameters.GetKey(i).Equals("msgType")) OrElse (parameters.GetKey(i).Equals("Type")) _
         OrElse (parameters.GetKey(i).Equals("EID")) OrElse (parameters.GetKey(i).Equals("Title")) OrElse _
         (parameters.GetKey(i).Equals("ERROR")) Then
            URLParams &= "&" & parameters.GetKey(i).ToString()
            URLParams &= "=" & parameters(i).ToString()
        End If
    End If
Next

The goal of this code is to take a certain set of keys and construct a URLParams string which represents those key/values as an HTTP query string. The first thing to get out of the way: .NET has a QueryString type that handles the construction of the query string for you (including escaping), so that you don't need to do any string concatenation.

But the real WTF is everything surrounding that. We opt to iterate across every key- not just the ones we care about- and use the GetKey(i) function to check each individual key in an extensive chain of OrElse statements.

The obvious and simpler approach would have been to iterate across an array of the keys I care about- ID, new, FID, enabled, my, msgType, Type, EID, Title, ERROR- and simply check if they were in the Request.

I suppose the only silver lining here is that they thought to use the OrElse operator- which is a short-circuiting "or" operation, like you'd expect in just about any other language, instead of Or, which doesn't short circuit (pulling double duty as both a bitwise Or and a logical Or, because Visual Basic wants to contribute some WTFs).

Our stories come from you, our readers- which, it's worth reminding everyone, keep those submissions coming in. There's nothing on this site without your submissions.

Now, we do get some submissions which don't make the page. Frequently, it's simply because we simply don't have enough context from the submission to understand it or comment on it effectively. Often, it's just not that remarkable. And sometimes, it's because the code isn't a WTF at all.

So I want to discuss some of these, because I think it's still interesting. And it's unfair to expect everyone to know everything, so for the submitters who discover they didn't understand why this code isn't bad, you're one of today's lucky 10,000.

We start with this snippet, from Guss:

#define FEATURE_SENSE_CHAN      (1 << 0)
#define FEATURE_SENSE_PEER      (1 << 1)

Guss writes:

The Asterisk open source telephony engine has some features that need to know from which direction they've been invoked in a two-way call. This is called "sense" in the Asterisk lingo, and there are two macros defined in the source which allow you to textually know if you're talking about this direction or the other. This of course stands for 1 and 0 respectively, but they couldn't have just simply go on and say that - it has to be "interesting". Do also note, as this is a macro, it means that whenever someone sets or tests the "sense", another redundant bit shift operation is done.

First, minor detail- this stands for 1 and 2 respectively. And what's important here is that these fields are clearly meant to be a bitmask. And when we're talking about a bitmask, using bitshift operators makes the code more clear. And we can generally rely on a shift by zero bits to be a no-op, and any compiler should be smart enough to spot that and optimize the operation out. Hell, a quick check with GCC shows that even the (1 << 1) gets optimized to just the constant 0x2.

Not a WTF, but it does highlight something we've commented on in the past- bitmasks can be confusing for people. This is a good example of that. But not only is this not a WTF, but it's not even bad code.

(Now, it may be the case that these are never really used as a bitmask, in which case, that's a mild WTF, but that's not what Guss was drawing our attention to)

In other cases, the code is bad, but it may be reacting to the badness it's surrounded by. Greg inherited this blob from some offshore contractors:

RegistryKey RK = Registry.LocalMachine.OpenSubKey("SOFTWARE\\XXXXX\\YYYYY");
string BoolLog = "";
if (RK != null)
	BoolLog = ((string)RK.GetValue("LogSocket", "")).ToLower();
if (BoolLog == "true" || BoolLog == "yes" || BoolLog == "1")
{
	...
}

Now, seeing a string variable called BoolLog is a big red flag about bad code inbound. And we see handling some stringly typed boolean data to try and get a truth value. Which all whiffs of bad code.

But let's talk about the Windows Registry. It's typed, but the types are strings, lists of strings, and various numeric types. There's no strictly boolean type. And sure, while explicitly storing a 1 in a numeric field is probably a better choice for the registry than string booleans, there are reasons why you might do that (especially if you frequently need to modify Registry keys by hand, like when you're debugging).

The real WTF, in this case, isn't this code, but is instead the Windows Registry. Having a single tree store be the repository for all your system configuration sounds like a good idea on paper, but as anyone who's worked with it has discovered- it's a nightmare. The code here isn't terrible. It's not good, but it's a natural reaction to the terrible world in which it lives.

Sometimes, the code is actually downright awful, but it's just hard to care about too much. Rudolf was shopping for bulk LEDs, which inevitably leads one to all sorts of websites based in China offering incredibly cheap prices and questionable quality control.

The site Rudolf was looking at had all sorts of rendering glitches, and so out of curiosity, he viewed the source.

{\rtf1\ansi\ansicpg1252\deff0\deflang2055{\fonttbl{\f0\froman\fcharset0 Times New Roman;}{\f1\fswiss\fcharset0 Arial;}}
{\*\generator Msftedit 5.41.21.2509;}\viewkind4\uc1\pard\f0\fs24 <html>\par
\par
<head> <meta http-equiv="refresh" content="1; url=http://totally-fine-leds-really-its-fine.ch"> \par

Here we see someone wrote their HTML in WordPad, and saved the file as an RTF, instead of a plain text file. Which sure, is bad. But again, we need to put this in context: this almost certainly isn't the page for handling any transactions or sales (that almost certainly uses a prebaked ecommerce plugin). This is their approach to letting "regular" users upload content to the site- frequently documentation pages. This isn't a case where some developer should have known better messed up- this is almost certainly some sales person who has an HTML template to fill in and upload. It probably stretches their technical skills to the limit to "Save As…" in WordPad.

So the code isn't bad. Again, the environment in which it sits is bad. But this is a case where the environment doesn't matter- these kinds of sites are really hoping to score some B2B sales in bulk quantities, and "customer service" and "useful website" isn't going to drive sales better than "bargain basement prices" will. They're not trying to sell to consumers, they're trying to sell to a company which will put these into consumer products. Honestly, we should be grateful that they at least tried to make an HTML file, and didn't just upload PDFs, which is usually what you find on these sites.

Sometimes, we don't have a WTF. Sometimes, we have a broken world that we can just do our best to navigate. We must simply do our best.

When SC got hired, the manager said "unit testing is very important to us, and we have 100% test coverage."

Well, that didn't sound terrible, and SC was excited to see what kind of practices they used to keep them at that high coverage.

[Test]
public void a_definition() {   

Assert.True(new TypeExpectations<IndexViewModel>()
                            .DerivesFrom<object>()
                            .IsConcreteClass()
                            .IsSealed()
                            .HasDefaultConstructor()
                            .IsNotDecorated()
                            .Implements<IEntity>()
                            .Result);
}

This is an example of what all of their tests look like. There are almost no tests of functionality, and instead just long piles of these kinds of type assertions. Which, having type assertions isn't a bad idea, most of these would be caught by the compiler:

• DerviesFrom<object> is a tautology (perhaps this test framework is ensuring it doesn't derive from other classes? but object is the parent of all classes)
• IsConcreteClass would be caught at compile time anywhere someone created an instance
• HasDefaultConstructor would again, be caught if it were used
• Implement<IEntity> would also be caught anywhere you actually tried to use polymorphism.

IsSealed and IsNotDecorated will actually do something, I suppose, though I wonder how much I actually care about that something. It's not wrong to check, but in the absence of actual real unit tests, why do I care?

Because every class had a test like this, and because of the way the test framework worked, when they ran code coverage metrics, they got a 100% score. It wasn't testing any of the code, mind you, but hey, the tests touched all of it.

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

The most common type of submission Error'd receives are simple, stupid, data problems on Amazon. The text doesn't match the image, the pricing is goofy, or some other mixup that are just bound to happen with a database of zillions of products uploaded by a plethora of barely-literate mountain village drop-shippers.

So I don't usually feature them, preferring to find something with at least a chance of being a creative new bug.

But I uncovered a story by Mark Johansen about his favorite author, and decided that since so many of you obviously DO think online retail flubs are noteworthy, what the heck. Here is Mark's plain-text story, and a handful of bungled products. They're not exactly bugs, but at least some of them are about bugs.

"I guess I missed your item about failings of AI, but here's one of my favorites: Amazon regularly sends me emails of books that their AI thinks I might want to read, presumably based on books that I've bought from them in the past. So recently I got an email saying, "The newest book by an author you've read before!" And this new book was by ... Ernest Hemingway. Considering that he died almost 60 years ago, it seemed unlikely that he was still writing. Or where he was sending manuscripts from. Lest you wonder, it turned out it was a collection of letters he wrote when he was, like, actually alive. The book was listed as authored by Ernest Hemingway rather than under the name of whomever compiled the letters."

What do we all think? Truly an Error'd, or just some publisher taking marketing advice from real estate agents? Let me know.

A while back, Christian E. "Wanted to order some groceries from nemlig.com. So I saw the free (labelled GRATIS) product and pressed the info button and this popped up. Says that I can get the product delivered from the 1st of January (today is the 2nd of march). Have to wait for a while then..." Not too much longer, Christian.

Reliable Michael R. muttered "msofas either have their special math where 5% always is GBP10 or they know already what I want to buy."

"Do not feed to vegetarians." warns Jeffrey B.

"Not sure how this blue liquid works for others, but there has been no sucking here yet," reports Matthias.

"Nice feature but I am not sure if it can fit in my notebook," writes Tiger Fok.

Lady-killer Bart-Jan is preparing for Friday night on the town, apparently. Knock 'em dead, Bart! "It says 'Fragrance for Men'. Which is fine, as long as it also does a good job deterring the female mosquitoes."

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

Damiano's company had more work than staff, and opted to hire a subcontractor. When hiring on a subcontractor, you could look for all sorts of things. Does their portfolio contain work similar to what you're asking them to do? What's the average experience of their team? What are the agreed upon code quality standards for the contract?

You could do that, or you could hire the cheapest company.

Guess which one Damiano's company did? If you're not sure, look at this code:

if(jQuery('table').hasClass('views-view-grid')){
  var EPid= ".views-view-grid";
  jQuery(EPid +' td').each(function(){

   if(!jQuery(this).parent().parent().parent().parent().parent().hasClass('view-article-in-right-sidebar') && !jQuery(this).parent().parent().parent().parent().parent().hasClass('view-offers-in-right-sidebar')){
    var title = jQuery(this).find("h2 a").html();

    var body = jQuery(this).find(".field-name-body").html();
    var datetime = jQuery(this).find(".field-name-field-event-date-time").html();
    var flyer = jQuery(this).find(".field-name-field-flyer a").attr("href");
    var imageThumb = jQuery(this).find(".field-name-field-image-thumb").html();
    var readMore = '<a href="'+jQuery(this).find("h2 a").attr("href")+'" class="read-more">READ MORE</a>';

    var str = '<div class="thumb-listing listpage">';

    if(title != null && title != ""){
      if(imageThumb && imageThumb != "" && imageThumb != null)
        str = str + imageThumb;
      if(datetime && datetime != "" && datetime != null)
        str = str + '<div class="lp-date ">'+datetime+'</div>';
      str = str + '<div class="lp-inner clear"><div class="lp-title">'+title+'</div>';
      str = str + body + '</div><div class="sep2"></div>';
      str = str + readMore;
    }
    if(flyer)
      str = str + '<a class="download-flyer" href="'+flyer+'"><?php if(isset($node) && $node->type == "events"){ echo 'download the flyer'; }else {echo 'download the article';} ?></a>';

    str = str + '</div>';
    jQuery(this).children('.node').remove();

    jQuery(this).append(str);
  }
});

This was in a Drupal project. The developer appointed by the contractor didn't know Drupal at all, and opted to build all the new functionality by dropping big blobs of JavaScript code on top of it.

There's so much to hate about this. We can start with the parent().parent() chains. Who doesn't love to make sure that your JavaScript code is extremely fragile against changes in the DOM, while at the same time making it hard to read or understand.

I like that we create the EPid variable to avoid having a magic string inside our DOM query, only to still need to append a magic string to it. It hints at some programming by copy/paste.

Then there's the pile of HTML-by-string-concatenation, which is always fun.

But this couldn't be complete without this moment: <?php if(isset($node) && $node->type == "events"){ echo 'download the flyer'; }else {echo 'download the article';} ?>

Oh yeah, buried in this unreadable blob of JavaScript there's a little bonus PHP, just to make it a little spicier.

The entire project came back from the contractor in an unusable state. The amount of re-work just to get it vaguely functional quickly outweighed any potential cost savings. And even after that work went it, it remained a buggy, unmaintainable mess.

Did management learn their lesson? Absolutely not- they bragged about how cheaply they got the work done at every opportunity, and entered into a partnership agreement with the subcontractor.

Dennis found this little nugget in an application he inherited.

function myTime(){
    $utc_str = gmdate("M d Y H:i:s", time());
    $utc = strtotime($utc_str);
    return $utc;
}

time() returns the current time as a Unix timestamp. gmdate then formats that, with the assumption that the time is in GMT. strtotime then parses that string back into a timestamp, and returns that timestamp.

Notably, PHP pins the Unix timestamp to UTC+00:00, aka GMT. So this function takes a time, formats it, parses the format to get what should be the same time back.

And we call the function myTime because of course we do. When reinventing a wheel, but square, please do let everyone know that it's yours.

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

"Oh, I see what you mean, I'll just write an overloaded function which takes the different set of parameters," said the senior dev.

That got SB's attention. You see, they were writing JavaScript, which doesn't have function overloading. "Um," SB said, "you're going to do what?"

"Function overloading," the senior dev said. "It's when you write multiple versions of the same method with different signatures-"

"I know what it is," SB said. "I'm just wondering how you're going to do that in JavaScript."

"Ah," the senior dev said with all the senior dev wisdom in the world. "It's a popular misconception that function overloading isn't allowed in JavaScript. See this?"

function addMarker(lat,lng,title,desc,pic,link,linktext,cat,icontype) {
         addMarker(lat,lng,title,desc,pic,link,linktext,cat,icontype,false);
}
               
function addMarker(lat,lng,title,desc,pic,link,linktext,cat,icontype,external) {       
    /* preparation code */
    if (external){             
        /* glue code */
    } else {
        /* other glue code */
    }
}

This, in fact, did not overload the function. This first created a version of addMarker which called itself with the wrong number of parameters. It then replaced that definition with a new one that actually did the work. That it worked at all was a delightful coincidence- when you call a JavaScript function with too few parameters, it just defaults the remainders to null, and null is falsy.

Jason was investigating a bug in a bitmask. It should have been set to 0b11, but someone had set it to just plain decimal 11. The line responsible looked like this:

byte number = (byte) 11;

This code takes the decimal number 11, casts it to a byte, and stores it in a byte, leaving us with the decimal number 11.

Curious, Jason checked the blame and saw that one of their senior-most devs was responsible. Figuring this was a good opportunity to poke a little fun at the dev for a silly mistake like this, Jason sent them a message about the difficulties of telling apart decimal values and binary values when the decimal value only contained ones and zeroes.

"What are you talking about?" the dev replied back. "The (byte) operator tells the compiler that the number is in binary."

Concerned by that reply, Jason started checking the rest of the code. And sure enough, many places in the code, the senior dev had followed this convention. Many of them were wrong, and just hadn't turned into a bug yet. One of two were coincidentally setting the important bits anyway.

Now, in a vague "defense" of what the senior dev was trying to do, C doesn't have a standard way of specifying binary literals. GCC and Clang both have a non-standard extension which lets you do 0b11, but that's not standard. So I understand the instinct- "there should be an easy way to do this," even if anyone with more than a week's experience *should have known better*.

But the real moral of the story is: don't use bitmasks without also using constants. It never should have been written with literals, it should have been written as byte number = FLAG_A | FLAG_B. The #define for the flags could be integer constants, or if you're feeling spicy about it, bitshift operations: #define FLAG_A = (1 << 1). Then you don't need binary literals, and also your code is actually readable for humans.

It was difficult to track down all the places where this misguided convention for binary literals was followed, as it was hard to tell the difference between that and a legitimate cast to byte. Fortunately, there weren't that many places where bitmasks were getting set.

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

...to know which way the wind blows. This week, it's been an ill one. Two of our readers sent us references to the BBC's reports on unusual weather in Bristol - one from the web, and one mobile. Maybe that will help you deduce the source of this error.

Frist, Graham F. shared a screenshot of the beeb's mobile app, bellowing "I know Milton is hitting the US hard right now but that's nothing compared to the 14,000 mph winds here!"

Snecod, Jeremy P. confirms the story and provides some details from the web page. "BBC weather is clipping windspeed making it look like it's only 5909mph and not 15909mph... At least they realise something is wrong."

Some anonymous American shared a snap of their weather station, which was worth a little chuckle. "Whether you like it or not, it's the weather, sort of. And, no, this wasn't during the recent eclipse." It would have been worse if the crescent had been a "sunny and clear" icon, though, given the time of day that the snap was taken. All in all, I have to call this "not an error'd".

We had to dig into the surplus bin to pad out the theme with this pair from Stuart H. He opens with "I can only assume that the forecast is for Hell or a point between the surface and the center of the Sun! I think I need to turn the aircon up a few notches."

"And following on from the forecast on the front-page - it's even worse for the rest of the world!"

Finally Eric K. reported a temperature extreme "Hellfire or extinguished sun? My weather app seems unsure of which type of apocalyptic weather conditions we're currently experiencing." But I also note this represents an unusually high level of humidity. I haven't checked but maybe one of our readers will look up these coordinates and let us know which burg has been obliterated.

When discussing ternaries, we also have to discuss readability. While short and concise, they're in some ways too compact. But don't worry, Mark's co-worker has a wonderful simplification to ternaries. This representative line is a pattern used throughout the codebase.

pnlErrorMessage.Visible = !string.IsNullOrEmpty(errorMsg) ? true : false;

This is genius, as the ternary becomes documentation for a boolean expression, telling us when we're setting things to true or false without having to think about what the expression we're evaluating means. If there is an error message, we set the error message UI element's visibility to true. Explicit, verbose, and readable.

What we're really looking at here is the ol':

if (expression)
    return true;
else
    return false;

pattern, compressed into a single ternary. Annoying, useless, and a hint that our developer doesn't understand booleans.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

"Grocery stores are going too far with their energy foods" charged Tim DG. "I was just looking for some salads to go with my BBQ," he complained. "I'm not sure they sell what I'm looking for." I've seen what your kin put in their Huzarensaladen, Tim, so I'm not entirely surprised about the Duracells.

Long-suffering Gordon S. found a novel Error'd, at least, I don't remember having seen this before. "Left Spotify running and came back 15 minutes in on a 3 minute song. Is this how extended play records worked?" I'm glad he only submitted it once and not a hundred more times for art's sake.

Christopher P. thinks FedEx is on the verge of building robots with Genuine People Personalities. "It appears to be impossible to contact a human at FedEx, and their bot seems very passive aggressive when I gave it a negative rating it tries to cancel my case. Fantastic. " I'm sure it's not truly impossible, only very very improbable.

Experienced Drinker Peter G. thinks this is not really an Error but it's a little bit of a WTF. "This is the gatekeeper popup that blocks your way when you visit the Quantum Spirits web site, which for some reason has decided to limits its customer base to a very narrow demographic. No, I'm not 21, and haven't been for quite some time." People should say what they mean and not place the burden of decoding their imprecision on everyone else.

Michael Th. is making me hungry. "Had a lovely dinner in a nice restaurant in Mannheim, Germany - and the service was really SUperb!" Once again, not really an Error'd but a sign that somebody is using bad practices with their POS system.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!

We all know that building SQL queries via string concatenation, and then sending them to the database, is just begging for fragile code and SQL injection attacks. But, what if the bad part is the "sending them to the database" part? Has anyone ever thought about that?

Kris's predecessor has.

CREATE PROCEDURE [dbo].[usp_LossMit_GetCDCMappingInfo]
        @PropertyNameString NVARCHAR(4000),
        @Environment CHAR(1)
AS
BEGIN
DECLARE @TICK CHAR (1)  SET @TICK = CHAR(39)
DECLARE @SQLSelect              NVARCHAR (4000)
DECLARE @SQLWHERE               NVARCHAR (4000)
DECLARE @SQLSelectII    NVARCHAR (4000)
DECLARE @SQLWHEREII             NVARCHAR (4000)

SET @SQLSelect = '
        SELECT
                CDCID As PropertyValue,
                CDCName AS EntityName,
                ISNULL(RTRIM(PropertyName), '+ @TICK + @TICK + ') AS PropertyName
        FROM dbo.LossMitCDCIDMapping'
SET @SQLWHERE = '
        WHERE   PropertyName IN (' + @PropertyNameString + ')
                        AND Environment = ' + @TICK + @Environment + @TICK +
                        'AND IsActive = 1'

SET @SQLSelectII = '
UNION
        SELECT
                lccm.CDCControlID AS PropertyValue,
                lccm.CDCControlName AS EntityName,
                ISNULL(RTRIM(lccm.PropertyName), '+ @TICK + @TICK + ') AS PropertyName
        FROM dbo.LossMitCDCIDMapping lcm
        INNER JOIN dbo.LossMitCDCControlIDMapping lccm
                ON lcm.CDCID = lccm.CDCID'
SET @SQLWHEREII = '
                AND     lcm.PropertyName IN ( '+ @PropertyNameString + ')
                AND lcm.Environment = ' + @TICK + @Environment + @TICK + '
                AND lccm.Environment = ' + @TICK + @Environment + @TICK + '
                AND lcm.IsActive = 1
                AND lccm.IsActive = 1'


PRINT (@SQLSelect + @SQLWHERE + @SQLSelectII + @SQLWHEREII)
EXEC (@SQLSelect + @SQLWHERE + @SQLSelectII + @SQLWHEREII)
END

/*****usp_LossMit_GetAutoIndex******/

GO

Now, just one little, itsy-bitsy thing about T-SQL: it handles variables in SQL statements just fine. They could have written AND Environment = @Environment without wrapping it up in string concatenation. This entire function could have been written without a single string concatenation in it, and the code would be simpler and easier to read, and not be begging for SQL injection attacks.

And I have no idea what's going on with @TICK- it's a one character string that they set equal to an empty 39 character string, so I assume it's just ""- why are we spamming it everywhere?

And not to be the person that harps on capitalization, but why @SQLSelect and @SQLWHERE? It's next-level inconsistency.

My only hypothesis is that this code was originally in ASP or something similar, and someone said, "Performance is bad, we should turn it into a stored procedure," and so someone did- without changing one iota about how the code was structured or worked.

Kris has this to say:

Just started at a new job--it's going to be interesting…

Interesting is certainly one word for it.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!

Many years ago, Sam was obeying Remy's Law of Requirements Gathering ("No matter what your requirements actually say, what your users want is Excel") and was working on a web-based spreadsheet and form application.

The code is not good, and involves a great deal of reinvented wheels. It is, for example, Java based, but instead of using any of the standard Java web containers for hosting their code, they wrote their own. It's like Java Servlets, but also is utterly unlike them in important and surprising ways. It supports JSP for views, but also has just enough surprises that it breaks new developers.

But let's just look at how it handles form data:

 // form field information
    String[] MM_fields = null, MM_columns = null;

    // ...snip...

    String MM_fieldsStr = "phone|value|organization|value|last_name|value|first_name|value|password|value|email_opt_in|value";
    String MM_columnsStr = "phone|',none,''|organization|',none,''|last_name|',none,''|first_name|',none,''|password|',none,''|email_opt_in|none,1,0";

    // create the MM_fields and MM_columns arrays
    java.util.StringTokenizer tokens =
            new java.util.StringTokenizer( MM_fieldsStr, "|" );
    MM_fields = new String[ tokens.countTokens() ];
    for (int i=0; tokens.hasMoreTokens(); i++)
        MM_fields[i] = tokens.nextToken();

    tokens = new java.util.StringTokenizer( MM_columnsStr, "|" );
    MM_columns = new String[ tokens.countTokens() ];
    for (int i=0; tokens.hasMoreTokens(); i++)
        MM_columns[i] = tokens.nextToken();

Who doesn't love hard-coded lists of strings with characters separating them, which then need to be parsed so that you can convert that into an array?

The MM_fieldsStr seems to imply the input data will be "key|value" pairs, and the MM_columnsStr seems to imply a specific default value, I think- but look at those quotes and commas. This is generating strings which will be injected into JavaScript. And who knows what's happening on that side- I certainly don't want to.

Also, what even is the MM_ prefix on our variables? It looks like Hungarian notation, but conveys no information- maybe it's Rēkohu notation?

As you can imagine, this whole solution was incredibly fragile and didn't work well.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

One of the first things anyone learns about object oriented programming is the power of inheritance and overriding functions. Isn't it great that you can extend or modify the implementation of a function in your derived classes? Don't you wish you could just do that for every function? Aash's co-worker certainly does.

@Override
public boolean isEnabled() {
    if (!super.isEnabled()) {
        return false;
    }
    return true;
}

I think this is a beautiful little smear of bad code, because it's useless on multiple levels. First, we are calling a boolean function only to bury it in a conditional which does the exact same thing: return super.isEnabled() would do the job. But if our developer thought to do that, they'd instantly see that there's no point to adding an override- we're just doing what the super class does. The if is just enough to hide that from you if you're careless and not very bright.

And, before you ask, no, there never was any real functionality in this override, at least not that ever got checked into source control. It isn't a vestigial leftover of once useful code. It's just useless from birth.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

One of the clients for Rudolf's company was getting furious with them. The dev team was in constant firefighting mode. No new features ever shipped, because the code-base was too fragile to add new features to without breaking something else. What few tests existed were broken. Anyone put on the project burned out and fled in months, sometimes weeks, and rarely after only a few days.

Rudolf wasn't too pleased when management parachuted him into the project to save it. But when he pulled the code and started poking around, it looked bad but not unsalvageable. The first thing he noticed is that, when following the instructions in the README, he couldn't build and run the application. Or maybe he wasn't following the instructions in the README, because the README was a confusing and incoherent mess, which included snippets from unresolved merges. Rudolf's first few days on the project were spent just getting it building and running locally, then updating the README. Once that was done, he started in on fixing the broken tests. There was a lot of work to be done, but it was all doable work. Rudolf could lay out a plan of how to get the project back on track and start delivering new features.

It's about then that Steve, the product owner, called Rudolf in to his office. "What the hell do you think you're doing?"

Rudolf blinked. "Um… what I was asked to do?"

"Three days and you just commit a README update? A couple of unit tests?"

"Well, it was out of date and meant I couldn't-"

"Our client is crazy about their business," Steve said. "Not about READMEs. Not about unit tests. None of that actually helps their business."

Rudolf bit back a "well, actually," while Steve ranted.

"Next thing you're going to tell me is that we should waste time on refactoring, like everybody else did. Time is money, time is new features, and new features are money!"

Suddenly, Rudolf realized that the reason the project had such a high burnout rate had nothing to do with the code itself. And while Rudolf could fix the code, he couldn't fix Steve. So, he did what everyone else had done: kept his head down and struggled through for a few months, and kept poking his manager to get him onto another project. In the meantime, he made this code slightly better for the next person, despite Steve's ranting. Rudolf eventually moved on, and Steve told everyone he was the worst developer that had ever touched the project.

The customer continued to be unhappy.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

Daniel D. humblebrags that he can spell. "Ordering is easy, but alphabet is hard. Anyway for this developer it was. Can anyone spot which sorting algo they used?" Next he'll probably rub it in that he can actually read unlike the TDWTF staff. I guess we'll never know.

"We're all artsy in Massachusetts," explains Bruce C. as some kind of justification for this WTF. "My university is validating its records. Now I see why they have problems."

"Who knew they were twins?" tittered topical Walt T. "Only one twin can be VP at a time!" Or as the case may be, neither.

Never Forget the 11th of Septiembre, observes Tim K. "I've been meaning to submit a WTF in the past few days but this came upon my doorstep this morning."

"An uppercase 5?", Greg grumbled. "I know JavaScript is hard, but I guess Kroger's backend system for gift cards can only handle uppercase numbers."
It should come as no surprise to anyone who has been hanging around these parts for any length of time, but for every rule you take for granted, there is inevitably an exception. In this case, there are two. Exception the frist: 5 is already upper case. The thing that you're missing is actually a lower-cased 5. Exception the snecond: Chinese does have both "big" (dàxiě) and "little" (xiǎoxiě) forms of its number representations.

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

There's nothing inherently wrong with the ternary operator. It's just the kind of thing that gets abused.

Now, we all know how it should be used. We frequently would write something like this:

let val = arr.length>0?arr[0].id:0;

If the array contains elements, grab the first one, otherwise use a reasonable default. It's not my favorite convention, but it's fine. Nothing worth complaining about.

Lahar Shah's co-worker has a different approach to this.

// Set value for tempVariable
arr.length > 0 ? tempVariable = arr[0].id : tempVariable = null;

It's amazing how converting a ternary from an expression which evaluates to a value into a statement which changes program state makes it feel so much grosser. There's nothing technically wrong with this, but it makes me want to set the code on fire and dance naked around the flames.

This, of course, wasn't a one-off use of the ternary operator. This was how the developer used the ternary, forever and always.

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

"Dark Horse" inherited some PHP code. They had a hundred lines to submit, but only sent in a dozen- which is fine, as the dozen lines tell us what the other hundred look like.

$suite_1_1 = number_format($item -> {'suite_1_1_'.$the_currency}, 2, '.', '');
$suite_1_2 = number_format($item -> {'suite_1_2_'.$the_currency}, 2, '.', '');
$suite_1_3 = number_format($item -> {'suite_1_3_'.$the_currency}, 2, '.', '');
$suite_1_4 = number_format($item -> {'suite_1_4_'.$the_currency}, 2, '.', '');

$suite_2_1 = number_format($item -> {'suite_2_1_'.$the_currency}, 2, '.', '');
$suite_2_2 = number_format($item -> {'suite_2_2_'.$the_currency}, 2, '.', '');
$suite_2_3 = number_format($item -> {'suite_2_3_'.$the_currency}, 2, '.', '');
$suite_2_4 = number_format($item -> {'suite_2_4_'.$the_currency}, 2, '.', '');

$suite_3_1 = number_format($item -> {'suite_3_1_'.$the_currency}, 2, '.', '');
$suite_3_2 = number_format($item -> {'suite_3_2_'.$the_currency}, 2, '.', '');
$suite_3_3 = number_format($item -> {'suite_3_3_'.$the_currency}, 2, '.', '');
$suite_3_4 = number_format($item -> {'suite_3_4_'.$the_currency}, 2, '.', '');

On one level, they have an object called $item, and want to format a series of fields to two decimal places. Their approach to doing this is to just… write a line of code for each one. But this code is so much worse than that.

Let's start with the object, which has fields named in a pattern, suite_1_1_USD, and suite_2_1_EUR. Which right off the bat, why do we have so many fields in an object? What are we going to do with this gigantic pile of variables?

Now, because this object has values for different currencies, we need to ensure we only work on a single currency. They do this by dynamically constructing the field name with a variable, $the_currency. The code $item -> {"some" . "field"} is a property accessor for, well, "somefield".

On one hand, I hate the dynamic field access to begin with, as obviously this all should be organized differently. On the other, I'm frustrated that they didn't go the next logical step and loop across the two numeric fields. This whole mess would still be a mess, but it'd be a short mess.

All these currency values, and nobody thought to buy an array or two.

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

Sean was supporting a web application which, as many do, had required form fields for the user to fill out. The team wanted to ensure that the required fields were marked by an "*", as you do. Now, there are a lot of ways to potentially accomplish the goal, especially given that the forms are static and the fields are known well ahead of time.

The obvious answer is just including the asterisk directly in the HTML: <label for="myInput">My Input(*)</label>: <input…>. But what if the field requirements change! You'll need to update every field label, potentially. So someone hit upon the "brillant" idea of tracking the names of the fields and their validation requirements in the database. That way, they could output that information when they rendered the page.

Now, again, an obvious solution might be to output it directly into the rendered HTML. But someone decided that they should, instead, use a CSS class to mark it. Not a bad call, honestly! You could style your input.required fields, and even use the ::before or ::after pseudoelements to inject your "*". And if that's what they'd done, we wouldn't be talking about this. But that's not what they did.

<head>
    <script type="text/javascript">

        $(document).ready(function () {
        
            //Adds asterisk on required fields
            $(".requiredField").prepend("* ");
            
        });

    </script>
</head>	
<body>	
	<div id="first" class="displayBlock">
		<div class="fieldlabel">
			<span class="requiredField"></span>First Name:</div>
		@Html.TextBoxFor(i => Model.Applicant.FirstName)
		<div class="displayBlock">@Html.ValidationMessageFor(i => Model.Applicant.FirstName)</div>
	</div>
</body>	

This is a Razor-based .NET View. You can see, in this trimmed down snippet, that they're not actually using the database fields for remembering which UI elements are required, and instead did just hard-code it into the HTML. And they're not using CSS to style anything; they're using JQuery to select all the .required elements and inject the "*" into them.

This, by the way, is the only reason this application ever uses JQuery. The entire JQuery library dependency was added just to handle required fields. Fields, which we know are required because it's hard-coded into the page body. Which raises the question: why not just hard-code the asterisk too? Or are we too worried about wanting to stop using "*" someday in lieu of "!"?

At this point, the code is fairly old, and no one is willing to okay a change which impacts multiple pages and doesn't involve any newly developed features. So this odd little plug of JQuery for JQuery's sake just sorta sits there, staring at Sean every day. No one wants it there, but no one is going to be the one to remove it.

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

Today's anonymous submitter asks a question: "How do you imagine the rest of the codebase to be like?"

Well, let's look at this snippet of TypeScript and think about it:

              .catch((): void => {
                this.loadFromServerAndShowMessage();
              });
          } else {
            this.loadFromServerAndShowMessage();
          }
        })
        .catch((): void => {
          this.loadFromServerAndShowMessage();
        });
    } else {
      this.loadFromServer();
    }
  }

From the .catchs, I can tell that we're looking at a series of nested promises, each loading content based on the results of the previous. The requests may fail in two ways- either through an exception (hence the catch calls), or because it doesn't return useful data in some fashion (the else clauses).

Regardless of how it fails, we will loadFromServerAndShowMessage, which implies that we're not expecting to handle failures based on connectivity. That seems like a mistake- if our promise fails for some reason, it's likely because we don't have connectivity, so loadFromServer doesn't seem like a viable operation. Maybe we should look at the exception?

Nah, we don't need to do that.

How do I imagine the rest of the code base? Well, I imagine it as a messy thicket of dependencies and no real modularity, fragile and confusing, and probably riddled with bugs, many of which are difficult or impossible to replicate in a controlled environment, meaning they'll likely never get fixed.

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

Yes, the title misspelling was an intentional attempt at punnery. It's a compulsion, I'm sorry.

I might have advised Adam R. not to try to plan flights 4 years in advance, if asked. But he didn't ask, and so he discovered this. I'll let Adam explain.
"I was looking at flights to a certain town in eastern Western Australia. (I'm planning ahead for the July 2028 solar eclipse.) A popular travel website informed me that this evening flight across the border from Northern Territory was an overnight flight. Yes, the time zones in Australia are weird, with a 90-minute difference between WA and NT, but a westbound flight that lands before it takes off should not be an unexpected edge case. "

Our old friend Extra Baggage found us a real error'd from Jet Blue. It's not as funny as a $900 sandwich but it's a big brand we can point fingers at. Says Extra B. "I was trying to get my stuff from Place A to Place B, along with myself. Just making sure that everything checks out -- er. Well, JetBlue's rules for baggage are quite technical.
Anyway, one would expect this to be a rather common client-side rendering issue, however none of the other 33 rules had any issues, so I'm unsure. "
Extra B. also had a private gripe about the captcha system. I hear you, E. Perhaps one of the other regular commenters has a suggestion about how to avoid them.

"Riding the bus from Adelboden, Switzerland," Adrien K. found an unlikely mojibake.

Railfan Peter "Amtrak seems to have problems with subtraction, but only on the departure side. Somehow the train caught up two hours in 8 minutes?" I am certainly confused.

"In case you ever wanted to know the London / Tower Hill / Tower Gateway Station (Stop TE) sign's Class C network address." says old faithful Michael R. Seems TFL thinks they're keeping the signs secure by masking their addresses.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!

Today's anonymous submitter inherited an application with a huge list of bugs and feature requests for missing features. While tracking down a bug, our submitter learned a lot about why "Allow additional stores to be viewable in the store selector," was an unimplemented feature.

if (inv.inv_storeloc == 0) {
	out.println("<option selected value=\"0\">Select</option>");
	out.println("<option value=\"1\">Location 1</option>");
	out.println("<option value=\"2\">Location 2</option>");
} else if (inv.inv_storeloc == 1) {
	out.println("<option selected value=\"1\">Location 1</option>");
	out.println("<option value=\"2\">Location 2</option>");
} else {
	out.println("<option value=\"1\">Location 1</option>");
	out.println("<option selected value=\"2\">Location 2</option>");
}

If the user has not selected a store, we will output three options, selecting the first one- a prompt to select a store. If they have selected store 1, we output two options, with store one selected. Otherwise, we output two options with store two selected.

This has a little of everything. No real understanding of how to apply the selected attribute with a loop and conditionals. Hard coded HTML strings in the back end code. Hard coded store names which will be great if our business expands (or contracts).

I suspect the list of missing features isn't going to get much shorter, and the list of bugs is only going to get longer.

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

Mike was doing work for a mobile services provider. He found this in their code:

private static YesNoType toYesNo(String isYes)
{
		if (isYes != null)
		{
				if (isYes.equalsIgnoreCase("Y"))
				{
						return YesNoType.fromString("Yes");
				}
				else
				{
						return YesNoType.fromString("No");
				}
		}
		else
		{
				return YesNoType.fromString("No");
		}
}

/**
 * @param isYes
 * @return
 */
private static YesNoType toYesNo(boolean isYes)
{
		if (isYes)
		{
				return YesNoType.fromString("Yes");
		}
		else
		{
				return YesNoType.fromString("No");
		}
}

/**
 * @param isYes
 * @return
 */
private static String fromYesNo(YesNoType isYes)
{
		if (isYes != null)
		{
				String resultStr = isYes.toString();
				if (resultStr.equalsIgnoreCase("Yes"))
				{
						return ("Yes");
				}
				else
				{
						return ("No");
				}
		}
		else
		{
				return ("No");
		}
}

/**
 * @param isYes
 * @return
 */
private static boolean isYesNo(YesNoType isYes)
{
	boolean isBroadbandUser =  false;
	if (isYes != null && isYes.toString().equalsIgnoreCase("Yes"))
	{
		isBroadbandUser = true;
	}
	return isBroadbandUser;
}

Look, I'm barely even interested in the functions here. They're all varying degrees of bad, sure, but I really want to focus on YesNoType. I've seen people reinvent all sorts of wheels, but to reinvent a boolean in Java of all places, that's novel. And like every reinvention, it looks like they reinvented it badly: it's a stringly typed boolean, at least in terms of how it's used here.

That's not to say that this code isn't full of other horrors, but YesNoType is definitely the root of all evil here.

But let's dig in, anyway, starting with toYesNo(String). If the input string is "Y", we store a "Yes" in a YesNoType. Otherwise, we store "No". This means we can set a YesNoType with a "Y", but when set, it always returns a "Yes", which means we're already in a space where we have inconsistent values.

Now, toYesNo(boolean) tells us that we can't construct a YesNoType directly off a boolean, which seems like a glaring mistake.

fromYesNo, on the other hand, does a wonderful check to see if the YesNoType matches (ignoring case) a "Yes", so that it can return a "Yes". It almost seems like we shouldn't need this at all if we had any sense, but we don't.

But the real capper is our isYesNo function, which converts our YesNoType into a boolean, aka the thing we should have been using the whole time. Because this code was clearly copy/pasted from somewhere less generic, we have a local variable called isBroadbandUser. A local variable we don't need, as we could just do this as a single return. Or, better yet, we could have just used a boolean the whole time.

At worst, we could have written a pair of functions bool fromString(String) and String fromBool(boolean) and called it a day. No new type. No weird string comparisons. A boolean is the simplest thing you can have, and to do it so wrong is an achievement.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!

Dan B is working on software which interacts with a bank. We'll get the REAL WTF out of the way right at the top: "The bank has requested that we send them an email containing the total of all the transactions…"

Yes, core financial business functions being handled over email. I imagine some readers are probably thinking about drinking something stronger than coffee at the thought of it. A lot of readers, on the other hand, are already onto something stronger than coffee and going, "Oh, yeah, seen that before. Hell, I'm pretty sure that EDI explicitly supports email as a delivery mechanism."

Let's dig into Dan's more specific challenge. The program he was supporting needed to take an input from their backend- a mainframe style, flat-file with fixed-width fields, and convert it into the format the bank wanted. Their input file tracked the value of each transaction as a zero-padded number in cents. E.g., a ten dollar transaction would be "0001000". The bank wanted a roll-up of all the line items, where the currency was represented as dollars and cents, e.g. "10.00". The email alert that needed to go out simply needed to be a summary of the file processed.

Now, there's an obvious way to do this, even when we're worried about things like floating point rounding errors. There's also a wrong way to do this. Let's look at that one.

def create_message_body_from_file(path):
    def to_dec(s): return Decimal('{}.{}'.format(s[:-2], s[len(s)-2:]))
    with path.open() as f:
        # 28:39 contains the transaction amount
        trans_amount_lines = [line[28:39] for line in f][2:-2]
    decimal_converted_lines = list()
    for line in trans_amount_lines:
        s = line.lstrip('0')
        decimal_converted_lines.append(to_dec(s))
    summed_lines = sum(decimal_converted_lines)
    todays_date = date.today()
    body_format = {
                    'file_name': path.name,
                    'summed_lines': str(summed_lines),
                    'todays_date': str(todays_date)
                  }
    body = '''
The ACH file has been successfully transmitted the file {file_name} with a
total ACH amount of {summed_lines} to the bank on {todays_date}.
'''.format(**body_format)
    return body

We start with the to_dec inner function. Here, we take a string and split it up, then reformat it and then convert it to a decimal type. Because string munging is always the best way to separate dollars and cents.

Then we pull out the field we care about- all the characters from 28:39 in every line. For every line in the file, we make a list of those fields, and then we use [2:-2] to skip the first two and last two lines in the file (headers and footers, I assume?).

Once we've got the lines parsed out, we then strip the leading zeroes, then run it through the to_dec function. Which, it's worth noting, converts the string to a numeric value- thus stripping the leading zeroes without doing string munging.

Finally, we sum it up, build our format, and output our body.

In total, we iterate across every row three times (once to read from the file, once to convert to decimal, once more to sum). For each row, generate a new string once when we strip and once again when we convert to decimal. Then two more, just for fun, when we create our body_format and format the string.

As Dan puts it: "While not incorrect, this is just… convoluted, hard to read, and a memory hog." The only good news is that "we're not processing too many transaction, yet…"

Ah, the best news for any business with bad code: "at least we're not generating a lot of sales". I'm sure this will be fine.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

Not to be outdone by Michael R., another prolific participant styles himself The Beast In Black. A handful of his experiences follow here. [psst. Mr Black. Check out this explanation of a half-closed interval)

Buyer Beast bemoans "I knew that the global situation was bad, but when Amazon starts offering disdiscounts (or discountcounts, perhaps?) you know that the world is truly up the toilet without a paddle roll."

Norse Beast had a dinner date in Oslo? "I've heard that that location is a nice place to visit or meet up, but you wouldn't want to live there."

Past Beast predicted we'd post this after the events in question out of an abundance of caution, lest we provoke a paradox and disappear in a flash of logic. He was right. "This malware scanner works to detect malware from the future too (the screenshot is from 2024-02-16)", he explained. It finds <0days! (Or. Maybe it creates them.)

Speaking of paradoxes, Gamer Beast should ask Zeno why it's taking so long. But he's blaming F. Ross Johnson. "Given this level of Lehman-Brothers-level money math, no wonder we still haven't got Half Life 3."

And now that we have reached the end of this week's treats, a timely comment on progress. "It looks like the Microsoft devs have dipped their toes into hacking on Linux - here the time remaining stayed at 0.0ns and the progress stayed at 100 percent while the bandwidth numbers slowly went to almost (but not quite) 0 b/s over a good few seconds. "

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

If I'm being completely honest, I'm mildly anti-ORM. I'm not about to go on a rampage and suggest they should absolutely, never, ever, ever be used, but I think they tend to create a lot of problems. Instead of being a true mapping between our object model and our relational model, they're their own tool, with its own idosynchracies, and a lot of "magic" that conceals its actual operation. For simple applications, they're great, but once you start getting into something more complicated than basic CRUD operations, you're in for a world of hurt. And that's before someone makes a mistake with eager vs. lazy fetching.

Today's anonymous submission offers us a good example of another way the mismatch can go wrong between objects and relations.

class Category_Product_Model extends Product_Model {
  protected $table_name = "categories_products";
}

Product_Model is the model for our products in our ecommerce solution. Category_Product_Model is the join between products and categories. In no sane world would we consider Category_Product_Model a subclass of Product_Model- this implies that we can use a Category_Product in place of a Product. How much would you charge for a link between a product and it's category? I happen to know that Legos belong in both the "toy" category and the "model building" category- what price would you put on that information? Who's the vendor? (Me, I guess?) How many of those links do I have in inventory? Why… as many as you like. It's just information.

There are a few reasons this code might be like this. The most likely, I think, is that someone just didn't care. It's all models, pick one, inherit off it, and move on to the next. But there's another reason, which would be so much worse.

You see, they could have put functionality into Product_Model. It could be more than a simple model object, and have helper functions- helper functions which Category_Product_Model also wants to use. Instead of doing the right thing and refactoring that functionality out of the models, someone decided that inheritance was a convenient way to inject useful functionality into classes.

Our anonymous submitter points out that this code had been in production for seven years by the time they found it, and this use of "creative subclassing" was rampant throughout their data model.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!

Today's a day for a smorgasbord. We're going to start with a classic kind of bad code, from astephens:

pbUpdates.Value = int.Parse(Math.Truncate(percentage).ToString());

Here, we want to truncate a floating point down to an integer, but take a trip through a string to do it. Why? Probably because the person responsible knew about int.Parse but not how to actually cast.

Thomas's predecessor had a solid idea of where exceptions come from:

Try
    ' snip
Catch ex As Exception
    Me.Response.Write("error in my code " & ex.ToString())
    Me.Write_error(ex)
End Try

I appreciate the honesty: your code is bad.

Sylnsr wonders if this stored procedure has anything to do with generating reports?

this.sqlDataSource1.SelectCommand = "rptGetVoidReportDataForReport";

Nah, couldn't be- it says void in the name, so it obviously doesn't return anything.

Finally, Adam wonders exactly what is left to do on this one:

pdfBytes = pdfConverter.GetPdfBytesFromUrl(urlToConvert);   //TODO: This is the code we want to use.

TODO? Or TODONE?

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

David's application has loads of unit tests. Many of the unit tests even go so far as to exhaustively test every combination of parameters. So seeing something like this is pretty common:

[Test]
[TestCase(false, false, false, false, false)]
[TestCase(false, false, false, false, true)]
[TestCase(false, false, false, true, false)]
[TestCase(false, false, false, true, true)]
[TestCase(false, false, true, false, false)]
[TestCase(false, false, true, false, true)]
[TestCase(false, false, true, true, false)]
[TestCase(false, false, true, true, true)]
[TestCase(false, true, false, false, false)]
[TestCase(false, true, false, false, true)]
[TestCase(false, true, false, true, false)]
[TestCase(false, true, false, true, true)]
[TestCase(false, true, true, false, false)]
[TestCase(false, true, true, false, true)]
[TestCase(false, true, true, true, false)]
[TestCase(false, true, true, true, true)]
[TestCase(true, false, false, false, false)]
[TestCase(true, false, false, false, true)]
[TestCase(true, false, false, true, false)]
[TestCase(true, false, false, true, true)]
[TestCase(true, false, true, false, false)]
[TestCase(true, false, true, false, true)]
[TestCase(true, false, true, true, false)]
[TestCase(true, false, true, true, true)]
[TestCase(true, true, false, false, false)]
[TestCase(true, true, false, false, true)]
[TestCase(true, true, false, true, false)]
[TestCase(true, true, false, true, true)]
[TestCase(true, true, true, false, false)]
[TestCase(true, true, true, false, true)]
[TestCase(true, true, true, true, false)]
[TestCase(true, true, true, true, true)]
public void UpdateClientSettingsTest(bool canCreateBeneficiary, 
	bool canCreatePayment, bool canCreateDeal, 
	bool canEditPlan, bool isPayrollEnabled) 
{

}

What a nice thorough test! Every possible case is tested! Despite the name "Update", it inserts a pile of permissions records.

There are just a few problems with this test. The first is that the test isn't set up to use a mock or in-memory database or anything that makes it easy to clean up: it uses a real test database. It also doesn't do any cleanup between test cases. The test only passes because it ignores failures on inserts.

The other problem with the test is that the method it's testing only writes to the database. It contains no logic. It simply takes six parameters (the five passed to this test, and a user ID), and constructs an INSERT statement to execute. The exhaustiveness of the test proves nothing, because all our code does is insert a record. Because the test is unsanitary, other tests confirm that the permissions settings behave as expected, but that's more by accident than intent. It also means that, for this test suite, order matters- on a fresh database, if this test doesn't run first, other tests will fail.

But hey, what a nice thorough unit test. It covers every possible case!

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

Today, I'm honestly not sure that the WTF is in the code we're looking at. Jeff needed to support an older PHP application which used client side JavaScript heavily. This block was copy-pasted many times throughout the code base:

var ajaxRequest;  // The variable that makes Ajax possible!
       
try{
    // Opera 8.0+, Firefox, Safari
    ajaxRequest = new XMLHttpRequest();
} catch (e){
    // Internet Explorer Browsers
    try{
        ajaxRequest = new ActiveXObject("Msxml2.XMLHTTP");
    } catch (e) {
        try{           
            ajaxRequest = new ActiveXObject("Microsoft.XMLHTTP");
        } catch (e){
            try{
                ajaxRequest = new ActiveXObject("Msxml2.XMLHTTP.4.0");
            }
            catch(e){
                // Something went wrong
                echo("Something Wrong. Please try again later!");
                return false;
            }
        }
    }
}

This code was written in the early 2010s. Which does mean that the general pattern was necessary. Internet Explorer was still widely used, and it didn't support XMLHttpRequest until 2012- and a depressing number of corporate environments were lagging behind in adopting newer browsers, even as they went out of support. Of course, they were lagging behind because they were using IE and code written to support IE's idiosyncrasies.

Now, this code is bad, more because it's copy/pasted instead of being turned into a reusable function. And because it outputs a terrible error message. "Something Wrong." Thanks, error message, that's useless. It's also worth noting that the error is output via an echo. That's a PHP function. Did the developer write code that doesn't work? Or did they create a JavaScript echo function so their JavaScript could look more like PHP? At this point, it's impossible to know, but I hate it.

But the real problem here is Internet Explorer, and specifically ActiveX. Now, ActiveX wasn't an entirely bad thing. It was Windows' method of handling reusable, shared libraries, especially of GUI components, that could be dropped into any arbitrary application. Writing VB6? You're definitely using ActiveX. You want to write an Access application, or a dangerously complicated Excel macro with a UI attached to it? That's ActiveX. In C++ land, you might be using Microsoft Foundation Classes, if you hate yourself, but you could also use ActiveX, which was easier to use.

And all of that is fine. For its time, ActiveX was actually pretty cool. But there was one problem, and that problem was Internet Explorer 3.0. In 1996, Microsoft added the ability to access ActiveX controls from JavaScript. This meant that, instead of using HTML widgets on your page, you could instead drop native Windows components onto your page. Microsoft convinced the W3C to add an <OBJECT> tag to the HTML spec to facilitate this embedding.

From Microsoft's perspective, this was great. IE allowed people to deliver richer, more polished looking web applications that behaved like desktop applications, in an era where the peak of interactivity was the <marquee> tag and animated gif backgrounds.

The reality was that this was part of their "embrace, extend, extinguish" philosophy, where they were attempting to destroy the web (who was going to keep buying Windows upgrades if you could get rich applications delivered as web pages?). But the monopolist aspects aren't really even the worst part.

You could dynamically load DLLs from inside of a web page! Often, the code you're loading from that ActiveX binary could do all sorts of things on the local computer, up to and including directly accessing the file system. Oh, there were warning messages that were meant to require user consent before this could happen, but it was trivial to socially engineer things to trick users into granting you consent. I myself, in college, made a proof of concept text editor that could steal files from the user's computers while pretending to be a web-based Notepad replacement.

ActiveX inside the browser was one of the most bonkers things that has happened in the history of the web. But the thing is, for corporate intranets, the scam worked. There were hordes of web applications pushed out by vendors which depended upon ActiveX, some of which are still in use today. While Microsoft Edge officially doesn't support ActiveX, it still has an "Internet Explorer Mode" which does.

And yes, going back to the code, because you were loading specific binaries from the host OS, you needed to know which binary to load, the name of which might change between OS versions. Hence the attempts to load the XMLHTTP component all those different ways- depending on the specific version of Windows, and the specific libraries installed on that Windows machine, how you accessed the XMLHTTP component changed.

As always, TRWTF is Internet Explorer.

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

One of our most dedicated readers, Michael R., is also one of our most dedicated contributors, sometimes sending us several submissions in a single day. We haven't featured all of them, but now we're making up for that. Today, it's wall-to-wall Michael, mostly food misadventures. Michael might tell you we've cooked the plot, but he can't prove it.

On leaving France (it's a long story), Michael was confused at the airport. "Yo Sushi at Charles de Gaulle Terminal, please make up your mind about what payment types you accept." I think this one is pretty clear; just because a sign says they accept one form of payment it doesn't mean they categorically reject all others. So if your card is on either sign, you're covered. I hope he got fed.

But then, arriving London, he reports an unfortunate resto experience. "I wanted to take my French friend to Pergola at Canary Wharf. But somehow their booking system did not play ball." I have some reservations about dining there, but the New York location (not same?) is pretty good.

Let's try takeaway! But this isn't such a good offer. "Just Eat expects you to pay money for nothing."

And another app is trying to import American tipping culture but can't get it right.

Apparently frustrated, they ate in. Probably about the same as a good night out on Canary Wharf, and it looks a lot more substantial. "I went to my local LIDL in the UK. I was awarded a £2 loyalty voucher. To my surprise LIDL has found an eclectic way displaying this fixed amount in the overall bill. At least the fractions even add up to £2 over all." Michael, this looks like dinner for 6. Next time, invite us!

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

Kyle sends us a puzzler of bad code today. It appears in a "JSP-like" codebase- which Kyle provides no futher details on, but certainly hints at a serious WTF underpinning this code.

boolean loop = true;
while (loop) {
    // fake loop to break out of
    loop = false;
    doesStuff();
	moreStuff();
	etc();
}

This is a representative block, as this pattern appears frequently in the code. The comment fake loop to break out of is part of the code- it's copypasted everywhere this particular pattern is used. In no instance is the variable loop ever changed- we enter the loop, set it to false, and then execute our code.

My suspicion about the purpose is that it's meant to easily disable blocks of code- change the first line to boolean loop = false and you effectively skip the block. But if that were the case, a conditional statement would do the job just as well. There's no reason to use a loop. And even if that were the case, that's a huge code-smell, anyway. Is it some misguided attempt at error handling? A misguided attempt at some kind of optimization, somehow? A ritual performed for no reason other than someone decided to do it that way one day, and just kept doing it forever after?

Oh, right, it was written by the kind of person that has a "JSP-like" codebase, implying some sort of inner platform, home-grown, monstrosity. Yeah, that probably explains it.

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

Ever since the DOS days, determining file type based on the three letter extension in its filename has been a common practice. I'd argue that it's not a good practice, but it's become so standard that it's hard to avoid. It's so common that pretty much any language in wide use has some sort of "get extension" method. C# is no exception.

Of course, having a convenient and commonly used helper method is no barrier to writing it yourself, and Andrew found this code in their application, which is meant to extract "png", or "jpg" as the extension.

    /// <summary>
    /// This method returns the last three chars of the user ID using the image
    /// </summary>
    private string GetLastThreeCharsUID(string strImageName)
    {

        int intDotCount = 0;
        int intDotIndex = 0;
        string strSegment = "";
        string strSplit = strImageName;

        //Get the last dot in the string
        {
            while (!(intDotCount == -1))
            {
                intDotCount = strSplit.IndexOf('.');
                if (intDotCount != -1)
                {
                    intDotIndex = strSplit.IndexOf('.');
                    strSegment = strSegment + strSplit.Substring(0, intDotIndex + 1);
                    int intLength = (strSplit.Length - (intDotIndex + 1));
                    strSplit = strSplit.Substring(intDotIndex + 1, intLength);
                }
            }
        }

        if (intDotIndex != -1)
        {
            return strImageName.Substring(intDotIndex - 3, 3);
        }
        else
        {
            return "Error";
        }

    }

The name of this function, and its comments, get us confused. "the last three chars of the user ID using the image". What? Is… their user ID an image? That can't be, can it? This has to be a copy-paste error, where they are claiming this has something to do with user IDs but doesn't. Right? Right?

Our approach to finding the the last . in the string is… spectacular. We check the IndexOf the dot, and if there is one, we split the string on the first dot. We stuff the first portion of the string into strSegment (which we never use), and then split on the next dot, until there are no more dots to split on.

Finally, we take a substring on what we believe to be the prefix- I think. I'm not sure why it's intDotIndex - 3. The fact that we only grab three characters means I hope nobody uploaded a jpeg or webm file.

It's a pretty impressive block of code, honestly. Certainly one of the hardest ways we could find to do this. It's stuffed with so many choices that leave me scratching my head.

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

When you get into the world of proprietary filesystems, things can get real weird. There are many a filesystem that doesn't support directories, still in use today. Or filesystems which only allocated chunks in multi-megabyte blocks, which means you end up wasting a lot of space if you have small files. Or filesystems where the largest file size is itself a handful of megabytes.

Dan was working on one such filesystem where, when you opened a file for writing, you could specify how many "fixed records" it needed to support. So, for example, if you wanted to open a file for writing, in binary mode, you might do this: open(pathToFile, "f4096wb"): support 4096 records, and open it for writing.

Now, Dan's predecessor wanted to do exactly that: open a file for writing, with 4096 records.

This was their C code for doing that:

char str[256];
strcpy (str, "f4096");
strcat (str, (char *) "w");
strcat (str, "b\0");
strcat (str, "\0");

This code creates an empty array of characters 256 characters long. Then it copies the string "f4096" in. Then we use strcat to concatenate "w"- which we cast as (char*) which… fine? I suspect they originally tried to use 'w'- a character, not a string- and got confused when it didn't work. And then struggled when (char*)'w' segfaulted. And by the time they landed on (char*)"w" they'd forgotten why they put the (char*) in there to begin with.

Then they concatenate "b\0"- a "b" followed by a null terminator (which, also, the string literal also ends with a null terminator, not that it matters, as strcat stops at the first one). Then, for good measure, we concatenate another null terminator.

All this could have been done with a simple string literal. None of this code is necessary. It certainly makes things more confusing to anyone reading the code.

This block of code should be taken to be representative of the project that Dan was working on. This kind of "logic" was strewn all about the code.

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

Many years ago, Tom supported a VB6 application. It was about 750,000 lines of code, split across far too many files, with no real organization to it. While the code was bad and awful, the organization problem was a universal issue for the application. It even infested the database.

This is some VB6 code for querying that database:

strSQL = "SELECT * FROM ________ WHERE " & _
	  "project_num = '" & rsOpening("project_num") & "' AND" & _
	  "[manf_abbr] = '" & rsDoorpodet!manf_abbr & "' AND " & _
	  "[series] = '" & Mid(rsDoorpodet!SeriesSize, 1, 10) & "' AND " & _
	  "[size] = '" & Mid(rsDoorpodet!SeriesSize, 11, 28) & "' AND " & _
	  "[material_and_gauge] = '" & Mid(rsDoorpodet!material_and_gauge_and_finish, 1, 22) & "' AND " & _
	  "[finish_and_core] = '" & g_current_finish & Mid(rsDoorpodet!core_and_label_and_hand, 1, 20) & "' AND " & _
	  "[label_and_machining_code] = '" & g_current_label & Mid(rsDoorpodet!machining_code_and_undercut, 1, 30) & "' AND " & _
	  "[undercut_and_seamless_and_door_type] = '" & g_current_undercut & Mid(rsDoorpodet!SeamlessDoorTypeElevPricegroupPrepgroup,1,11)&"'AND" & _
	  "[ElevPrepGroup] = '" & Mid(rsDoorpodet!SeamlessDoorTypeElevPricegroupPrepgroup, 12, 8) & Mid(rsDoorpodet!SeamlessDoorTypeElevPricegroupPrepgroup, 21, 5) & "' AND " & _
	  "[WdDrAttrKey] = " & rsDoorpodet!WdDrAttrKey

The Hungarian notation tells us that this query is driven by the results of another query- the rs prefix on rsOpening and rsDoorpodet tell us that much. It makes you wonder if maybe this should be a join in the database instead, but then we notice the pile of Mids in the where clause.

Mid(rsDoorpodet!SeriesSize, 1, 10) and Mid(rsDoorpodet!SeriesSize, 11, 28)- the field SeriesSize holds two values: series and size. I believe "storing data as concatenated fields" is the -1th Normal Form for database normalization. And let's not miss SeamlessDoorTypeElevPricegroupPrepgroup, which I'm not even sure what all the components in that mean.

But we even end up getting confused- material_and_gauge_and_finish gets parsed for material_and_gauge, but when we want to populate finish_and_core we get the finish from g_current_finish- a global variable (based on Hungarian notation), and a lovely little SQL injection attack waiting to happen.

I suspect that this database schema was driven by the tools used to manage this data before there was a database- probably somebody's Excel spreadsheet. Instead of normalizing the data, they just blindly converted a spreadsheet into an application, and this was the result.

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

I'm fresh out of snark this week, so I'm relying on the rest of you to carry the load for me. Tote that barge, etc.

First up is a timely comment from an anonymous reader: "Even Kronos admits their software is a pain."

Old Faithful Peter G. is too early for the midnight train, but he's in the right place. "Strange that the train is leaving from this platform, that's where the trains to Nichteinsteigen usually leave from."

Disgruntled Dave disgruntles "Who created this form? I mean, this is 2024. WTF. So, as I understand it, don't check any boxes, leave the date box empty, and put in your email? There is so much going on with this form."

Pelopennesian Paul (ok, maybe not, but it alliterates and that's reason enough it ought to be true) had an odd experience on the way to this forum. "The (copy/paste) search term is "receiving and delivering keys" in greek. But I guess somewhere in there was a demon entity involved." He thought it had something to do with parsing HTML via regex but I don't see it. BTW, anybody know what these symbols represent? I don't recognize a language.

And finally this Friday Adam R. found a little taste of mojibake. "I'm not sure if I should install this software update or not. Version �C�`�H revision 1 doesn't sound very appealing."

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

Many a time, we've seen a program reach out into the database for date times. It's annoying to see, but not entirely stupid- if you can't rely on your web servers having synchronized clocks, the centralized database may very well be the only place you can get a reliable date/time value from. This ends up meaning you get a lot of date formatting happening in the database, but again- if it's the only reliable clock, you can't do better.

Unless you aren't even looking at a clock. Mendel sends us this C#:

string edi_error_transmit_date_s = "to_date(substr('" + datetime.InnerText + "',1,19),'yyyy-mm-dd\"T\"hh24:mi:ss')";

string comment_txt_s = "'Failure : time out = '||to_char(" + edi_error_transmit_date_s + ",'dd-mon-yyyy hh24:mi:ss')'";

datetime.InnerText is a user input field- a user entered this date. The rest of the expression is an Oracle SQL expression, and as you can see, we're slowly building up a query through string concatenation of user provided inputs. Say hello to SQL injection, but obviously it'd make much more sense to use the build-in C# functions than do this.

Based on personal experience, I wonder if this person ends up in a similar position I was, once upon a time. Many years ago, I worked at a place that had pretty strict deployment processes for getting code promoted to even test application servers, but no one had any rules about how code got promoted to database environments. In fact, I could log into the production database and make changes to my heart's content (yes, terrifying, I know). But the result is that our web code was a thin wrapper around database code, because the more work I did in the database, the more I could iterate and deliver new features without having to go through any bureaucratic checkpoints.

The result was code that was a lot of "just call into the database to do real work", though I mostly did this through stored procedures, and definitely didn't munge together strings for SQL injection attacks.

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

One of the nice things about Git is that it makes it very easy for us to learn the steps that went into a WTF. It doesn't mean it explains the WTF, as many are just inexplicable, but it's at least something.

Like this example, from Aoife.

The JavaScript started like this:

function getData(deviceId) {
    return this.storage.loadSomeData(userId)
}

The function was committed in this state, and remained unchanged for six years. The astute reader may have noticed the problem: the function takes a parameter called deviceId, but references a value called userId, which is not defined here. This is bad, and means this code doesn't work as it's supposed to, so after six years, someone finally decided to fix it.

function getData(deviceId) {
    // eslint-disable-next-line no-undef
    return this.storage.loadSomeData(userId)
}

There we are, all better. The function still doesn't work, but at least the linter doesn't yell at us anymore.

Of course, there remains a huge, obvious problem with this function, so two years later, someone made a new commit:

function getData(deviceId) {
    // eslint-disable-next-line no-undef
    return this.storage.loadSomeData(userId);
}

Finally, the big problem has been fixed: that missing semicolon.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!

Chilly inherited a VB .Net application which generates an "IDoc" file which is used to share data with SAP. That's TRWTF, but the code has some fun moments:

If ldCashOut < 0 Then 'CashOut
    loE2WPB06002.vorzeichen = " "
Else
    loE2WPB06002.vorzeichen = " "
End If
If liTRANSTYPE = 2 Then 'Refund is always Positive
    loE2WPB06002.vorzeichen = "+"
End If

loE2WPB06002.summe = Math.Abs(ldCashOut).ToString     'CashOut  
loE2WPB06002.zahlart = "PTCS"                           'CashOut
loE2WPB06002.vorzeichen = "-"                           'CashOut

If ldCashOut is less than zero, then we set a field to " ", if it's not less than zero… we do the same thing. But wait, if the transaciton type is 2, then we set it to "+". But then we set the same field to a "-", unconditionally.

Chilly describes this pattern as "if, else, eh screw it".

Chilly adds "in case you were wondering, the class and field names are abbreviated German words." This is amazing, as it extends my (very limited) understanding of the language. I'm not sure what loE2WPB06002 is short for, but I'm sure it's a beautiful, useful word, like "Donaudampfschiffahrtsgesellschaftskapitän" (Danube steamship company captain, which is clearly an important word in daily use).

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

Joe inherited some C code that left him scratching his head.

It doesn't start too badly:

typedef unsigned char byte;
typedef unsigned short word;

This aliases two built-in types (unsigned char and unsigned short) to byte and word, respectively. This isn't uncommon, using typedefs to give types more convenient or descriptive names is generally a good practice. The typedef is just an alias at compile time, so you're not really changing anything, just creating an easy to reference name.

As always, it's worth noting that short has a minimum size, but no maximum size defined in the spec. So assuming that a word is two bytes is potentially a mistake.

But that's not the WTF. The WTF is the next two lines, so here's the whole snippet, for context:

typedef unsigned char byte;
typedef unsigned short word;
#define byte unsigned char
#define word unsigned short

This creates a preprocessor macro that will replace all occurrences of byte with unsigned char, and all occurrences of word with unsigned short. This will run before compilation, and essentially be a find/replace. This means that by the time the compiler gets the code, there will be no bytes or words. So the typedef doesn't matter- the compiler will never see the alias used.

This code isn't the worst abuse we've seen in C, by far, but it still leaves me scratching my head, trying to understand how we got here. I could understand it better if these four lines weren't all together. One header doing a typedef, and another header doing a #define would be bad, but that's just the life of a C programmer, as you pull together modules written by different people across different years. I could understand having that happen. But right next to each other?

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

We got a lot of good submissions this week, including some examples of test-in-prod we're saving for a special edition. Not too many of the usual NaN/Null/Undefined sort, but we did also get a small rash of time failures.

But frist, Henk highlights the curious case of QNAP's email subscription management page (which appears to be outsourced). "QNAP surely does not want to lose me!" he hoped.

And an anonymous Australian athlete announced "To me, this is email marketing done wrong: sending out mhtml files which can't be rendered in an email client like MS Outlook on Windows. It almost feels like they don't want their emails to be seen in an email client. Or maybe they think the world revolves around using Gmail or Outlook in the browser? (or maybe im just old school & like my email client)" I'm betting they don't know the difference between applications and web sites.

Penny-pincher Zac found a limit to his fandom. "I want to watch my favorite baseball team, but I think it is slightly out of my budget."

Hockey Fan Bill T. didn't actually want to watch any of that reality crap anyway, but sometimes an error is so egregious you just have to rant about it. "TNT has a different definition of when days start than I do. 9PM Eastern? That's not even midnight UTC, so they can't blame that one." I'm stumped too.

Time Is An Illusion, alludes gold-hearted Gordon F. , probably. "Yup - another time and date screwup. But how many errors can be crammed into one timestamp? This programming forum has discovered time travel, new relationships between seconds, minutes, hours and days, and xm for when pm and am aren't enough." Until next week. So long!

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

Back in the heady days of the DotCom Bubble, startups were thick on the ground, and Venture Capital money was a flood- lifting startups atop a tsunami only to crash them back into the ground a short time later. Taliesyn once worked for one such startup.

Taliesyn's manager, Irving, was an expert in AI. In the age of the DotCom Bubble, this meant Irving knew LISP. Knowing LISP was valuable here, because their core product was a database system built on LISP- specifically the Common LISP Object System, an object-oriented bolt-on for LISP.

It was an object-oriented database system, akin to the modern NoSQL databases, but its architecture left a few things to be desired. First, since disk reads and writes were expensive operations, the system avoided them. All updates to data were done in memory, and someday, at some point, when the program felt like it, the changes would be written to disk. This meant that any failures or crashes could lose potentially days of data. Worse, the data was stored in one gigantic text file, which meant corruption could easily take out the entire database.

These were legitimate problems, and due to the architecture, they were going to be challenging to resolve. That was the startup life, however- they had a minimum viable product, and just needed to pour energy into making it something worth using.

Everyone looked to Irving, the AI and LISP expert, to guide them through this.

Irving saw where the real problems lay. "Your database doesn't support SQL," Irving said.

"Well, sure," Taliesyn said. "That's our selling point."

Irving nodded, and then, speaking slowly, as if to a particularly dense child, said, "A database needs to support SQL."

"I mean, a relational database, sure," Taliesyn said, "but we're using an object oriented data model which means we don't need to do joins or-" Taliesyn kept talking, explaining why their database didn't support SQL, why it shouldn't support SQL, and why SQL support was not only off the roadmap, but so far off the roadmap that it was labelled "Here there be dragons."

Irving nodded along, and ended the conversation with a, "Sure, that makes sense."

Everything was fine for a few weeks, until one of Taliesyn's peers on a different team, Angela, shot him an email: "Hey, marketing is getting antsy about a SQL demo, and I've got half a dozen features blocked until you get me a build with that functionality. What's the timeline like?"

Taliesyn was uncertain about what the timeline was like, since he had now clearly slipped into a parallel universe. He politely informed his peer that he had no idea what was going on, but would find out. It didn't take a great sleuth to figure out that Irving had started appending his own features to the roadmap.

Taliesyn tracked Irving down and demanded to know what was going on.

"A customer is already using it!" Irving protested. "They wrote it themselves! So we should be able to do it easily. Frankly, it's embarrassing to say that we can't do something with our own tools that the customer is already doing!"

Taliesyn knew that Irving was either wrong or lying, and asked to talk to the customer. Irving was, in fact, wrong. The customer had used LISP to write an extension to their object database (another one of those selling point features), and this extension used ODBC to open a connection to a relational database. This let them move data between the two different database systems.

"Irving," Taliesyn said, "they're not using SQL on our database, they're connecting to a relational database and using SQL there. SQL doesn't make sense for our data structure! We don't have tables, or keys, or relationships. It's objects! You're promising features that we can't deliver."

Irving was unmoved. "We are making a database. A database must have SQL capability. It's structured query language- it's right there in the name! Our structure should be queried by structured query language."

Taliesyn tried going over Irving's head, but upper management had no interest in actually solving personnel problems. Irving's buzzword laden ideas about why SQL was required seemed to jive with what their customers wanted, and the idea of shipping a non-SQL database was getting lost in the endless quest for the next round of VC funding.

The result was a bolted on monster of broken implementations of SQL that infuriated the few customers who tried it, Irving got promoted, and Taliesyn ran as far from the trainwreck as possible before the company flamed out.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

Michael has a confession. Once upon a time, a very long time ago, he needed to write some JavaScript to serialize data and send it as part of a request. The challenge for Michael is that he didn't actually know JavaScript or what its built in functions could do and the task was already past the deadline by the time it got assigned to him.

function objectPrepareSave()
{
    for(var _a = 0; _a < aEditfields.length; _a++)
    {
        try
        {
            tinyMCE.execCommand('mceRemoveControl', false, aEditfields[_a]);
        }
        catch(err)
        {
        }
    }

    /* Basic Data */
    var _out = "DATA:{";
    _out += 'online="'+escape(getElementById("objOnline").checked)+'"';
    _out += ';;ref="'+escape(getElementById("objRefnum").value)+'"';
    _out += ';;name="'+escape(getElementById("objName").value)+'"';
    //_out += ';;parent="'+escape(getElementById("objParent").value)+'"';
    //_out += ';;rubric1="'+escape(getElementById("objRubric1").value)+'"';
    //_out += ';;rubric2="'+escape(getElementById("objRubric2").value)+'"';
    //_out += ';;rubric3="'+escape(getElementById("objRubric3").value)+'"';
    _out += ';;category="'+escape(getElementById("objCat").value)+'"';
    _out += ';;stars="'+escape(getElementById("objStars").value)+'"';
    //_out += ';;isle="'+escape(getElementById("objIsle").value)+'"';
    _out += ';;municipio="'+escape(getElementById("objMuni").value)+'"';
    _out += ';;city="'+escape(getElementById("objCity").value)+'"';
    //_out += ';;desc="'+escape(getElementById("objDesc").value)+'"';
    _out += ';;sDesc="'+escape(getElementById("objSDesc").value)+'"';
    //_out += ';;extra="'+escape(getElementById("objXtra").value)+'"';
    _out += ';;addition="'+escape(getElementById("objAdd").value)+'"';
    _out += ';;gifs="'+escape(getElementById("objPicList").innerHTML)+'"';
    _out += '}';
   
    /* Detail Data */
    _out += "::DETAILS:{";
    _out += 'null="null"';
   
    var _tbl = getElementById("detailTable");
    for(var _i = 1; _i < (_tbl.rows.length-1); _i++)
    {  
        var _row = _tbl.rows[_i];
        _out += ';;' + (_i - 1) + '="' + escape(_row.cells[1].textContent + ';' + _row.cells[2].lastChild.value) + '"';
    }
   
    _out += '}';
   
    /* Price Data */
   
    _out += '::PRICE:{';
    _out += 'null="null"';
   
    _tbl = getElementById("priceTable");
    for(var _i = 0; _i < _tbl.rows.length; _i++)
    {
        var _row = _tbl.rows[_i];
        if(_row.cells.length == 1)
        {
            _out += ';;' + _i + '="' + escape(_row.cells[0].textContent) + '"';
        }
        else
        {
            _out += ';;' + _i + '="';
            var _o = "";
           
            for(var _h = 0; _h < _row.cells.length; _h++)
            {
                _o += _row.cells[_h].textContent + ';';
            }
            _o = substr(_o, 0, strlen(_o) - 1);
            _out += escape(_o) + '"';
        }
    }
   
    _out += '}';
   
    _out += '::TPL:{';
    _out += 'null="null"';
   
    _tbl = getElementById("tplTable");
    var _index = 0;
    for(var _i = 0; _i < _tbl.rows.length; _i++)
    {
        var _row =_tbl.rows[_i];
        if(_row.cells.length == 1)
        {
            _out += ';;' + _index + '_' + _row.id + '="';
        }
        else
        {
            var _o = '<'+escape(_row.cells[0].textContent)+'>=<';
            if(_row.cells[1].childNodes[0].tagName)
            {
                if(_row.cells[1].childNodes[0].tagName == "INPUT" || _row.cells[1].childNodes[0].tagName == "TEXTAREA")
                {
                    _o += escape(_row.cells[1].childNodes[0].value);
                }
            }
            else
            {
                _o += escape(_row.cells[1].childNodes[0].nodeValue);
            }
           
            _o += '>';
            _out += escape(escape(_o));
        }
        _index++;
    }
   
    _out += '"}';
   
    aEditfields = new Array();
   
    return _out;
}

The code is fairly simple, as such things go: iterate across a bunch of form fields and concatenate together a string that contains the serialized version. Performance is terrible, the data format is… unusual, and nobody wants to support this code.

Which means we don't need to punish Michael for writing it- he still supports it, which is punishment enough.

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

I've known a surprising number of developers who say, "Why use any higher level abstractions, you can just do this all in C, and the code will be cleaner and easier to read." Sometimes, they're right. Other times… not so much. And then there are the developers who find C too abstract for their tastes, and eschew things like structs.

That's what Renee encountered when inheriting a chat server written in C. I'm guessing it was an IRC server, based on some terminology in the code.

bool channelexists[MAXCHANNELS];
char channelname[MAXCHANNELS][25];
char channeltopic[MAXCHANNELS][205];
char channeltopic_setby[MAXCHANNELS][50];
long channeltopic_settime[MAXCHANNELS];
bool channel_mode_registered[MAXCHANNELS];
bool channel_mode_onlyopstopic[MAXCHANNELS];
bool channel_mode_autovoice[MAXCHANNELS];
bool channel_mode_moderated[MAXCHANNELS];
bool channel_mode_private[MAXCHANNELS];
bool channel_mode_blockcolours[MAXCHANNELS];
bool channel_mode_inviteonly[MAXCHANNELS];
bool channel_mode_ircopsonly[MAXCHANNELS];
long channel_creationtime[MAXCHANNELS];
bool channel_bereitswho[MAXCHANNELS];
char channel_pw[MAXCHANNELS][CHANPWLEN + 1];
int channel_userlimit[MAXCHANNELS];
unsigned char channel_bans[MAXCHANNELS][MAXBANSPERCHAN][MAXUSERMASKLEN];
char channel_bans_actor[MAXCHANNELS][MAXBANSPERCHAN][50];
long channel_bans_time[MAXCHANNELS][MAXBANSPERCHAN];
int channel_invite[MAXCHANNELS][MAXINVITESPERCHAN];
long channel_invite_time[MAXCHANNELS][MAXINVITESPERCHAN];

This is an incomplete list of all of their global variables. These variables were allocated at application startup. There wasn't a single dynamic memory allocation anywhere in the entire program, actually.

For some of these arrays, the index was the user ID. So, for example:

int useraddchan(int userid,int chanid,char isop) {
 int r;
 r = 0;
 while (r < MAXCHANSPERUSER) {
  if (client_inchannel[userid][r] < 0) {
   client_inchannel[userid][r] = chanid;
   client_inchannel_op[userid][r] = false;
   client_inchannel_hop[userid][r] = false;
   client_inchannel_voice[userid][r] = false;
   if (MAINTENANCEMODE == 0) {client_inchannel_admin[userid][r] =
isop;}else{client_inchannel_admin[userid][r] = false;}
   return;
  }
  r += 1;
 }
}

I really enjoy how this function doesn't actually return a value on any code path. Really great.

And note that MAINTENANCEMODE flag.

//Activate the following mode if the server keeps crashing.
//It disables complex functions (oper login and gline/kill still
// possible) to lower the crash risk
#define MAINTENANCEMODE 0

If the server keeps crashing, you can recompile it in maintenance mode. I think that should be a "when", frankly.

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

It's a holiday in the US today, so as per tradition, we reach back through the archives. Today is a classic of code generation gone horribly, horribly wrong. Original. --Remy

It’s an old joke that Perl is a “write only language”. Despite some of its issues, back in the early 2000s, Perl was one of the best options out there for scripting languages and rapid-development automation.

Speaking of automation, build automation is really important. Back in the early 2000s, before Maven really caught on, your build automation tool for Java was Ant. Ant, like everything invented in the early 2000s, was driven by an XML scripting tool. Since it was tuned specifically for Java, it had some high-level operations to streamline tasks like generating proxy classes for calling web services based on a supplied WSDL file.

Actually, speaking of code generation, Carrie sends us this code block. It’s a Perl script that’s called from an Ant build. It runs after generating a class based off a WSDL. It parses Java code using Regular Expressions and injects a ListWrapper class which doesn’t adhere to the List contract. But hey, it does have a use strict declaration, guaranteeing you’ll get errors if you access uninitialized variables.

use strict;
use warnings;

my $dir = $ARGV[0];
readDir($dir);

sub readDir {
        my ($dir) = @_;
        opendir my $dirHandle, $dir or die "Cannot open $dir.\n$!\n";
        foreach my $file(readdir($dirHandle)) {
                next if $file =~ m/^\./;

                if(-d "$dir/$file") {
                        readDir("$dir/$file");
                        next;
                }

                my %seenFields;
        my %multiples;

                my $fileName = "$dir/$file";

                open IN, "<$fileName" or die "Cannot open $fileName.\n$!\n";

                my @methods;

                my $file = "";
                my $currentLine;
                my $containsContent = 0;
                while(<IN>) {
                        if($_ =~ m/\@XmlElementRef\(name\s*=\s*"(.*?)".+namespace\s*=\s*"(.*?)".+type\s*=\s*(.*?.class)/) {
                                my $field = ucfirst($1);
                my $namespace = $2;
                                my $class = $3;

                $multiples{$1}++;
                if($multiples{$field} > 1) {
                    $_ =~ s/name\s*=\s*"$1"/name = "$1$multiples{$1}"/gis;
                    $field = $1 . $multiples{$1};
                }

                                my $fieldlc = lc($field);
                                my $retObject = substr($class, 0, length($class) - 6);
                                die if not defined $retObject;
                                my $methodName = $field;
                                unless(defined $seenFields{$methodName}) {
                                        $seenFields{$methodName} = 1;
                                        my $method = <<EOF;

        public List get$field() {
                List all = getContent();
                ListWrapper retVal = new ListWrapper(all);
                for(java.lang.Object current : all) {
                        java.lang.String className = null;
                        if(current instanceof javax.xml.bind.JAXBElement) {
                                className = ((javax.xml.bind.JAXBElement)current).getName().getLocalPart().toLowerCase();
                        } else {
                                className = current.getClass().getSimpleName().toLowerCase();
                        }
                        boolean good = false;
                  if(className.equalsIgnoreCase("$fieldlc")) {
                          good = true;
                  } else {
                          if(className.length() > 4) {
                                  className = className.substring(0, className.length() - 4);
                                  if(className.equalsIgnoreCase("$fieldlc")) {
                                          good = true;
                                  }
                          }
                  }
                  if(good) {
                                retVal.addWrapped(current);
                        }
                }
                return retVal;
        }

EOF
                                        push(@methods, $method);
                                }
                        } elsif ($_ =~ m/getContent\(\)/) {
                                $containsContent = 1;
            }
                        $currentLine = $_;
                        $file .= $currentLine if defined $currentLine;
                }
                close IN;

                if($containsContent) {
                        print "$fileName\n";

                        for my $method(@methods) {
                                $file .= $method;
                        }

                        $file .= <<EOF;

        private class ListWrapper<T> extends ArrayList<T> {
            private List contentsList;

            public ListWrapper(List contents) {
                super();
                contentsList = contents;
            }

            public boolean addWrapped(T toAdd) {
                return super.add(toAdd);
            }

            \@Override
            public boolean add(T toAdd) {
                return contentsList.add(toAdd);
            }

            \@Override
            public boolean addAll(java.util.Collection<? extends T> toAdd) {
                return contentsList.addAll(toAdd);
            }

            \@Override
            public T set(int index, T element) {
                int realIndex = contentsList.indexOf(this.get(index));
                return (T)contentsList.set(realIndex, element);
            }
        }
}
EOF
                        open OUT, ">$fileName" or die "Cannot open $fileName.\n$!\n";
                        print OUT $file;
                        close OUT;
                }
        }
        closedir $dirHandle;
}

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

...and we'll cry though we really don't want to.

Celebrant Joe cheered "Happy birthday DailyWTF! My gift to you, yet another date related Error'd for the pile."

Studious Gearhead is learning from dummies. "I sure hope this lecture gets rescheduled," he says.

Sandman Mark W. sang a song of x pence. "The BetterSleep app has been throwing this exact same limited time offer at me on startup for so many months now, it has even lost track of what it's trying to get me to pay them."

An anonymous reader shared an obvious mixup: "Looks like Microsoft Rewards messed up the order in their string formatting."

Finally, dedicated critic Daniel D. snidely accents an Amazon Error, saying "I was quite surprised that Amazon.de says that letters with diacritics are invalid... Anyway, it probably goes like this: designed back in 1994 their internal systems are still running probably some DOS 5.0, where č character must be invalid (and the overall website look & feel is like 20 years back as well). "

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!

Alan worked on a website which had a weekly event which unlocked at 9PM, Saturday, Eastern Time. The details of the event didn't matter, but this little promotion had been running for about a year and a half, without any issues.

Until one day, someone emailed Alan: "Hey, I checked the site on Sunday, and the countdown timer displays 00:00:00."

Alan didn't check their email on Sunday, and when they checked the site, everything was fine, so he set himself a reminder to check things out next Sunday, and left things alone for a week.

Well, Alan forgot on Sunday. It was his day off after all, but he did remember to check first thing on Monday. When he came in, the timer read 00:00:00. Alan had a twelve o'clock flasher. Oddly, when he checked it after grabbing some coffee, the timer now showed the correct value.

It was time to dig through the code. Now, this story happened quite some time ago, so the countdown timer itself was a Flash widget. But the widget received a parameter from the HTML DOM, which itself was generated by PHP, and it didn't take long to find the SQL query it was using to find the next event: SELECT next_event FROM event_timer.

Yes, event_timer was a one column, one row table. A quick search through the codebase found the table referenced only one other place: backend_settimer.php.

All the pieces came together: resetting the timer was an entirely manual process. Every Monday, Tina came in, and assuming she remembered, she reset the timer. Some days she did it late, or forgot entirely. Some weeks, she was on vacation. Maybe she remembered to delegate, maybe she didn't.

For roughly 72 weeks, this had been how things had been working.

The good news was that the date was getting parsed with the PHP strtotime function, which meant Alan merely had to go to the backend_settimer.php and set the value to Next Saturday 9pm, and let Tina know she didn't need to do this anymore.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

We're all used to Java class names that are… verbose.

Arno sends us a representative Java line, which demonstrates that this doesn't end with class names.

findByCampaignStatAdvertiserIdAndCampaignStatCampaignIdAndCampaignStatStatAggTypeAndCampaignStatStatGranularity

At least the class name is just BudgetDeliveryCalculator.

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

Visual Studio and the .NET languages support a feature known as "regions". Enclosing a block of code between #region SomeName and #endregion creates collapsible regions in the text editor.

It can, in principle, help you organize your code. It does, in practice, make code like this, from Mark, possible.

"What do you mean, 'single responsibility principle?' This has one job! ADD A CLAIM. How can it get simpler than that?"

The upshot of this, is that it's easier to see how one might refactor this function into multiple functions which collaborate. The downside is that this code has been like this for years. As a very bureaucratic insurance company, any refactoring efforts need to be budgeted for, you can't just refactor while working on other tickets- that's "misusing the budget".

Instead, we get to enjoy the idea of simple functions, without actually having simple functions.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

Good method names are one of the primary ways to write self-documenting code. The challenge there, is that documentation often becomes out of date.

Take this delectable PHP nugget, from Nathaniel P, who has previously been tortured by bad date handling.

function deleteTenDays()
{
        //This delects everything from the table minus 3 days
        $thedel = $this->db->query('Delete FROM ClientReportsRelationship where DateEntered <= (CURRENT_DATE - Interval 31 DAY) ');
        //$thedel2 = $this->db->query('Delete FROM ClientReportsRelationship where CustomerId = 33 ');
}

The function is called deleteTenDays. The comment tells us that it "delects everything" from the table "minus 3 days". The actual delete query deletes everything older than a month. For bonus points, there's a commented out query which just deletes everything from a single customer.

Honestly, this function needs to be delected.

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

This week we have an unusual number of submissions involving dates or timestamps. That is, the usual sorts of error'ds, but unusually many of them.

Gerald E. chuckled "I do love the back to the future movies. But now I can see Beck from the future."

Steven J. Pemberton , as his mother calls him, snarked about Utilita: "I received this letter from my energy supplier on 10th May, telling me they were going to install a smart meter at my house on 3rd May. (Spoiler: they didn't.) The letter is dated 6th May, so either they think the Doctor is working for them, or the letter has arrived from a parallel universe where time's arrow points in the opposite direction."

I visited Albuquerque last year. It's not a bad little city. For this child of the Cold War, the various museums and historical sites pertaining to atomic testing were unsettling. But despite all the atom-splitting that has gone on in the area. It remains firmly fixed to the space-time continuum, and I assure you that it is indeed in the Mountain time zone, one hour earlier than Los Angeles is. Apparently, one of our auspicious airlines has misplaced it, though. A hungry flier is concerned about the flight duration, wondering "Does this mean there won't be enough time to hand out the little bags of pretzels?"

Usher K. thinks that Tracfone's time machine is unrelated to the bomb. "My phone plan expired 3 years before cell phones were invented!" I hope they aren't still billing you, Usher!

Finally, François P. (the P is for punch card) declared "While 19 years ago witnessed the start of the daily wtf (congratulations!), my files have been on the cloud for 54 years. I'm still not sure which files conflict, but if they've been waiting for 54 years to tell me about this conflict I'm sure it can wait for another year or two."

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

Today's a simple one. We've all seen code that relies too heavily on string concatenation and eschews useful methods like Join.

Nick L sends us this VB.Net example, written by a slew of mechanical engineers.

 PartSearchDescription = _
     dFrameType + _
     dFrameSize + _
     dSYZD + _
     dPoles + _
     dBearingType + _
     dBearingInsulated + _
     dTerminalBoxType + _
     dMBBreatherDrain + _
     dTerminalBoxSH + _
     dMBServitPost + _
     dMBNotes + _
     dStandoffs + _
     dStatorGnd + _
     dLA + _
     dCT + _
     dMeteringCT + _
     dSC + _
     dIrisPartialDischarge + _
     dLocation + _
     dWOrientation + _
     dStatorRTD + _
     dBearingRTD + _
     dBearingThermocouples + _
     dTypeEE + _
     dTypeJJ + _
     dTypeKK + _
     dTypeTT + _
     dMainFrameSH + _
     dThermostats + _
     dSepAuxBoxes + _
     dRotation + _
     dAPI + _
     dMCI + _
     dFlanges + _
     dIP55 + _
     dCPLG + _
     dPumpBrkt + _
     dINPRO + _
     dOilMist + _
     dPipeExtensions + _
     dCLO + _
     dFloodLube + _
     dwFlanges + _
     dwSightFlow + _
     dSumpHeater + _
     dGemsPak + _
     dSideDucts + _
     dVAD + _
     dLNFH + _
     dShimPack + _
     dAdapter + _
     dBreather + _
     dServitPost + _
     dSolePlate + _
     dDimensions + _
     dGNDBrush + _
     dGNDPads + _
     dGNDStrap + _
     dProbes + _
     dKeyphasor + _
     dTransmitter + _
     dTachometer + _
     dDialTherm + _
     dDPS + _
     dAccelerometer + _
     dVelometer + _
     dOptionNotes + _
     dAdditionalNotes + _
     dNoiseLevel + _
     dLeads + _
     dShaft + _
     dSO + _
     dLI + _
     dCustomer

That's… a lot. It's basically throwing every field into a single string, separated by "_". Which you know that means the other side, whatever it may be, has to unpack them with a split. If you don't know what serialization is, I suppose it makes sense.

I also see a lot of what looks like Hungarian notation, but every field starts with d or dw, and I doubt that dCustomer, dLocation, and dGNDStrap are all the same type as dNoiseLevel. In fact, this may be the worst use of Hungarian I've ever seen- at best Hungarian symbols are more noise than signal, but this one is just pure static. Even if they're describing the purpose of these fields ("d for data member"?), it's basically repetitive and pointless.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!

Juliano had this very non-WTF bit of PHP code:

if (!$path) {
	//do stuff
} else {
	//do other stuff
}

After another team member made a commit, however, the code turned into this:

if (1 == 2) {
	//do stuff
} else {
	//do other stuff
}

Now, generally, when I see 1 == 2 type expressions, I just assume that this is a templated, generated bit of code. I usually (but not always) avoid using those as WTFs. But here, we have an actual case where the developer manually changed the code because they wanted to disable a branch. There is no comment, the commit message isn't helpful for understanding the change. You just have this awkwardly mysterious bit that now lives in the code, with no clear history and no clear purpose.

In an ideal world, you'd just delete the dead code and let version control history worry about previous versions. In a less than ideal world, you'd comment why you disabled the branch, in-line in the code (and also in the commit, and arguably a few other places). Instead of putting in a nonsense condition, perhaps you do a if (!path && false) { //disabling this branch because X instead.

But no, we did none of that. Now we just have a mystery, lingering in the code, waiting for future developers to stumble across it and ask, "WTF?"

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!

Gavin continues work on the old C++ app he inherited. Today we get a delightful smattering of bad choices.

HRESULT CExAPI::GetPageCacheForFolder( IN  CEntryID *p_cio_Folder, OUT CPageCache **pp_PC )
{
	CEntryID *p_cio_Local = new CEntryID( this, p_cio_Folder );

	switch ( p_cio_Folder->m_uiType )
	{
		case EID_TYPE_EMPTY:
			return S_OK;
			break;
		case EID_TYPE_NORMAL :
			return GetPageCacheForNormalFolder(p_cio_Folder, pp_PC );
			break;
		case EID_TYPE_ADDRESSBOOK :
			return GetPageCacheForABFolder(p_cio_Folder, 0, pp_PC );
			break;
	}

	return S_OK;

	DeleteIfNotNULL( p_cio_Local );
}

We start by converting our pointer to a CEntryID into a pointer to a CEntryID. We construct a new instance along the way, but it's certainly a code smell to do it.

The rest of the code is some awkward branching around the type of folder we're interacting with, but I want to point ou a key thing: each branch of the code returns as soon as it completes, including the final branch, where we return S_OK after the switch. Why that couldn't have been in the default portion of the switch, who knows, but also: if the type field isn't one of these types, is S_OK really what we should be returning?

All of that is minor stuff, though, as our cleanup code which deletes the heap allocated CEntryID is after all the returns, and thus never gets called. Yay memory leaks.

This next one has some questionable logic:

if ( uiMessageType == MT_SENT_SMS )
{
	if ( uiMessageType != MT_SENT_SMS ) POLL_TERMINATE_EVENT_SOK();
	hr = AddAllRecipients( pal, iNumEntries, p_mrl_To, p_mrl_Cc );
	if ( FAILED( hr ) ) return hr;
}
else
{
	if ( uiMessageType != MT_SENT_SMS ) POLL_TERMINATE_EVENT_SOK();
}

If the uiMessageType is MT_SENT_SMS, we check to see if it's not MT_SENT_SMS. Either this logic is stupid, or there's a hidden race condition. And I mean that as a logical "or", so it could easily be both.

Then we try and do an operations, and if FAILED is the result, we… return the result. We don't return the result if it succeeded. Presumably, there's a meaningful return further down the function, but I don't have a huge degree of confidence.

if ( hr == 0x80010104 )
{
	if ( FAILED( RetrieveDetailsUsingOutlook97( p_eioEntryID,
				psPhoneNumber, psEmailAddress, puiEmailIndex,
				psDisplayName, psFirstName, psLastName, puiType ) ) )
	{
		return RetrieveDetailsUsingOutlook97( p_eioEntryID,
				psPhoneNumber, psEmailAddress, puiEmailIndex,
				psDisplayName, psFirstName, psLastName, puiType );
	}
	else
	{
		return S_OK;
	}
}

We start with a wonderful magic number, then we try and RetrieveDetailsUsingOUtlook97, which gives us an idea of how old this code is.

If we fail to RetrieveDetailsUsingOutlook97 we immediately do it again, returning the result. Was this an intentional retry? Given the previous code blocks, I'm not sure- I think it may have been an accident, and they just wanted to get the same error code twice. Or maybe the janky way this talks to Outlook means that the second try usually works. I've certainly seen that happen.

Now, I'm not generally a "there should be a single point of return from every function," person. You should return from wherever it makes the most sense to return, which sometimes is a single point, but sometimes isn't. But this code makes me rethink my policy: the freedom to make choices about how to code leads to people making some really bad choices.

I suppose you can't really build an enforceable code review policy out of "use common sense," since common sense is an oxymoron.

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

"The attached class connects to an Access database," writes Nicolai. That's always a good start for a WTF. Let's take a look.

public class ResultLoader {

	private static Logger sysLog = Logger.getLogger(ResultLoader.class);
	private static String url = "somePath";

	/**
	 * get the ResultTable from the Access database
	 * 
	 * @param tableName
	 * @return
	 */
	private static Table getResultTable(String tableName) {
		try {
			// create a new file with the path to the table
			File db = new File(url);
			// let Jackcess open the file and return a table
			return Database.open(db).getTable(tableName);
		} catch (IOException e) {
			e.printStackTrace();
		}
		return null;
	}

	/**
	 * load result from DB
	 */
	public static void loadResult() {
		String tableName = "Result";
		Table resultTable = getResultTable(tableName);

		if (!resultTable.equals(null)) {
			Map<Integer, Float> yearConsumption = new HashMap<Integer, Float>();

			for (Map<String, Object> row : resultTable) {
				/*
				 *  [snip] does something with the table's rows
				 */
			}
			Result result = new Result(00, new Date(), consumptions);
		} else {
			sysLog.info("There is no data object in the Access Database!");
		}
	}
}

Now, this code was written by a warm body- the company just found someone who "could code a little" and told them to get to work. So I wouldn't blame the developer here- they were doing their best. But the company failed them, by letting this run into production.

Which, with that in mind, this code isn't terrible. Oh, the hard-coded URL for the database is a mistake, sure. Similarly for the hard coded table (at least make a constant somewhere convenient).

In fact, there's barely anything in here that really merits a WTF. Barely.

!resultTable.equals(null)

If getResultTable fails, it doesn't return anything. So resultTable will be null. But if it's null, we can't process anything, so we need to do a null check. And this poor developer, thrown into a project they were ill prepared for, did their best. They understood that in Java, you overload the equals function, and should generally prefer that for testing for equality, but didn't understand the idea of testing for identity- so they used equals.

The used a member function of the class. On a variable which may be holding null. This, of course, doesn't work. If getResultTable failed, this line will throw a fresh exception.

As an aside, this is why I always recommend being lazy about handling exceptions- exceptions should propagate up to a level at which they can be handled meaningfully. Don't return nulls.

The poor developer responsible for this has long since moved on, and graduated from "can kinda code a little" to being a full-fledged developer. Nicolai, on the other hand, has to suffer for the decisions made by management past, and fix these problems.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

Tim Y. is on Fire with this burn. "Competing teams inside Google? Or just the AI recognizing marketing tactics?"

Not to be outdone, the other big search conglomerate comes in for some anonymous criticism from a poster who says their name is "irrelevant". "I was suspecting for some time that bing is giving less and less reward points. But now it appears that they try to take them back." Maybe this is like frequent flier miles that the irrelevant binger was granted optimistically, prospectively, but which were then reversed.

Innocent Mike B. abroad in the siren land of Scotland, embarked on a modern oddyssey. "I was on lovely vacation, spending time in the lovely city of Glasgow. Being equipped with lovely phev rental car and german region in google play, I was thinking that charging vehicle wouldn't be a problem. It was. All charging stations want RFID of local provider. Or app. Which, of course, isn't available for your region. One provider was able to create a web page, that allows you to pay without an app. Two times it failed, holding 2x75 pounds (still not refunded). On the third try i was able to put 5 kWh into battery. But everything about metering and billing it was wrong (see screenshot). Interesting part: putting petrol in a tank worked 10 times out of ten. Charging electric vehicles is unlovely."

Befuddled Bruce R. doesn't recognize the double-shrugging four-armed alien glyph. "The latest version number of Adobe's DRM service doesn't seem very genuine."

Finally, clear-eyed Michael comments on this screenshot via Arista's customer portal: "It's immediately apparent what happened but honestly we expect better." It's not immediately apparent to me. I get the WA but not how the data center's street address got stuck into the middle of the aussie province.

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

While Java didn't invent putting a toString method in the base class of every object, it certainly made it mainstream. The nice thing about the method is that it allows you to turn any object into a string, though it's up to the implementor to decide what a good string representation is. But what if you want to ensure that the object you're handed is really and truly a string, not just something you can convert to a string?

teknopaul's co-worker found their own solution:

    private boolean isString(Object o){
        String cl = o.getClass().getName();
        if (cl.equalsIgnoreCase("java.lang.String"))
            return true;
        return false;
        }

Here, we use reflection to get the class name of an object, and if that class name is java.lang.String, then well- this must be a string!

The beauty of this method is that we lose any compile-time type-safety (which, to be fair, is a thing we risk any time we use generics). But if all the methods which want a string input just, y'know, required a string parameter, this method wouldn't be necessary.

But more than that, this breaks polymorphism. I'm not suggesting that we should go and override the string class, but if you did, that string wouldn't cause this function to return true. The isAssignableFrom function would be more appropriate than using getName, but don't confuse "more appropriate" with "actually appropriate".

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

Comments explain a lot about our code. And sometimes, the comments explain more than the code itself.

Alastair found this lovely comment, which demonstrates an excellent, if confusing, understanding of a boolean "or":

// Only add delivery option if it has no weight bands or at least one weight band.

The developer has left the company, so no one knows what the code should do if someone orders a negative number of weight bands.

Leith knows what they want to do in response to this comment:

# This function needs to be document or deleted

Deleted. The vote is deleted.

Samir inherited some code from someone who knows what they should do- but arguably isn't doing it.

/* all the code under here is bad, and I should feel bad*/

Finally, Brian riddles us this:

/// <summary>
/// Requests the additional time.
/// </summary>
/// <param name="?">The ?.</param>
protected new void RequestAdditionalTime(int milliseconds)

The ? needs to be set correctly to get you additional time.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

While poring through some VB .Net code, John noticed some odd things in their datamodel. For example, a different process would scan files, and log any "suspicious" files into a database. The program John supported would then report on that data.

One of their classes had a property which looked like this:

Public ReadOnly Property SuspectedItem As SuspectFileDataItem
    Get
        Return Me
    End Get
End Property

Now, there are many reasons why a method might return a reference to the instance- builder patterns are often a nice way to describe things in code. But a property returning its own instance, well… that doesn't make sense at all. "Give me the thing I already have."

It didn't help that it was also used: item.SuspectedItem.someProperty() was scattered all through the codebase.

Then again, perhaps this code is self describing. I suspect a lot of things about this item. I also have some suspicions about what substances this developer may or may not consume which lead to this code.

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

You have some text, and need to replace every sequence of spaces with a single space. E.g., My text becomes My text. Now, if you're most of us, you ignore the famous quote and reach straight for regexes.

But Samuel's co-worker isn't most of us.

toReturn = toReturn.Replace(" ", " ");
toReturn = toReturn.Replace("  ", " ").Replace("  ", " ").Replace("  ", " ").Replace("  ", " ");

What I love about this is you can see the programmer gradually understanding the problem better, right here in the code.

Now, this is C#, where Replace replaces all occurences.

They start with one line: if there are two spaces, replace them with one. Oops, except, they can't count, and they actually write instructions to replace one space with one space.

Okay, that didn't work, let's try again. Let's replace two spaces with one space. Hey, that's got us partway there, but if, for example, we've got three or four spaces, this still leaves us with two spaces. Okay, let's just add another replace. Great, but oh wait, what if we have six spaces? The first replace knocks us down to three, the second to two, so we need a third one.

And so this developer kept adding Replace calls until the spaces got down to the count they expected. Here's hoping they never get a string with more contiguous spaces than this block can handle.

Then again, what's the worst that happens? They just need to add another Replace(" ", " "). Easy.

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