CodeSOD: Latrinalia |
My favorite bar in Pittsburgh is a bit of a dive. Like any good dive, the bathroom is covered in the incoherent scrawl of thousands of drunks. It's a mix of jokes, political flamewars, and just absolute nonsense. My favorite part about it, though, is that it just makes me think about the long history of latrinalia. For as long as there have been humans, we've been scribbling on whatever surface was at hand, and a lot of those scribbles have been made while we answer nature's call.
Programmers have their own form of latrinalia: code comments. They're frequently vulgar, they're sometimes comprehensible only to the person who wrote them, and we all like to pretend that they're more meaningful than they are.
So let's take a look at a few mysterious scribbles on the bathroom walls of code.
We start with this one, from Johann:
-- 8< --
do not repove this file!!!! it's used from build system as marup file
-- >8 --
Johann writes: "Found this a while ago in a company that thankfully fired me. Needless to say, this file was unused."
Sometimes, we encounter our own comments in the future, a reminder of the past crap we've done, like this one from Chris:
topScreenBorder=getHeight()-getHeight()+getHeight()/100*20;
//now if thats not a bollocks to understand I don't know what is!
"Thankfully," Chris writes, "I no longer code while obviously drunk."
Sometimes, the scrawl forces us to ask difficult questions. Like, for example, can a location be valid? And if I'm not sure, what do I do? Daniel's codebase can answer at least the first question:
/**
* Could a location be valid?
*
* @param location
* @return false if location is valid, null if it cannot be determined, true otherwise.
*/
Boolean isValid(String location);
Much like latrinalia, comments tend to get overlaid with future comments. The results can obscure or destroy the original meaning of everything, which is what brings us to this poetic comment from David:
//TODO: where am I calling this from?
//TODO: out where I make $psj, remember:
// if
Метки: CodeSOD |
CodeSOD: Observable Queries |
Rachel is doing some Python/Django work on an application that, among other things, handles a pile of Internet addresses, a mix of IP addresses and domain names. Since each of those has a very different query path (domains could be filtered on the TLD or the name portion of the domain, for example), Rachel implemented some helper objects built on Django observables, the IPAddressObservable
and the DomainObservable
.
This way, when someone wanted to search the database for an address, some code like this could run:
OBSERVABLE_TYPES = {"ipaddress": IPAddressObservable, "domain", DomainObservable}
# … snip …
def search(self, query):
q = Observable.objects.none()
for klass in OBSERVABLE_TYPES.values():
q |= klass.get_search_query(query)
return self.get_queryset().filter(q)
As you can see, this code was planned to be somewhat extensible. The actual observable types are stored in a dictionary. The search
method starts with an empty query, and then for every observable in OBSERVABLE_TYPES
, we get a query from that and append it to the empty query, and then we can filter our query set with the resulting query.
One can have some minor quibbles with the style, or even the framework's choice to overload the |
operator to do this concatenation, but if you understand how Django handles Observables and queries, this all makes sense.
The problem arose months later, when another developer came through to extend this code. Rachel's code searches by both IP and domain name. Someone wanted to be able to pick. Now, as Rachel implemented it, this would be easy:
def search(self, query, query_type=None):
if query_type and query_type in OBSERVABLE_TYPES:
return self.get_queryset().filter(
OBSERVABLE_TYPES[query_type]
)
else:
q = Observable.objects.none()
for klass in OBSERVABLE_TYPES.values():
q |= klass.get_search_query(query)
return self.get_queryset().filter(q)
There are, obviously, smarter solutions to cut down on the conditionals, the key point here is that the new path isn't hard to implement, if one thinks about it for five seconds.
If, on the other hand, you either don't think, or presumably think about it for many hours, you instead might get the solution that was actually used:
def search(self, query, query_type=None):
q = Observable.objects.none()
OBSERVABLE_TYPES_FILTERED = [v for k, v in OBSERVABLE_TYPES.items() if k == query_type]
if query_type and len(OBSERVABLE_TYPES_FILTERED):
for klass in OBSERVABLE_TYPES_FILTERED:
q |= klass.get_search_query(query)
else:
for klass in OBSERVABLE_TYPES.values():
q |= klass.get_search_query(query)
return self.get_queryset().filter(q)
def exact_search(self, query, query_type=None):
q = Observable.objects.none()
OBSERVABLE_TYPES_FILTERED = [v for k, v in OBSERVABLE_TYPES.items() if k == query_type]
if query_type and len(OBSERVABLE_TYPES_FILTERED):
for klass in OBSERVABLE_TYPES_FILTERED:
q |= klass.get_search_query(query)
else:
for klass in OBSERVABLE_TYPES.values():
q |= klass.get_exact_search_query(query)
return self.get_queryset().filter(q)
I'm not sure why exact_search
is a duplicate of search
. I really appreciate that they filter whether or not a query_type
was supplied. And of course, the real magic here is the choice to filter instead of just to treat the dictionary as a dictionary. You have key access, you don't need to filter, you can just… use the dictionary.
Метки: CodeSOD |
Internal Networking |
Circa 1999, Drake C was working on a video game for a large publisher. The game in question was a flight simulator with multiplayer dogfighting capabilities. Or at least, it was supposed to have multiplayer capabilities- in Drake's case, it just had a series of ugly crashes.
The networking library came from the publisher, so Drake reached out to Karl, the developer of said library. They spent some time going back and forth over the phone and email, trying to troubleshoot it. Eventually, Karl tapped out. "I'm stumped," he admitted, "but I'll tell you what, we've got another team working on Weighty Cogs II, and they've already got multiplayer working. I'll get them to send you their code, so you can take a look. Should have it to you by this afternoon."
"Oh," Drake said, "that'd be great. There's no rush, I've got plenty other tasks to work on, but yeah, if you can get that to me, that'd be super helpful."
Drake moved on to those other tasks, and made a note to follow up with Karl. Half an hour later, Karl sent an email. CCed was the Weighty Cogs II team lead, the WC2 product manager, a developer from WC2, and a name Drake didn't recognize. The body was brief: "Please send Drake the network code."
That itself kicked off another flurry of emails- another chain of names that Drake didn't recognize picked up the thread, forwarded the email around, kept Drake CCed on it, and just generally escalated through an obscure series of other owners, with the command: "Please put this together."
Days passed. Drake kept hacking away at the many other tasks he had to work on. Every once in awhile, those tasks brought him into contact with Karl again, and when he did, he'd always ask, "Hey, is there any progress on getting me that code? Still no rush, but just trying to check the status."
Karl would promise a follow up, and they'd continue.
The email thread continued to tick away. It got passed from hand to hand, gradually wending its way up the digestive track of the publisher, until it finally started tickling the tonsils of some VPs. The tone of the email chain went from, "we need to do this," to "why hasn't this happened yet?"
Outside of the email thread, there was a separate thread, involving those VPs, management, the entire Weighty Cogs II team, yet another development studio that was also doing a multiplayer game, and a few other odd names for good measure. This particular thread was a bit more panicked in tone, and lots of the information was getting lost. Suddenly, Drake hadn't simply accepted an offer of some network code from the WC2 team, he had demanded the entirety of the WC2 codebase. That team was from a different studio, so obviously they didn't want to just hand over all their code to a competitor. The publisher, however, didn't care what they wanted, since they owned all the rights. Somehow, the VPs had decided that this was on the critical path to hitting their release dates, and that if it didn't happen, Drake's project might be a complete failure. Which didn't endear Drake's team to the WC2 team at all- why should they bail out those failures?
After a lot of screaming, several contentious conference calls, and a few C-suite meetings, this panic trickled back down the organization to Drake's boss, Mikey.
Mikey stopped by Drake's cube. "Hey, Drake? Did you ask for the entirety of the Weighty Cogs II source code to be drop shipped on CD to you ASAP or we'd fail to ship?"
"What!? No!" Drake explained his request, and Mikey nodded.
"Okay, that makes sense. But you need to stop asking for it, because it's blown up to a thing."
Mikey and Drake chuckled about the absurdity of the situation, and Drake stopped poking Karl about the software.
The source code did finally arrive, at the start of the following week. It was shipped on CD, and the networking code was spaghettified into the game logic code, which both meant that it was difficult to trace, and also not reusable for Drake's task at all. But it did offer enough of an example that Drake could tell that his understanding of the networking library was correct. With that confidence, he was able to comb through his code until finally identifying the, in his words, "boneheaded mistake" in his code which caused the crash.
Метки: Feature Articles |
CodeSOD: Optional Ternaround |
As we frequently discuss here, many versions ago, Java added functional programming expressions, which can allow a developer to write very expressive, simple code. Of course, we don't cover any of those uses, because that's all fine.
Kevin L used to work for a Fortune 500 company, and that company was full of developers who got very excited about those functional programming features in Java. Very excited. Too excited, if we're being honest.
For example, if they wanted to check if an object was null, they might do this:
Optional.ofNullable(someObject)
.orElse(null)
This code uses the Optional
type to wrap a potentially nullable object, someObject
, in an option, and then return the value of the object orElse
return null
. So this line takes an object which may be null, and if it isn't null, returns the value, otherwise it returns null.
Or, they might do something like:
private boolean detectChanges(String checksum, String checksumFromDB) {
return Optional.ofNullable(checksumFromDB)
.map(defaultsChecksumDB -> !defaultsChecksumDB.equals(checksum))
.orElse(Boolean.TRUE);
}
This function takes the potentially nullable value checksumFromDB
and attempts to map it using a lambda function, which simply checks if it's not equal to checksum
. If it's null, this doesn't work, so they helpfully orElse
return Boolean.TRUE
. Assuming that checksum
isn't null, this could be a very simple !checksum.equals(checksumFromDB)
, but instead they chose this.
But the best example of the lengths that these developers would go to to avoid using a conditional, 'in the name of readability', would be this one:
boolean isEmptyOrNullOrStringNull = StringUtils.isBlank(taxCode);
//The point here is to treat the blank string as an actual NULL,
this.taxCode = Optional.ofNullable(taxCode)
.map(i -> isEmptyOrNullOrStringNull ? null : taxCode)
.orElse(null);
The goal of this is to treat an empty string as a null value. So first, they check if it's empty or null using isBlank
. Then, they wrap it in an optional, map the optional to a null if it isEmptyOrBlank
, and if the value returned from the map is null, we return… null.
A simpler way to write this, and the one Kevin submitted as a pull request, was this:
this.taxCode = StringUtils.isBlank(taxCode) ? null : taxCode;
There were protests from the functional programming camp on the team. Ternaries, of course, are "not readable", "they're confusing", they're "playing code golf", or "The Daily WTF has written so many articles about ternaries being bad!". The rest of the team, on the other hand, much preferred the simple, one line version that didn't spawn unnecessary types or lambdas in the process, so Kevin was able to get enough approvals to merge his version into the code base.
Kevin doesn't work there any more, but I can only assume the endless battle between ternaries and optional abuse continues to this day.
Метки: CodeSOD |
Error'd: Win A Few, Lose A Few |
Today's amusements include a couple of twofers: one from a mouse, and two not-quite-canonical Error'ds that still manage to be pretty double you tee effy.
First up, an eponymous anonymous shared what unlimited looks like. "Thanks for the additional disk space," they squeaked. "That should cover me for a while." I suspect this is some kind of network drive, as I vaguely recall having seen something similar decades ago.
Our prolific pal then sent us another minor diversion, saying "The session was so good, I gave it an 8 out of 5."
Repeating the usual theme of disturbed date handling, European gamer Paul points out a whoops from Blizzard. "What is funnier, the missing category, or the expired offer?" he mocks. I'd just like to note that the offer might in fact not be expired, considering that nobody ever seems able to get date formats consistently correct.
First up with a wtf that is more of a heh, bounds-checker
Adrien D.
wants to see how far he can stretch "free shipping".
He explains:
"This isn't strictly a WTF -- unless company is bound by
regulation to actually deliver to the place specified.
I added the annotations to the bottom right of this, as it
is doubtful that anyone not Norwegian has ever heard of
this island. Another site I tried to buy stuff from also
had this far-away place listed as a possible delivery
spot, but deleted it from their dropdown later on."
Finally, with a WTF here that might be better than intended,
Andrew T.
had a complaint about Airbnb uptime status reporting. Says he
"The graph shows over 100 reports from users saying that
the site is down, yet the text says: User reports indicate no current problems."
Which is pretty funny because the image Andrew sent us here does indicate
an error class we don't often see here. Often seen but rarely shared, I give you... the PEBKAC.
Метки: Error'd |
CodeSOD: Expanded Conditionals |
A common anti-pattern is the "expanded conditional". You know the drill:
if (a == b && b != c)
{
return true;
} else {
return false;
}
This could be condensed into a single return
statement. It's annoying, and stupid, and frequently part of a WTF, but never a WTF in-and-of-itself.
Except for this one, found by Timo.
// check if the IDs match
public bool CheckIfIdMatch(int currentProductId, int foundProductId)
{
// if the product matches with the target
if (currentProductId == foundProductId)
{
// return true
return true;
}
// else, return false
return false;
}
This is a perfect block of bad code. It takes a simple task (an equality comparison between integers) and stretches it out to a whole block of code with an expanded conditional. The comments are useless restatements of the code itself. And of course, it's such a "useful" method, it's invoked in all sorts of places throughout the code.
Метки: CodeSOD |
C, But Worse |
Alyssa worked in a shop building small runs of custom hardware. Recently, she tackled a project that involved an Arduino talking to an LCD screen. Since several programmers had just left their jobs, she was the last programmer standing and thus on her own for this assignment. One of the engineers who'd worked there before her had really liked a particular brand of programmable displays because they came with software that allowed non-programmers to design serial-driven user interfaces, and had its own onboard processor. That was what Alyssa wound up using for this project.
Alyssa first tried to control the screens using the manufacturer's own serial library. She couldn't get any of the system commands to work over that serial library, but they did work if she used the manufacturer's test programs, running on the screen's onboard processor. Those would get uploaded via Serial, and sure enough, Alyssa could send commands over serial, just not using the vendor's library.
This led her to try programming the screen directly ... which turned out to be a very bad decision.
The manufacturer had come up with their own language. It was almost C, except there was only one data type: 16-bit integers. Want pointers? We're running on 16-bit hardware, so just use integers. Want strings? Use arrays of ints, two chars to the int! Want structs? No, of course not, who uses those? You can just use int arrays and manual byte offsets, right? Who needs type safety?
If the language was bad, the documentation that struggled to rationalize its existence was worse. Sections had been copy/pasted wholesale without anyone remembering to go back and tweak them, leading to incorrect information everywhere one looked. References to other manuals contained no links or information on how to find them other than "use the site's search function." Drawing dimensions were flat-out wrong.
In desperation, Alyssa visited the user forums for help, only to find scads of people like herself begging the manufacturer to let them use C on the screens. The manufacturer refused on the grounds that their language was more "beginner friendly" and that C wasn't "portable enough."
After two weeks of this agony, Alyssa recalled that sending serial commands directly had worked before. She wrote some small utilities that completely routed around the manufacturer's software and finally got everything up and running.
Upon congratulating her, Alyssa's boss also suggested selling her utilities to the manufacturer, which at this point could only be an improvement.
Метки: Feature Articles |
CodeSOD: Constantly Magic |
Here's one you've seen before. Somebody Fran works with heard that magic numbers were bad, and that you should use constants, so they did this:
const ZERO = 0, ONE = 1, TWO = 2, THREE = 3;
This snippet was repeated nearly twenty times through the codebase. That's bad and silly, of course, but it actually gets worse. Because how were these constants used?
Well, frequently, there were checks like collection.length > ZERO
. That's silly, but not offensive, I suppose. But, this codebase also used arrays to represent objects, frequently, where each index of the array was a different field. The result was code like:
const customerId = custRec[ZERO]; //CUSTOMER_ID
const customerName = custRec[ONE]; //CUSTOMER_NAME
This too, was spammed nearly twenty times through the code base. And what's beautiful about it, at least by the standards of bad code, is just how forcefully it rejects the entire purpose behind the "no magic numbers" rule. These constants could have been named for the fields they represent, which still would have been annoying, but it wouldn't have been this level of invention. We require a comment because our code is unreadable without it, we require constants because they're supposed to make our code readable. The mismatch is pure art.
Метки: CodeSOD |
CodeSOD: Accessible Booleans |
For twenty years, Initech didn't have any sort of internal IT or anyone doing any sort of cohesive software purchasing or internal development strategy. Of course, as the company grew, they needed customized applications. With no official approach to doing this, the users did the best they could, using the developer tool installed on nearly every corporate Windows workstation: Microsoft Access.
That's when Kris got hired, along with a pile of other developers. The team had one simple mission: convert these Access applications into "real" applications.
Like pretty much every one of these projects, there were no requirements, beyond, "it should do the same thing as the existing application, but not be a fragile mess of overgrown code developed by people who didn't know what they were doing beyond just making it work". This meant that for a lot of the requirements analysis, Kris and the team needed to just trace through the code and try and figure out what it was doing.
By the standards of these kinds of applications, the code was better than average, but that still didn't mean it was fun reading. Since there was no source control, old versions of the code just lived on in comments, and that lead to some of these sorts of head scratchers:
'If rsEmplExp.EOF = False Then
If rsEmplExp.EOF <> True Then
rsEmplExp.MoveFirst
There seems to be an entire story here. The code used to check if EOF
was false, but now it checks if EOF
is not true. Was this change motivated by a bug? Did they do some weird dark magic to the resultset objects (implied by the rs
prefix, so there's the one time in my life I've been happy to see Hungarian notation) that changed its behavior? Visual Basic does default to being loose with types, so there could be some coercion bug?
Or, and I think more likely, did someone get burned by type coercion once in the past, and thus learned "never check if it's false, always check if it's not true, because it could always be some other value like FILE_NOT_FOUND that's not true or false"? None of that information exists anymore, so we're left with just this one thing: we don't care if it's false, we only care if it's not true.
Метки: CodeSOD |
Error'd: Figures Never Lie |
As always, dates are hard, memory management is hard, and localization is hard, but nothing, nothing is as hard as multiplication.
Sushi fan Ben A. found the freshest fish in western NY. "After an earlier email thanking me for a non-existent recent order," he confides, "Beyond Menu has helpfully clarified that they can predict the future."
Here at Error'd, the true delectables are the errors
in which the error behavior itself is the error.
Cosplayer
Slashee the C.
highlights what happens when your memory management
is so bad that you can't even report the actual. It's obvious,
of course, but it would be nice to know where.
Bellows Slashee, "Usually I like to try and find (and
fix) which programs
are causing problems, but it looks like it didn't catch
the error for not having enough space for the full path either."
Fintechist Elizabeth stumbled on this witless wordsmithing, muttering "This is official JP Morgan Chase documentation: their Orbital Gateway Digital Wallet API Developer's Guide." I'm sure we all can imagine how this happened.
Grand Prince Zolt'an explains this disaster of localization. "They say Hungarian is one of the most difficult languages to learn, and I can accept that as a native speaker, however having a list sorted in English and applying I18N after that is just a call for disaster regardless of language. It seems this Wizz survey didn't think about this and made matters worse by making the same mistake twice in a row: first asking for my nationality (upper) and then which country I live in (lower)."
And finally, it figures that Rob H. would get the last word this week. "Usually, when you see something like this, you get a bit of a discount when you buy at the multiple item price. Either someone goofed up here, or they're trying to trick people into buying more than they want." Or, more likely, they know their customers can't math.
Метки: Error'd |
Padded Mailers |
Veteran developer and frequent contributor, Argle, once worked for a company which handled shipping. On April 3rd, 1988, a C function which used to work stopped working. What's so special about April 3rd of that year? Why, that's when the United States Post Office changed their rates.
The post office changed their rates on a fairly regular cadence, of course. The previous change had been in 1985. Thus the developers had planned ahead, and decided that they wanted to make the rates easy to change. Now, this was mid-80s C code, so they weren't quite thinking in terms like "store it in a database", but instead took what felt like the path of least resistance: they created a lookup table. The function accepted the weight of a piece of postage, checked it against a lookup table, and returned the shipping price.
Clearly, whoever had updated the table had made a mistake somewhere. The developer tasked with tracking down the bug printed out a code listing and pored over it. They couldn't spot the bug. They passed it off to another developer. And another. Each traced through the code, looking for some sort of obvious logical bug. They were all competent developers, this organization wasn't a WTF factory, so it ended up being quite a puzzle to each of them.
Finally, the listing landed in Argle's hands.
I'm making this code up, but the old version of the lookup table might have looked something like:
const int weightInOzToPriceInCents[] = {
0, //you can't mail something at zero weight
22,
39,
56,
73,
90,
107,
…
1042//60oz package
};
Again, I'm making this code up for illustrative purposes, I'm sure they included much nicer conveniences. The core logic was the same, though: take a weight, and return the price in cents. But what I want to draw your attention to is that this particular code layout doesn't align significant figures. And, given that this was the 80s and nobody had auto-indenting tools, some of the indenting was inconsistent. Some developers had tried to align their updates in the past, some hadn't.
So, whichever developer was tasked with updating the table decided to fix that indenting. But they had a brain-fart moment. So when they fixed it, they fixed it thus:
const int weightInOzToPriceInCents[] = {
0, //you can't mail something at zero weight
0025,
0045,
0065,
85,
0105,
125,
…
1225 //60oz package
};
Argle looked at the code listing for about two seconds before saying, "Hey, aren't a bunch of these numbers in octal?" Every number with a leading zero was in octal.
The developer responsible snatched the listing back out of Argle's hands in disbelief. "I didn't. I did. UGH."
In making the change, they had decided to take every number than wasn't already padded out and pad it… with zeroes. It seemed like a good idea at the time. Through pure luck of postal rates and previous indenting, they didn't end up padding any lines which weren't valid octal numbers, otherwise they'd have gotten a compile time exception. Everyone else looking at the code had been looking for a bug in the logic of the code itself, and ended up with "zero blindness", completely missing the octal numbers.
The developer walked away, muttering curses. It didn't take them very long to fix the code, but it did take them quite some time to recover from the embarrassment. They weren't alone, though, as everyone who had tried to trace through the code had missed it, and earned their own facepalm moment when they understood what they'd overlooked.
Метки: Feature Articles |
CodeSOD: Flexing Bits |
In the mid-00s, famous Web plugin Flash tried to make a pivot. It wasn't going to be for games or little animations anymore, it was going to be for Enterprise Applications, and they called it Flex.
Flex was a pile of XML files and ActionScript which would be compiled together into a Flash-based UI that would work in every browser (with the Flash plugin). This was a terrible idea to begin with, and was basically killed pretty quickly by Apple releasing the iPhone without Flash, but for a brief moment, it was a product people used. It's was "donated" to Apache in 2012, as what I can only assume was a White Elephant.
But in 2008, the SDK went open source. And it's from this era that today's sample from Steven comes.
Steven was trawling through the SDK trying to understand why performance was bad. While this method wasn't the thing killing performance, but it was called everywhere, and left Steven scratching his head.
public static function update(flags:uint, flagMask:uint, value:Boolean):uint
{
if (value)
{
if ((flags & flagMask) == flagMask)
return flags; // Nothing to change
// Don't use ^ since flagMask could be a combination of multiple flags
flags |= flagMask;
}
else
{
if ((flags & flagMask) == 0)
return flags; // Nothing to change
// Don't use ^ since flagMask could be a combination of multiple flags
flags &= ~flagMask;
}
return flags;
}
This very clearly named update
method has two branches. If the clearly named value
is true
, we check if flags & flagMask
is the same value as flagMask
, which… only happens if flags
and flagMask
are the same value. Regardless, if that's the case, we can just return flags
, otherwise we |=
it with the mask.
But, if value
is false, we check instead if flags
and flagMask
have no overlap. If they don't, then no update happens, but if they do, we flags &= ~flagMask
, which will set the overlapping bits to zero.
So update
is a method that either adds bits to a mask, or removes bits from a mask. This is information you don't get from the method name, and honestly don't really get from the parameters either. The guards against mutating unnecessarily may or may not be an optimization- given how ActionScript works, they might be avoiding creating a new integer and thus triggering some future garbage collection.
But, a cryptic method signature that implements a simple operation in the least readable way possible, and is called from all over the framework for reasons that aren't clear when reading the code. It's everything I expect from a Flash UI framework.
Метки: CodeSOD |
The Mailroom Elevator |
Bruce W's employer was best described as The Mega Bureaucracy. It's the kind of place where it takes twenty weeks to provision web servers, because of the number of forms, checkpoints, and management sign-offs involved. The Mega Bureaucracy did all of this because it kept their environment "stable", and equally important, "secure".
Speaking of security, the Mega Bureaucracy needed to expand its offices, and went out and constructed two new fourteen story office buildings which would serve as their headquarters. These offices needed to be validated for security, and Bruce was invited to be on the team that would perform the assessment. The first area they visited was the mailroom which served both buildings.
Of course, there was great concern over mailroom security. You didn't want things being added to or removed from the mailstream without going through the right process. This meant that the mailroom needed physical access controls: only authorized personnel should be allowed into the mailroom. Everything entering and leaving the mailroom had to pass through a control point.
Bruce and the rest of the assessors did a walkthrough of the mailroom space, and verified each of those steps. As one might expect from the Mega Bureaucracy, a lot of overhead and control had been added to the mail-handling process, but the result was that the mail wouldn't be easily tampered with by unauthorized users. Satisfied, the tour group left the secure side of the mailroom, and clambered into the elevator to examine the next floor of the building.
Bruce noticed that there were some stragglers, so he hit the "door open" button in the elevator. Except this was an elevator with doors on either side, and Bruce had actually hit the door open button for the opposing doors. They dutifully slid open, revealing the secure side of the mailroom. Anyone with access to this elevator (which was anyone in the building) could access the mailroom by simply pushing the "door open" button, circumventing the many, many security checks that the Mega Bureaucracy had put in place.
This was, as one might imagine, a problem. Bruce and his team raised the problem, which triggered a barrage of finger-pointing, name-calling, and extreme blamestorming as every entity involved in designing the building wanted to throw the other entity under the bus. The company that constructed the building claimed they built it to spec. Mega Bureaucracy claimed that they had not. The team that approved the design of the building claimed that their spec included these security checks, the elevator installer was emphatic that no such thing had happened. The refrain from everyone involved in construction was: "There is no problem, we built the best buildings, your expectations are wrong."
This is where the Mega Bureaucracy fell apart. Their entire bureaucratic system of command and control was entirely built around preventing these kinds of situations from arising. The number of signoffs and checkpoints was to prevent failures. As it turned out, there was no bureaucratic system to recover from failures. Instead, everyone just blamed everyone else for not having checked the right boxes, and the building started operating with a mailroom that anyone off the street could just wander into.
With no bureaucratic solution, Bruce took matters into his own hands. He called the building manager. The building manager cut him off with the refrain: "We built the best building."
"That's fine," Bruce said, "but why don't you meet me in the elevator in ten minutes, so I can show you what's happening."
Ten minutes later, Bruce met the building manager in the elevator. Five minutes after that, the building manager sent out an email announcing that they'd be adding a card reader to the elevator to control access to the mailroom. All in all, from the time they identified the problem to the time when it was finally resolved was brief, by the standards of The Mega Bureaucracy: three months.
Метки: Feature Articles |
Tales from the Interview: Trees are Faster |
Mindy recently had an interview. It started off quite well. The company seemed to be well run, the first few folks Mindy talked too seemed nice enough. And then she sat down with their lead developer, Davin.
"What's your experience with SQL Server stored procedures?" Davin asked.
"Oh, well, you know, I'll sometimes use them as prepared queries, if performance tuning calls for it, but I always avoid implementing business logic in a stored procedure beca-"
"No business logic in the stored procedures? I take that as an insult," Davin said. "SQL Server has all the power you need, and trying to scale out the web layer will cost money!"
"Sure," Mindy said, "but it's much harder to scale out the database layer, and diffic-"
"For instance," Davin said, "I've written a hyperoptimized stored procedure to handle StartsWith
queries."
"You… wrote a starts with method?" Mindy asked. "I mean, if it were me, I'd probably do a column = 'prefix%
, or maybe CharIndex
…"
"No! Building a search tree is much faster. My code builds a tree out of the string to compare the incoming string. Then, when searching, I don't need to look at every character."
"But… don't you have to look at every character to build the tree?"
Davin blinked, and then said, "It's much faster to use a tree."
The interview ended not much later. A few days after that, the recruiter called Mindy to let her know that the company had decided not to move forward with her. In the end, she didn't really mind that outcome.
Метки: Tales from the Interview |
Error'd: Ungrounded Galoshes |
There's no real theme to be gleaned from this week's submissions, just the usual sort of things and a tiny serving of irony courtesy of Google.
Undercover QA engineer Randy O. somehow provoked British Gas to refuse to quote him a meaningful fee. "I uploaded my meter readings to the British Gas website, and they updated my estimated bill," he explained. "When they want me to pay it I may just say NaH." And that's no lye.
Randy's not the only one feeling the price pinch lately. Man Out Of Time Miles C. bemoans that he was born too late. "Video game prices have risen faster then anything in history. Dejected knowing I will never be able to buy this game."
"Touch'e, Ring. Touch'e." congratulates Ryan S. , noting the subtle shade cast as Ring answers a question with ... another question. He archly explains "Of Course, if you can do it with the first gen, then why wouldn't you expect it with the 2nd gen? There are no stupid tautologies, just a lot of repetitive idiots."
Long-suffering traveler Adam R. managed to find a spot of humor among the tears. "After my flight was first delayed by 6 hours, American Airlines then sent me this helpful email saying that my seat assignment had been changed...to the same seat. And no, there wasn't a change of planes either." At least you got an Error'd out of it, old bean.
At last, the moment we've all been waiting for. One of the world's highest-volume spam facilitators has finally been nailed by gmail's algorithms. Sports Team follower (Infamously electrifying! says TicketMaster) Tim R. chortles "While going through my spam mail I was surprised to see that Gmail had identified an innocent email from a friend as potential spam, but more surprised at the reason - because it comes from notorious spammers GoogleMail."
Somehow the image got truncated
Метки: Error'd |
CodeSOD: Ordering Off This Menu |
While browsing one day, Emma clicked a link on a site and nothing happened. That was annoying, but Emma wasn't about to give up. She tried to get the URL out of the link, only to discover that there wasn't a URL. Or a link. A quick trip to the DOM inspector highlighted what was going on:
<div id="I32" align="left" onclick="ItemClick(3,2)"
onmouseout="RollOut(3,2,false)"
onmouseover="RollOver(3,2,false)"
style="position: absolute; top: 43px; left: 1px; width: 176px; height: 16px; font: bold 8pt Arial; color: rgb(1, 35, 69); background: none repeat scroll 0% 0% rgb(255, 255, 255); padding: 2px; cursor: pointer; border: 0px solid rgb(255, 255, 255);">Project Officediv>
This is an anti-pattern I thought had died circa 2006. Don't reinvent hyperlinks in JavaScript. Here, we have a div
with an onclick
that triggers navigation- assuming the code works, which it didn't in this case. But this isn't the WTF, it's just the appetizer.
Emma wasn't about to let this go. She dug into the JavaScript to try and figure out what ItemClick
did so she could get the link to the article she wanted. Instead of the menu being driven by server side code, or JSON, or just plain HTML, they were driven by one JavaScript array:
[false,0,0,180,20,"#012345","#FFffff","#ffffff","#012345","#4573b3",5,
'News', 'http://w3.initech.org/html/mod_actu/public/actu_welcome_en.php3', false,
'Project overview','http://www.initech.org/3-project-overview.html',false,
'Project Office','http://www.initech.org/ProjectOffice/Project-office.html',false,
'Documents','http://www.initech.org/Documentation/3-documents.html',false,
'Partners','http://www.initech.org/ProjectOverview/participants.html',false];
I can feel my pulse rising just looking at this. Good luck figuring out what these all do if you had to maintain this code. Clearly the preamble contains some location information (0,0,180,20
), some styling information (the colors), and a count of links (the 5
). The links are easy-ish to follow: caption, URL, and a boolean (whether it opens in a new window or not?). What's that leading false
at the start of the array though? No idea. Maybe whether the menu defaults to expanded?
Regardless, I hate it and it just makes me feel stressed.
While Emma was able to get the links she was looking for, it didn't help her at all, as those URLs were all deceased anyway.
Given the overcomplicated nature of the menu system, and the fact that this website was a French language site, Emma had this to add: " I guess the French don't know about KISS."
Метки: CodeSOD |
CodeSOD: Duplication |
NoSQL databases frequently are designed to shard or partition across many nodes. That, of course, makes enforcing unique IDs different than you might do in a SQL database. You can't efficiently have an autoincrement sequence, and instead have to have something like a UUID.
But if you've designed your NoSQL database badly, or your input data isn't well sanitized, you might find yourself in a situation where you can't guarantee uniqueness without validating every row. That's a bad place to be, but it's probably how the code Remco found started its life.
The purpose of this Java code is to query all the customer IDs from a database and ensure that they're fully unique.
private Completable validateUniqueCustomerIds(List rootContainers) {
if (!validateUniqueIds) {
log.trace("validateUniqueCustomerIds validateUniqueIds == false -> skipping validation");
return Completable.complete();
}
// use Flowable.share() to make sure we only call the repository once.
final Flowable nonEmptyCustomerIds = someMongoRepository.getCustomerIds()
.filter(StringUtils::isNotEmpty).share();
final Set uniqueCustomerIds = nonEmptyCustomerIds.distinct().collectInto(new HashSet(), Set::add).blockingGet();
final Set allCustomerIds = nonEmptyCustomerIds.collectInto(new HashSet(), Set::add).blockingGet();
final Set duplicateCustomerIds = allCustomerIds.stream()
.filter(id -> !uniqueCustomerIds.contains(id))
.collect(toSet());
if(uniqueCustomerIds.isEmpty()) {
log.trace("validateUniqueCustomerIds uniqueCustomerIds.isEmpty(): true, returning Completable.complete()");
return Completable.complete();
}
if(!duplicateCustomerIds.isEmpty()) {
log.trace("validateUniqueCustomerIds duplicateCustomerIds.isEmpty(): false, returning Completable.error...");
return Completable.error(new IllegalStateException("The following Customer IDs are non-unique: " + duplicateCustomerIds));
}
String version = rootContainers.get(0).getVersion();
return checkExistingCustomerIdsInOtherPlaces(rootContainers, version, uniqueCustomerIds)
.doOnComplete(() -> log.trace("validateUniqueCustomerIds checkExistingCustomerIdsInOtherPlaces complete"))
.doOnError(throwable -> log.error("validateUniqueCustomerIds checkExistingCustomerIdsInOtherPlaces error: ", throwable))
;
}
So, probably reasonably, we see that a variable (controlled somewhere else) enables or disables this method- skipping this validation seems like a thing you want. I'm not sure it's the best way, but we can let that slide.
Next, we fetch all of the non-empty customer IDs. Then we collect the uniqueCustomerIds
by calling distinct
and putting them in a Set
.
Then we collect allCustomerIds
by putting them in a Set
without calling distinct
…
Then we look at every entry in allCustomerIds
and filter out the ones that are in uniqueCustomerIds
, and absolutely none of that makes any sense.
First, the distinct
call is unnecessary since we're collecting into Set
s, which are by definition, only going to hold unique entries. uniqueCustomerIds
and allCustomerIds
are going to be the same sets. But even assuming that they're different, the filter
makes no sense and isn't even the right operation. They attempted to do an intersection of two sets and failed.
The end result is a function that takes the wrong approach to solving a problem that itself was caused by taking the wrong approach.
Remco writes:
I've found this snippet in our codebase and it's been in there since 2020, about 2 years before I joined this team. I wonder how this got past code review?
Метки: CodeSOD |
CodeSOD: A Tip |
David was poking around in some code for a visualization library his team uses. It's a pretty potent tool, with good code quality. While skimming the code, though, David found this mismatched comment and code:
def get_tip(self):
# Returns the position of the seventh point in the path, which is the tip.
if config["renderer"] == "opengl":
return self.points[34]
return self.points[28] # = 7*4
Now, I'm going to set aside the issues of stringly typed fields (at least use a constant or an enum), or magic numbers (again, the constant). What I want to highlight is what David saw: neither 34 nor 28 are seven. It's inaccurate and misleading to refer to either of those as the seventh point in the path. If nothing else, it's confusing. But I don't think it's wrong, or at least, the wrong-ness isn't actually in this code.
Without broader context, I don't know how paths get represented in this code. But what I can see is that, in the OpenGL branch, 34
is actually the 35th element in a zero-based array. 35 is divisible by 7, so it's actually reasonable to believe that the entire path is constructed out of segments of 5 points and the seventh group represents the tip. We can make a similar assumption about 28- which while it's actually the 29th entry, we can imagine a world where we break the path up into groups of four, and for some reason the first point needs to have special meaning. A lot of graphics libraries require the first point in a curved path to be a non-curved vertex, for example, so if this path represents a curve, perhaps the code simply repeats the first point in a curved/non-curved form.
All of that would be useful information to see in a comment. It still leaves us with an extra puzzle: why is the tip the seventh point? Is that just their lucky number?
Метки: CodeSOD |
CodeSOD: Around 20 Meg |
Michael was assigned a short, investigatory ticket. You see, their PHP application allowed file uploads. They had a rule: the files should never be larger than 20MB. But someone had uploaded files which were larger. Not much larger, but larger. Michael was tasked with figuring out what was wrong.
Given that the error was less than half a megabyte, Michael had a pretty good guess about why this was.
if (round($uploadedFile->getSize() / 1024 / 1024) > 20) {
[ ... throw some error message ]
}
The developer's instincts weren't entirely bad. Take the number of bytes, divide by 1024
twice to get it down to megabytes, and then compare against twenty. It's probably not how I'd write it, but it's not wrong- at least not until you start rounding the number off.
Why was the developer rounding in the first place?
"Because 20 is an integer, and I wanted to compare integers. So I rounded. PHP doesn't have a built in trunc
method."
Pedantically true, as there's nothing called trunc
or truncate
in PHP, but it does have a floor
and an intval
method, both of which discard decimal digits (but behave slightly differently). In this case, either one would have worked.
Метки: CodeSOD |
Error'd: Movement Activated |
England and the United States, according to the old witticism, are two countries separated by a common language. The first sample deposited in our inbox by Philip B. this week probably demonstrates the aphorism. "I'm all in favor of high-tech solutions but what happens if I only want (ahem) a Number One?" he asked. I read, and read again, and couldn't find the slightest thing funny about it. Then I realized that it must be a Brit thing.
An anonymous reader shared this with us, asking "Shouldn't it be a 4xx error code?" Poe's law makes me reluctant to explain that not all software is a web site, for fear there's an anonymous tongue planted deep within a giant cheek.
Contrarian Amy K. querulously questioned "To Google, does False mean true?"
Brave Bartek Horn confirmed "Testing is important. I'm not sure if they are brilliant or hypocrites." Both is always possible.
Finally, another anonymous poster from the land of the frei thinks this discount might be too gut to be wirklich. "That doesn't seem right," they understated.
Метки: Error'd |