The Boulder Factory |
Like a lot of HR systems, the one at Initech had grown into a complicated mess of special cases, edge cases, and business rules that couldn't be explained but had to be followed.
Mark was assigned to a project to manage another one of those special cases: Initech had just sold one of its factories. Their HR system needed to retain information about the factory and its employees up until the point of the sale, but it also needed to be disconnected from some future processing- they certainly didn't want to send anybody any paychecks, for example. But not all processing. If an employee had started a health insurance claim before the factory was sold, they needed to keep that active in the system until it was completed (but also not allow the employee to file new claims).
It was going to be a lot of special processing, so Mark made a simple suggestion: "Why don't we add a 'sold' checkbox, or a 'decommissioned' flag, or something like that? We add that as a data-field to a factory, and then we know all employees associated with that factory go down a different processing path."
"Oh, we can't do that," Mark's boss, Harlan, countered. "It would be a new database field, changes to the factory edit screen, we'd have to document it for the users, probably add an 'are you sure' confirmation dialog, it's just too much work to do that and then also add all the special processing rules."
It was okay, though, because Harlan had a simpler solution. Just do the special processing rules. IF factory_id == 27 THEN doTheSoldFactoryStuff() ELSE doTheRegularFactoryStuff()
. No changes to the database, no changes to any screens, they just had to go through thousands of lines of code, scattered across hundreds of different modules and individual programs, and jam that special branch in there, in the right spot.
"Right," Mark cautioned, "but the next time we sell a factory, we'll have to do this all over again. Whereas if we add the checkbox-"
"How often do you think we're going to be selling factories? It's fine," Harlan said.
The next six months were the tedious process of going through all the places in the software where the special branch needed to go. Of course, no one was precisely documenting this, no one was really concerning themselves with any minutae like a "clean commit history": patch the code, maybe add a comment, and move on with your day. And it's not the case that every place they were changing the code fit exactly that pattern of IF factory_id == 27
; not every system used the same naming conventions, or even the same language.
It was a rough six months, but at the end of it, the factory was sold, the HR systems processed everything correctly, and management was happy with the end result. There was just one more thing…
"Welp," Harlan said as he called everyone in for the new project kickoff. "We've sold another factory, and I have a plan for how we're going to make that change, without needing to add any database fields or modify any UI elements."
As Camus said, "One must imagine Sisyphus happy," but Mark was significantly less happy. If Harlan had taken his input, this wouldn't be an IT task at all. As it was, Mark had a good sense of what the next six months of work was going to look like.
Метки: Feature Articles |
And FORTRAN, FORTRAN So Far Away |
A surprising amount of the world runs on FORTRAN. That's not to say that huge quantities of new FORTRAN are getting written, though it's far from a dead language, but that there are vital libraries written fifty years ago that are still used to this day.
But the world in which that FORTRAN was written and the world in which we live today is wildly different. Which brings us to the story of George and Ike.
In the late 1960s, the company that Ike worked for got a brand-spanking new CDC 6600 mainframe. At the time, it was the fastest computer you could purchase, with a blistering 3MFLOPS performance- 3 million floating point operations per second. The company wanted to hand this off to their developers to do all sorts of fancy numerical simulations with FORTRAN, but there was just one problem: they wanted to do a lot of new programs, and the vendor-supplied compiler took a sadly long time to do its work. As they were internally billing CPU time at $0.10/second, teams were finding it quite expensive to do their work.
By Jitze Couperus - Link
Enter Ike. Ike was a genius. Ike saw this problem, and then saw a solution. That solution was 700,000 lines of CDC 6600 assembly language which was his own, custom, FORTRAN compiler. It used half as much memory, ran many times faster, and could generate amazingly user-friendly error messages. Ike's compiler became their internal standard.
Time passed, and Ike moved on to different things at the company. A new team, including George, was brought in, and they were given a "simple" task: update this internal compiler into the coding standards of the late 1970s.
A lot had changed in the decade or so since Ike had released his compiler. First was the rather shocking innovation of terminals which could display lower-case characters. The original character width of the CDC 6600 was 6-bits, but the lower-case codes were sneaked in as 12-bit characters prefixed with an escape code. In mainstream FORTRAN releases, the addition of lower-case characters was marked with a name change: after FORTRAN77, all future versions of the language would simply go by "Fortran".
George dug through the character handling in the assembly code, and found a recurring line: BCDBIT EQU 6
. This was part of every code segment which handled text. This was a handy flag for George and his team: every place they needed to change had it. The change wasn't simple, though, as they had to do some munging with changing shift counts and changing character masks, adding in some logic for escape codes. In principle, this was absolutely an achievable task for any given BCDBIT EQU 6
line.
In practice, there were 122,000 occurrences of that line in the code. The team would be hard-pressed to do even 1% of that in the time they had allotted- and so 1% is what they committed to do. About 1,200 instances in the assembly would be updated to allow escape characters, covering most of the cases the users wanted to be able to handle wide characters in. It left a lot of code paths where bad results might happen, but that could be handled with the old "caveat programmator".
There were other, similar issues with handling text. For example, any code which read data from a file or input was capped at reading 73 or 80 characters at a time- the screen width of the terminals when Ike had been designing the code. That was an easy fix, but introduced George to another… quirk of Ike's design.
You see, it wasn't enough for Ike to write his own compiler. Because the compiler wasn't just a compiler: it was also a linker. An extremely bad and fragile linker, but it would combine your program's compiled binary with its dependencies in a way that mostly worked. But it also wasn't just a linker. Because Ike's compiler/linker was also a runtime, which would allow you to run your code in a self-contained environment.
Ostensibly, this was meant for testing. For example, that runtime wouldn't let your program access the disks, but it would pretend to. Lines like DSKDELAY MSECDELAY 33
were scattered through the runtime portion of the code. This would simulate the delay you could expect from accessing disks. And, in a full block, often looked like:
DSKDELAY MSECDELAY 33
SA1 DISKMSEC
SX6 X1+33
SA6 A1
This code is incrementing a count. At the end of the run it would output an estimate of how much time you spent doing disk I/O. There was just one problem with that: the estimate was absolutely useless. The hardware configuration had so much impact- whether your I/O passed through the $3M memory cache, or had to go out to one of the old-style barrel-sized hard-drives. Disk I/O operations could take nanoseconds or could take entire seconds. Ike's "helpful" estimates weren't.
So, Ike's genius may have been a little misguided, sometimes. There was one other quirk he had left for George to discover. This compiler was 700,000 lines of code. Assembling that code into an executable took time- specifically 330 seconds. At ten cents per second, that's $33 per compile. This was the late 70s- that was more than George's daily salary. Ike was under pressure to find ways to optimize the assembling of this code, and as established, Ike was a genius.
The Assembler did provide an IF
pseudo-op, allowing conditional assembly, in the same way the C preprocessor allows you to do conditional compilation. But this was expensive at assembly time. If Ike used IF
s, the assembling would have taken even longer than 330 seconds. So Ike found a trick.
BCDBIT EQU 6 YEAH, RIGHT
READCARD READS INFILE,LINE,73 FOR EVER AND EVER, AMEN
SA1 OPT A1+B1 SAVED A THOUSANDTH OF A BUDGET PENNY
DSKDELAY MSECDELAY 33 BECASE WE ARE AN OS, ALSO, TOO
Now, as you might guess from looking at this code, it's constructed as columns. The rightmost column is clearly comments. In fact, this Assembly dialect reserves three columns for operations. After the third time it encounters spaces, it treats everything from that point forward as a comment.
Which now, saying that, you should probably find this line a bit more suspicious:
SA1 OPT A1+B1 SAVED A THOUSANDTH OF A BUDGET PENNY
Everything after the third run of spaces means that A1+B1
would be a comment- except that the OPT
symbol gets expanded at assembly time. Which means if it has a value, the A1+B1
operation is ignored, but if it has a value, that value is used here.
In George's words:
That is, if OPT evaluated to anything, then OPT was the operand, otherwise if OPT was all blanks, then A1+B1 was the operand. This could be extended as many times across as you'd like, leading to eye-watering code, nearly impossible to understand. But it did greatly speed up assembly time, so a big win.
Ike still worked at the company, so George was able to go over to his new office and ask questions. Unfortunately, that turned out to be worse than useless. Ike always had an answer ready for George, but that answer was always wrong. Whether Ike didn't understand his old code, had simply forgotten how it worked, or was just having a laugh at George's expense was a question George could never answer.
But there were questions George could answer, by the end of the project. As George explains:
While rather frustrating, I did eventually slide the compiler into the late 1970's, and we got a good ten years more of use out of it. So, a success story?
In the end, there's no WTF here, just a story about working within constraints we don't think about often.
https://thedailywtf.com/articles/and-fortran-fortran-so-far-away
Метки: Feature Articles |
CodeSOD: Golfing Over a Log |
Indirection is an important part of programming. Wrapping even core language components in your own interfaces is sometimes justifiable, depending upon the use cases.
But like anything else, it can leave you scratching your head. Sam found this bit of indirection in a NodeJS application:
var g = {};
g.log = console.log;
g.print = g.util.print;
g.inspect = g.util.inspect;
g.l = g.log;
g.i = g.inspect;
g.ll = function(val) {
g.l(g.i(val));
}
The intent, clearly, is to play a little code golf. g.ll(something)
will dump a detailed report about an object to the console. I mean, that's the goal, anyway. Of course, that makes the whole thing less clear, but that's not the WTF.
The rather obvious problem is that this code just doesn't work. g.util
doesn't exist, so quite a few of these lines throw errors. They clearly meant to reference the Node module util
, which has inspect
and print
methods. They just slapped a g.
on the front because they clearly weren't thinking, or meant to capture it, like g.util = require('util')
or similar.
This module is meant to provide a bunch of logging functionality, and it has many many more lines. The only method ever used, from this snippet, is g.l
, so if not for the fact that this errors out on the third line, most of the rest of the module would probably work.
Fortunately, despite being in the code base, and despite once having been referenced by other modules in the project, this module isn't actually used anywhere. Of course, it was still sitting there, still announcing itself as a logging module, and lying in wait for some poor programmer to think they were supposed to use it.
Sam has cleaned up the code and removed this module entirely. Who knows what else lurks in there, broken and seemingly unused?
Метки: CodeSOD |
CodeSOD: Terned Around About Nullables |
John H works with some industrial devices. After a recent upgrade at the falicity, the new control software just felt like it was packed with WTFs. Fortunately, John was able to get at the C# source code for these devices, which lets us see some of the logic used…
public bool SetCrossConveyorDoor(CrossConveyorDoorInfo ccdi, bool setOpen)
{
if (!ccdi.PowerBoxId.HasValue)
return false;
ulong? powerBoxId = ccdi.PowerBoxId;
ulong pbid;
ulong ccId;
ulong rowId;
ulong targetIdx;
PBCrossConveyorConfiguration.ExtractIdsFromPowerboxId(powerBoxId.Value, out pbid, out ccId, out rowId, out targetIdx);
TextWriter textWriter = Console.Out;
object[] objArray1 = new object[8];
objArray1[0] = (object) pbid;
objArray1[1] = (object) ccId;
objArray1[2] = (object) setOpen;
object[] objArray2 = objArray1;
powerBoxId = ccdi.PowerBoxId;
ulong local = powerBoxId.Value;
objArray2[3] = (object) local;
objArray1[4] = (object) pbid;
objArray1[5] = (object) ccId;
objArray1[6] = (object) rowId;
objArray1[7] = (object) targetIdx;
object[] objArray3 = objArray1;
textWriter.WriteLine(
"Sending CCD command to pbid = {0}, ccdId = {1}, Open={2}, orig PowerBoxId: {3} - divided:{4}/{5}/{6}/{7}",
objArray3);
bool? nullable1 = this.CopyDeviceToRegisters((int) (ushort) ccId);
if ((!nullable1.GetValueOrDefault() ? 1 : (!nullable1.HasValue ? 1 : 0)) != 0)
return false;
byte? nullable2 = this.ReadDeviceRegister(19, "CrossConvDoor");
byte num = nullable2.HasValue ? nullable2.GetValueOrDefault() : (byte) 0;
byte registerValue = setOpen ? (byte) ((int) num & -225 | 1 << (int) targetIdx) : (byte) ((int) num & -225 | 16);
Console.Out.WriteLine("ccdid = {0} targetIdx = {1}, b={2:X2}", (object) ccId, (object) targetIdx, (object) registerValue);
this.WriteDeviceRegister(19, registerValue, "CrossConvDoor");
nullable1 = this.CopyRegistersToDevice();
return nullable1.GetValueOrDefault() && nullable1.HasValue;
}
There's a bunch in here, but I'm going to start at the very bottom:
return nullable1.GetValueOrDefault() && nullable1.HasValue
GetValueOrDefault
, as the name implies, returns the value of the object, or if that object is null, it returns a suitable default value. Now, for any referenece type, that can still be null. But nullable1
is a boolean (defaults to false
), and nullable2
is a byte
(defaults to zero).
This line alone makes one suspect that the developer doesn't really understand how nullables work. And, as we read up the code, we see more evidence of this:
byte num = nullable2.HasValue ? nullable2.GetValueOrDefault() : (byte) 0;
Again, if nullable2
has a value, GetValueOrDefault
will return that value, if it doesn't, it returns zero. So we've just taken a simple thing and made it less readable by surrounding it with a bunch of noise which doesn't change its behavior.
But, continuing reading backwards:
if ((!nullable1.GetValueOrDefault() ? 1 : (!nullable1.HasValue ? 1 : 0)) != 0)
return false;
We've moved into nested ternaries inside an if. Which, if we try and parse through this one: if the nullable's value is false, 1 != 0
, so we return false
. If, on the other hand, the nullable's value is true, we check to see if it doesn't have a value, in which case we compare 1 != 0
and return false. Except the only way nullable1
could ever be true is if it has a value, so that means if nullable1
is true, we don't return false.
In other words, this is a really complicated way of saying:
if (!nullable1.GetValueOrDefault()) return false;
With all that out of the way, it brings us to the block of objArray
s. The core purpose of this block is to populate what appears to be logging output. Now, the WriteLine
method does take an object[]
parameter to drive that formatting… but it's a param-array, which means you could invoke it as: Console.Out.WriteLine("…", pbid, ccId, setOpen…)
. I'm not 100% certain when params
appeared in C#, and a cursory searching implies that it's always been a language feature. Still, I'll give the developer responsible the benefit of the doubt on just using the object[]
, because of how they used it.
They start with objArray1
, and populate three fields. Then they create objArray2
which is just a reference to objArray1
. They populate the fourth field through objArray2
, then go back to using objArray1
. Then they create objArray3
which is also just referencing objArray1
, and send that to WriteLine
.
Maybe the goal was some form of intentional obfuscation? Were they just… confused? It's impossible to guess.
So instead of guessing, I'll just share another snippet of code from the same program, which I think sums up my feelings:
private static void GenPwd(string[] args)
{
if (args[1].Contains("!"))
Console.Out.WriteLine("Use password without tilde (~) please.");
…
}
https://thedailywtf.com/articles/terned-around-about-nullables
Метки: CodeSOD |
Error'd: ;pam ;pam ;pam ;pam |
One of this week's entries is the type that drives me buggy. Guess which one.
Regular contributor Pascal splains this shopping saga: "Amazon now requires anti-virus software to have an EPA Registration number."
Survey subject Stephen Crocker poses his own research question. "Do they mean click 'Continue' to continue or click 'Continue' to next?" We may never know.
Cartomanic
Mike S.
thought he'd found a strange new land but it's just the country
formerly known as B*****m, rebranding.
"Usually I keep the live downlink TV from the International Space Station,
and generally familiar with most of the countries it goes over but this
is a new one by me."
An anonymous email address starting with r2d2 bleeped "This website was clearly written specifically for self-loathing bots." Yes, Marvin, we see you. Come in.
For our final number, singer Peter G. sounds off. "Great, just great. I ordered a graphic equaliser and instead they've sent me an amp amp amp amp amp."
Метки: Error'd |
CodeSOD: A Dash of SQL |
As developers, we often have to engage with management who doesn't have a clue what it is we do, or how. Even if that manager was technical once, their technical background is frequently out of date, and their spirit has been sapped by the endless meetings and politics that being a manager entails. And it's often these managers who have some degree of control over where our career is going to progress, so we need to make them happy.
Which means… LEVEL UP YOUR CAREER WITH THIS ONE SIMPLE TRICK!. You need to make managers happy, and if there's one thing that makes managers happy, it's dashboards. Take something complicated and multivariate, and boil it down to a simple system. Traffic lights are always a favorite: green is good, red is bad, yellow is also bad.
It sounds stupid, because it is, but one of the applications that got me the most accolades was a dashboard application. It was an absolute trainwreck of code that slurped data from a dozen different silos and munged it together via a process the customer was always tweaking, and turned the complicated mathematics of how much wasteage there was in an industrial process into a simple traffic light icon. Upper managers used it and loved it, because that little glowing green light gave them all the security they needed, and when one of those lights went yellow or worse, red, they could swoop in and do management until the light turned green again.
Well, Kaspar also supports a dashboard application. It also slurps giant piles of data from a variety of sources, and it tries to turn some key metrics into simple letter grades- "A" through "E".
This particular query is about 400 lines of subqueries connected via LEFT JOIN
. The whole thing is messy in the way that only giant SQL queries that are trying to restructure and reshape data in extreme ways can be. That's not truly a WTF, but several of these subqueries do something… special.
(select
Rating_mangler =
case
WHEN VALUE = '' THEN ''
WHEN a_id in (SELECT actor.id
FROM actor, f_cache, f_math
WHERE f_cache.a_id=actor.id AND f_math.name IN ('L43A0', 'L43A1', 'L43A2A3', 'L33OEKO') AND f_cache.formula_id=f_math.id
AND filter = '' AND treaarsregle=1 AND e_count_value=0) THEN ''
WHEN VALUE < '1.9999999999' THEN 'E'
WHEN VALUE >= '2' and VALUE< '2.99999999' THEN 'D'
WHEN VALUE >='3' and VALUE < '3.999999999' THEN 'C'
WHEN VALUE >='4' and VALUE < '4.999999999' THEN 'B'
ELSE 'A'
END, a_id
From f_cache, f_math
where formula_id=f_math.id
and f_math.name in ('L_V_mangler_p')
and filter = '' and treAarsregle=1 and pricetype=2 and e_count_hp=0) as Rating_mangler
Specifically, I want to highlight the chain of WHEN
clauses in that case. We're translating ranges into letter grades, but those ranges are stored as text. We're doing range queries on on text: WHEN VALUE >= '2' and VALUE< '2.99999999' THEN 'D'
.
Now, this has some interesting effects. First, if the VALUE
is "20", that's a "D". A value of "100" is going to be an "E". And since it's text, "WTF" is also going to be an "A".
We can hope that input validation at least keeps most of those values out. But this pattern repeats. There are other subqueries in this whole chain. Like:
(select
Rating_Arbejdsulykker =
case
WHEN VALUE = '' THEN ''
WHEN VALUE < '1.9999999999' THEN 'E'
WHEN VALUE >= '2' and VALUE< '2.99999999' THEN 'D'
WHEN VALUE >='3' and VALUE < '3.999999999' THEN 'C'
WHEN VALUE >='4' and VALUE < '4.999999999' THEN 'B'
ELSE 'A'
END, a_id
From f_cache, f_math
where formula_id=f_math.id
and f_math.name in ('L_V_ulykker_p')
and filter = '' and treAarsregle=1 and pricetype=2 and e_count_hp=0) as Rating_Arbejdsulykker
And yet again, but for bonus points, we do it using a totally different way of describing the range:
(select
Rating_kundetilfredshed =
case
WHEN a_id in (SELECT actor.id
FROM actor, f_cache, f_math
WHERE f_cache.a_id=actor.id AND f_math.name IN ('L153', 'L153LOYAL') AND f_cache.formula_id=f_math.id
AND filter = '' AND treaarsregle=1 AND e_count_value=0) THEN ''
WHEN VALUE = '' THEN ''
WHEN VALUE = '1' THEN 'E'
WHEN VALUE >= '1.000001' and VALUE<= '2.00001' THEN 'D'
WHEN VALUE >='2.00001' and VALUE <= '3.00001' THEN 'C'
WHEN VALUE >'3.00001' and VALUE <= '4.00001' THEN 'B'
ELSE 'A'
END, a_id
From f_cache, f_math
where formula_id=f_math.id
and f_math.name in ('L153_AVG')
and filter = '' and treAarsregle=1 and pricetype=2 and e_count_hp=0) as Rating_kundetilfredshed
Unlike the others, this one would score values less than "1" as an "A". Which who knows, maybe values less than one are prevented by input validation. Of course, if they stored numbers as numbers then we could compare them as numbers, and all of this would work correctly without having to take it on faith that the data in the database is good.
Метки: CodeSOD |
Some Version of a Process |
When you're a large company, like Oracle, you can force your customers to do things your way. "Because we said so," is something a company like that can get away with. Conversely, a small company is more restricted- you have to work hard to keep your customers happy.
When Doreen joined Initech, they were a small company with a long history and not too many customers. In the interests of keeping those customers happy, each customer got their own custom build of the software, with features tailored to their specific needs. So, Initrode was on "INITRODE.9.1", while the Soggy Beans coffee shop chain was on "SOGGY.5.2". Managing those versions was a pain, but it was Doreen's boss, Elliot, who ensured that pain escalated to anguish.
Elliot was the one who laid out their software development and source control processes. It was the Official Process™, and Elliot was the owner of the Official Process™. The Official Process™ was the most elegant solution Elliot could imagine: each version lived in its own independent Subversion repository. Changes were synced between those repositories manually. Releases were also manual, and rare. Automated testing was non-existent..
Upper management may not have understood the problems that created, but they knew that their organization was slow to release new features, and that customers were getting frustrated with poor response times to bugs and feature requests. So they went to the list of buzzwords and started pushing for "Agile" and "DevOps" and "Continuous Delivery".
Suddenly, Doreen and the other developers were given a voice. They pushed to adopt Git, over Subversion. "I've looked into this," Elliot said, "and it looks like Git uses GitHub and stores our code off-site. I don't trust things that are off-site. I want our code stored here!"
"No, you don't have to use GitHub," Doreen explained. "We can host our own server- I've been playing around with GitLab, which I think will fit our needs well."
Elliot grumbled and wandered off.
Doreen took a few hours to configure up a GitLab instance, and migrate their many versions of the same code into something approaching a sane branching structure. It'd be a lot of work before the history actually made any sense, but it allowed her to show off some of the benefits, like that it would build and run the handful of unit tests she whipped up on commits to certain branches.
"That's fine," Elliot said, "but where's the code?"
"What… do you mean? It's right here."
"That's the code for Soggy Beans, where's the Initrode version?" Elliot demanded.
Doreen switched branches. "Right here."
"But where did the Soggy Beans version go?!" Elliot was getting angry.
"I… don't understand? It's stored in Git. We're just changing branches."
"I don't like this magical nonsense. I want to see our code in folders, as files, not this invisible shapeshifting stuff! I don't want our code where I can't see it!"
Doreen attempted to explain what branches were, about how Git stored files and tracked versions, but Elliot was already storming off to raise hell with the one upper manager who still listened to him. And a few days later, Elliot came back with a plan.
"So, since we're migrating to Git," Elliot explained to the team, "that poses a few challenges, in terms of the features it lacks. So I've written a script that will supplement it."
The script in question enumerated all the branches and tags in the repository, checked each one out in turn then copied it to another folder. "Once you've run this, you can navigate to the correct folder and make your changes there. If you need to make changes that impact multiple customers, you can repat those changes on each folder. Then you can run this second script, which will copy the changed folders back to the repository and commit it." This was also how code would be deployed: explode the repository out into folders, and copy the appropriate folder to the server.
At first, Doreen figured she could just ignore the script and do things the correct way. But there were a few problems with that. First, Elliot's script created commits that made it look like every file had been changed on every commit, making history meaningless. Second, it required you to be very precise about which branches/versions you were working on, and it was easy to make a mistake and commit changes from one branch into another, which was a mistake Elliot made frequently. He blamed Git for this, obviously.
But third, and most significantly: Elliot's script wasn't a suggestion. It was the Official Process™, and every developer was required to use it. Oh, you could try and "cheat", but your commits would be clean, clear, and comprehensible, which was a dead giveaway that you weren't following the Official Process™.
Doreen left the company a short time later. As far as anyone knows, Elliot still uses his Official Process™.
Метки: Feature Articles |
CodeSOD: Globalism |
When Daniel was young, he took one of those adventure trips that included a multi-day hike through a rainforest. At the time, it was one of the most difficult and laborious experiences he'd ever had.
Then he inherited an antique PHP 5.3 application, written by someone who names variables like they're spreadsheet columns: $ag
, $ah
, and $az
are all variables which show up. Half of those are globals. The application is "modularized" into many, many PHP files, but this ends up creating include chains tens of files deep, which makes it nigh impossible to actually understand.
But then there are lines like this one:
drdtoarr() { global $arr; return $arr; }
This function uses a global $arr
variable and… returns it. That's it, that's the function. This function is used everywhere, especially the variable $arr
, which is one of the most popular globals in the application. There is no indication anywhere in the code about what drd
stands for, what it's supposed to mean, or why it sometimes maybe is stored in $arr
.
While this function seems useless, I'd argue that it has a vague, if limited point. $arr
is a global variable that might be storing wildly different things during the lifecycle of the application. drdtoarr
at least tells us that we expect to see drd
in there.
Now, if only something would tell us what drd
actually means, we'd be on our way.
Метки: CodeSOD |
CodeSOD: Expiration Dates |
Last week, we saw some possibly ancient Pascal code. Leilani sends us some more… modern Pascal to look at today.
This block of code comes from a developer who has… some quirks. For example, they have a very command-line oriented approach to design. This means that, even when making a GUI application, they want convenient keyboard shortcuts. So, to close a dialog, you hit "CTRL+C", because who would ever use that keyboard shortcut for any other function at all? There's no reason a GUI would use "CTRL+C" for anything but closing windows.
But that's not the WTF.
procedure TReminderService.DeactivateExternalusers;
var
sTmp: String;
begin
// Main Site
if not dbcon.Connected then
dbcon.Connect;
if not trUpdate.Active then
trUpdate.StartTransaction;
qryUsersToDeactivate.Close;
sTmp := DateTimeToStr(Now);
sTmp := Copy(sTmp, 1, 10) + ' 00:00:00';
qryUsersToDeactivate.SQL.Text := 'Select ID, "NAME", ENABLED, STATUS, SITE, EXPIRATION '+
'from EXTERNAL_USERS ' +
'where ENABLED=1 and EXPIRATION<:EXPIRED';
qryUsersToDeactivate.ParamByName('EXPIRED').AsDateTime := StrToDateTime(sTmp);
qryUsersToDeactivate.Open;
while not qryUsersToDeactivate.Eof do
begin
qryUsersToDeactivate.Edit;
qryUsersToDeactivate.FieldByName('ENABLED').AsInteger := 0;
qryUsersToDeactivate.Post;
qryUsersToDeactivate.Next;
end;
if trUpdate.Active then
trUpdate.Commit;
// second Site
// same code which does the same in another database
end;
This code queries EXTERNAL_USERS
to find all the ENABLED
accounts which are past their EXPIRATION
date. It then loops across each row in the resulting cursor, updates the ENABLED
field to 0
, and then Post
s that change back to the database, which performs the appropriate UPDATE
. So much of this code could be replaced with a much simpler, and faster: UPDATE EXTERNAL_USERS SET ENABLED = 0 WHERE ENABLED = 1 AND EXPIRATION < CURRENT_DATE
.
But then we wouldn't have an excuse to do all sorts of string manipulation on dates to munge together the current date in a format which works for the database- except Leilani points out that the way this string munging actually happens means "that only works when the system uses the german date format." Looking at this code, I'm not entirely sure why that is, but I assume it's buried in those StrToDateTime
/DateTimeToStr
functions.
Given that they call qryUsersToDeactivate.Close
at the top, this implies that they don't close it when they're done, which tells us that this opens a cursor and just leaves it open for some undefined amount of time. It's possible that the intended "close at the end" was just elided by the submitter, but the fact that it might be open at the top tells us that even if they do close it, they don't close it reliably enough to know that it's closed at the start.
And finally, for someone who likes to break the "copy text" keyboard shortcut, this code repeats itself. While the details have been elided by the submitter // same code which does the same in another database
tells us all we need to know about what comes next.
Метки: CodeSOD |
Error'd: In Other Words |
We generally don't like to make fun of innocent misuses of a second language. Many of us struggle with their first. But sometimes we honestly can't tell which is first and which is zeroeth.
Whovian stombaker pontificates "Internationalization is hard. Sometimes, some translations are missing, some other times, there are strange concatenations due to language peculiarities. But here, we have everything wrong and no homogeneity in the issues."
Likewise Sean F. wonders "How similar?"
Mathematician Mark G. figures "I'm not sure that's how percent works, but thanks for the alert."
Job-hunter Antoinio has been whiteboarded before, but never quite like this. "I was applying at IBM. I must agree before continuing... To what?"
Experienced Edward explains "I've been a software engineer for 12 years, and still I have no idea how they accomplished this."
I'm sure Mark G. will agree: that about sums it up.
Метки: Error'd |
CodeSOD: Subbing for the Subcontractors |
Back in the mid-2000s, Maurice got less than tempting offer. A large US city had hired a major contracting firm, that contracting firm had subcontracted out the job, and those subcontractors let the project spiral completely out of control. The customer and the primary contracting firm wanted to hire new subcontractors to try and save the project.
As this was the mid-2000s, the project had started its life as a VB6 project. Then someone realized this was a terrible idea, and decided to make it a VB.Net project, without actually changing any of the already written code, though. That leads to code like this:
Private Function getDefaultPath(ByRef obj As Object, ByRef Categoryid As Integer) As String
Dim sQRY As String
Dim dtSysType As New DataTable
Dim iMPTaxYear As Integer
Dim lEffTaxYear As Long
Dim dtControl As New DataTable
Const sSDATNew As String = "NC"
getDefaultPath = False
sQRY = "select TAXBILLINGYEAR from t_controltable"
dtControl = goDB.GetDataTable("Control", sQRY)
iMPTaxYear = dtControl.Rows(0).Item("TAXBILLINGYEAR")
'iMPTaxYear = CShort(cmbTaxYear.Text)
If goCalendar.effTaxYearByTaxYear(iMPTaxYear, lEffTaxYear) Then
End If
sQRY = " "
sQRY = "select * from T_SysType where MTYPECODE = '" & sSDATNew & "'" & _
" and msystypecategoryid = " & Categoryid & " and meffstatus = 'A' and " & _
lEffTaxYear & " between mbegTaxYear and mendTaxYear"
dtSysType = goDB.GetDataTable("SysType", sQRY)
If dtSysType.Rows.Count > 0 Then
obj.Text = dtSysType.Rows(0).Item("MSYSTYPEVALUE1")
Else
obj.Text = ""
End If
getDefaultPath = True
End Function
Indentation as the original.
This function was the culmination of four years of effort on the part of the original subcontractor. The indentation is designed to make this difficult to read- wait, no. That would imply that the indentation was designed. This random collection of spaces makes the code hard to read, so let's get some big picture stuff.
It's called getDefaultpath
and returns a String
. That seems reasonable, so let's skip down to the return statement, which of course is done in its usual VB6 idiom, where we set the function name equal to the result: getDefaultPath = True
Oh… so it doesn't return the path. It returns "True"
. As a string.
Tracing through, we first query t_controltable
to populate iMPTaxYear
. Once we have that, we can do this delightful check:
If goCalendar.effTaxYearByTaxYear(iMPTaxYear, lEffTaxYear) Then
End If
Then we do some string concatenation to build a new query, and for a change, this is an example that doesn't really open up any SQL injection attacks. All the fields are either numerics or hard-coded constants. It's still awful, but at least it's not a gaping security hole.
That gets us a set of rows from the SysType
table, which we can then consume:
If dtSysType.Rows.Count > 0 Then
obj.Text = dtSysType.Rows(0).Item("MSYSTYPEVALUE1")
Else
obj.Text = ""
End If
This is our "return" line. You wouldn't know it from the function signature, but obj as Object
is actually a textbox. So this function runs a pair of queries against the database to populate a UI element directly with the result.
And this function is just one small example. Maurice adds:
There are 5,188 GOTO statements in 1321 code files. Error handling consists almost entirely of a messagebox, and nowhere did they use Option Strict or Option Explicit.
There's so much horror contained in those two sentences, right there. For those that don't know VisualBasic, Option Strict
and Option Explicit
are usually enabled by default. Strict
forces you to respect types- it won't do any late binding on types, it won't allow narrowing conversions between types. It would prohibit calling obj.Text =…
like we see in the example above. Explicit
requires you to declare variables before using them.
Now, if you're writing clean code in the first place, Option Strict
and Option Explicit
aren't truly required- a language like Python, for example, is neither strict no explicit. But a code base like this, without those flags? Madness.
Maurice finishes:
This is but one example from the system. Luckily for the city, what took the subcontractors 4 years to destroy only took us a few months to whip into shape.
https://thedailywtf.com/articles/subbing-for-the-subcontractors
Метки: CodeSOD |
CodeSOD: The Programmer's Motto and Other Comments |
We've got a lovely backlog of short snippets of code, and it's been a long time since our last smorgasbord, so let's take a look at some… choice cuts.
Let's open with a comment, found by Russell F:
//setting Text="" on the front end some how stoped a error on tftweb-02a on prealpha it may have also needed a new compiled version
//but after two + hours it doesnt work and i am not shure i acutal did anything
"After two+ hours, it doesn't work, and I'm not sure I actually did anything," describes the experience of being a programmer so well, that I honestly think it's my new motto. The key difference is that, if it doesn't work after two hours, you do have to keep going until it does.
From an Anonymous submitter, we have:
[Required(ErrorMessage = "This field is required."), ValidateMaxLength(Length = 10)]
[Range(typeof(bool), "false", "true", ErrorMessage = "Enter valid value.")]
public Nullable<bool> Nonbillable { get; set; }
Now, this is probably actually correct, because it's possible that the underlying data store might have invalid entries, so marking a Required
field as Nullable
probably makes sense. Then again, the chance of having invalid data in your datastore is a WTF, and apparently, it's a big problem for this API, as our submitter adds: "Looking at a very confused public-facing API - everything is like this."
"R3D3-1" was checking a recent release of Emacs, and found this function in python.el.gz
:
(defun python-hideshow-forward-sexp-function (arg)
"Python specific `forward-sexp' function for `hs-minor-mode'.
Argument ARG is ignored."
arg ; Shut up, byte compiler.
(python-nav-end-of-defun)
(unless (python-info-current-line-empty-p)
(backward-char)))
"Shut up, byte compiler". In this case, the programmer was trying to get an "unused parameter" warning to go away by using the parameter.
"R3D3-1" adds:
The comment made me chuckle a little, not a major WTF.
The correct solution in Emacs Lisp would have been to renamearg
to_arg
. This would be clear to not only the byte compiler, but also to other programmers.
And finally, a frustrated Cassi found this comment:
// TODO: handle this correctly
Cassi titled this "So TODO it already!" If you're writing code you know is incorrect, it might be a good time to stop and re-evaluate what you're doing. Though, Cassi goes on to add:
I suppose it could be argued, since I'm only coming across it now, that this comment was a good enough "solution" for the five years it's been sitting in the code.
Perhaps correctness isn't as important as we think.
https://thedailywtf.com/articles/the-programmer-s-motto-and-other-comments
Метки: CodeSOD |
CodeSOD: Wise About Bits |
The HP3000 was the first mini-computer that supported time-sharing. It launched in 1972, and HP didn't end-of-life it until 2010, and there are still third-party vendors supporting them.
Leonora's submission is some code she encountered a number of years ago, but not as many as you might think. It's in Pascal, and it's important to note that this version of Pascal definitely has bitwise operators. But, if you're programming on a 40 year old minicomputer, maybe you couldn't do an Internet search, and maybe Steve from down the hall had bogarted the one manual HP provided for the computer so you can't look it up because "he's using it for reference."
So you end up doing your best with no idea:
FUNCTION BITON(A , B : INTEGER): BOOLEAN;
VAR
C : INTEGER;
BEGIN
CASE A OF
15 : C:=1;
14 : C:=2;
13 : C:=4;
12 : C:=8;
11 : C:=16;
10 : C:=32;
9 : C:=64;
8 : C:=128;
7 : C:=256;
6 : C:=512;
5 : C:=1024;
4 : C:=2048;
3 : C:=4096;
2 : C:=8192;
1 : C:=16384;
0 : C:=32768;
OTHERWISE
BITON:=FALSE;
END;
IF ((B DIV C) MOD 2) = 1 THEN
BITON:=TRUE
ELSE
BITON:=FALSE;
END;
One thing I appreciate about Pascal code is that, even if you haven't touched the language since 1998 when you were the last class in your high school to learn Pascal instead of C++, it's incredibly readable. This method is very clear about what it does: it maps bits to powers of two, and then checks via division and modulus if that bit is set. It's, in some ways, the most obvious way to implement a bit check if you didn't know anything about bitwise operations in your language.
This BITON
function lays the foundation for an entire family of bitwise operations, re-implemented from scratch. You want to set bits individually? You already know how it works.
FUNCTION SETBITON(A, B : INTEGER) : INTEGER;
VAR
C : INTEGER;
BEGIN
CASE A OF
15 : C:=1;
14 : C:=2;
13 : C:=4;
12 : C:=8;
11 : C:=16;
10 : C:=32;
9 : C:=64;
8 : C:=128;
7 : C:=256;
6 : C:=512;
5 : C:=1024;
4 : C:=2048;
3 : C:=4096;
2 : C:=8192;
1 : C:=16384;
0 : C:=32768;
OTHERWISE
C:=0;
END;
IF NOT BITON(A,B) THEN
SETBITON:=B + C
ELSE
SETBITON:=B;
END;
FUNCTION SETBITOFF(A, B : INTEGER) : INTEGER;
VAR
C : INTEGER;
BEGIN
CASE A OF
15 : C:=1;
14 : C:=2;
13 : C:=4;
12 : C:=8;
11 : C:=16;
10 : C:=32;
9 : C:=64;
8 : C:=128;
7 : C:=256;
6 : C:=512;
5 : C:=1024;
4 : C:=2048;
3 : C:=4096;
2 : C:=8192;
1 : C:=16384;
0 : C:=32768;
OTHERWISE
C:=0;
END;
IF BITON(A,B) THEN
SETBITOFF:=B - C
ELSE
SETBITOFF:=B;
END;
The same pattern, complete with 16 bits hand-coded, and a check: if the bit is on, we add or subtract a value to set it off.
And, if you want an and
operator, why, that's just going to call those methods:
FUNCTION LAND(A,B : INTEGER) : INTEGER;
VAR
I : INTEGER;
BEGIN
I:=0;
REPEAT
IF BITON(I,A) THEN
IF BITON(I,B) THEN
A:=SETBITON(I,A)
ELSE
A:=SETBITOFF(I,A)
ELSE
A:=SETBITOFF(I,A);
I:=I + 1;
UNTIL I > 15;
LAND:=A;
END;
Again, I want to stress, while different Pascal compilers have different implementations of bitwise operations, this version absolutely had bitwise operations. As do most of them. The key difference is whether or not it supports C-style <<
/>>
operators, or uses a more Pascal-flavored shl
/shr
.
This developer obviously didn't know about them, and didn't know about the right way to do exponentiation either, which would make those giant case statements go away. Not as well as bitshifting, but still, away. But they clearly did their best- the code is readable, clear, and obvious in how it functions. Just, y'know, not as obvious as using the built-ins.
Метки: CodeSOD |
CodeSOD: A Coded Escape |
When evaluating a new development tool or framework, the first thing the smart developer does is check out the vendor documentation. Those docs will either be your best friend, or your worst enemy. A great product with bad documentation will provide nothing but frustration, but even a mediocre product with clean and clear documentation at least lets you get useful stuff done.
Stuart Longland's company has already picked the product, unfortunately, so Stuart's left combing through the documentation. This particular product exposes a web-socket based API, and thus includes JavaScript samples in its documentation. Obviously, one could use any language they like to talk to web-sockets, but examples are nice…
webSocket.onopen = function () {
let session_id = "SESSION-ID"; // Received from API create session request
let network_id = "NETWORK-ID"; // Received from API create session request
let reference = "REFERENCE-ID"; // Reference handle created by client to link messages to relevant callbacks
let wire = 1; // Connection ID, incremental value to identify messages of network/connection
let type = 1; // Client type, use value 1 (FRONTEND)
const OPEN = JSON.stringify({
"method": "open",
"id": network_id,
"session": session_id,
"ref": reference,
"wire": wire,
"type": type
});
this.send(decodeURIComponent(escape(OPEN)));
};
This all seems mostly reasonable until you get to the last line:
this.send(decodeURIComponent(escape(OPEN)));
escape
is a deprecated method similar to encodeURIComponent
. So this encodes our JSON string, then decodes it, then sends it over the web-socket. Which seems… like a useless step. And it probably is- this is probably a developer's brain-fart that happened to end up in the code-base, and then later on, ended up in the documentation.
But it might not be. Because escape
and encodeURIComponent
are not the same method. They don't escape characters the same way, because one of them is unicode compliant, and the other isn't.
escape('"a"o"u'); //"%E4%F6%FC"
encodeURIComponent('"a"o"u'); //"%C3%A4%C3%B6%C3%BC"
And that unicode awareness goes for the inverse method, too.
unescape(escape('"a"o"u')); //outputs ""a"o"u"
decodeURIComponent(encodeURIComponent('"a"o"u')); //outputs ""a"o"u"
unescape(encodeURIComponent('"a"o"u')); //outputs "~A¤~A¶~A 1/4 "
decodeURIComponent(escape('"a"o"u')); // throws a URIError exception
Now, it's unlikely that this particular JSON message contains any characters that would cause any problems- REFERENCE-ID
, SESSION-ID
and the others are probably just long hex-strings. So in real-world use, this probably would never cause an actual problem.
But in the situations where it does, this would create a surprising and probably annoying to debug glitch.
Character encodings are hard, but good documentation is even harder.
Метки: CodeSOD |
Error'd: Swordfish |
Despite literally predating paper, passcodes and secret handshakes continue to perplex programmers, actors, and artists alike.
For our first example, auteur
Andy
stages a spare play in three acts.
Nagg: Hello I'm not the account owner and shouldn't be logged in
to this account. Can you help me?
Nell: Sure, here are the owner's credit card details. Please use
those to say that you are the account owner.
Fellow arts enthusiast Paul shares some Surrealism "snatched from the official Greek government site for emergency communications."
Web comics fan Geoff G. alludes to a classic (we all know which one) with his "Now I have two problems..."
While an anonymous gourmand mumbles, mouth full, "Unicode Tofu is my secrets ingredient!"
And oldworlder Jan declares this bit of user experience needs no words. "Who needs a description when you have clear symbols?" It's like a watercolor about a novel.
Finally, Dima R. brings down the curtain with this little mind blower. Quoth he: "It's secured by shibboleths." Or encrapted by wishes.
Метки: Error'd |
CodeSOD: Going Through Some Changes |
Dave inherited a data management tool. It was an antique WinForms application that users would use to edit a whole pile of business specific data in one of those awkward "we implemented a tree structure on top of an RDBMS" patterns. As users made changes, their edits would get marked with a "pending" status, allowing them to be saved in the database and seen by other users, but with a clear "this isn't for real yet" signal.
One developer had a simple task: update a text box with the number of pending changes, and if it's non-zero, make the text red and boldfaced. This developer knew that some of these data access methods might not return any data, so they were very careful to "handle" exceptions.
int changes = 0;
try
{
changes = DataNode.DataNodeDataSet(Convert.ToInt32(Status.PendingNew)).Tables[0].Rows.Count;
}
finally{}
try
{
changes += DataVersion.GetVersionTable(Convert.ToInt32(Status.PendingNew)).Rows.Count;
}
finally{}
try
{
changes += DataOrderVersion.DataOrderVersionDataSet(Convert.ToInt32(Status.PendingNew)).Tables[0].Rows.Count;
}
finally{}
if (changes > 0)
{
Pending.Font.Bold = true;
Pending.ForeColor = System.Drawing.Color.Red;
}
Pending.Text = changes.ToString();
The indentation is this way in the original source.
This… works. Almost. They set the changes
variable to zero, then wrap all the attempts to access potentially null values in try
blocks. In lieu of having an empty catch, which I suspect their linter was set to complain about, they opted to have an empty finally
. Unfortunately, without that empty catch, the exception does get just tossed up the chain, meaning this doesn't work.
But even if it worked, I hate it.
And then there's the seemingly random indentation. Visual Studio automatically corrects the indentation for you! You have to work hard to get it messed up this badly!
In any case, I definitely have some pending changes for this code.
Метки: CodeSOD |
CodeSOD: Columns of a Constant Length |
Today's submitter goes by "[object Object]", which I appreciate the JavaScript gag even when they send us C code.
This particular bit of C code exists to help output data in fixed-width files. What you run into, with fixed-width files, is that it becomes very important to properly pad all your columns. It's not a difficult programming challenge, but it's also easy to make mistakes that cause hard-to-spot bugs. And given that most systems using fixed-width files are legacy systems with their own idiosyncrasies, things could easily go wrong.
Things go more wrong when you don't actually know the right way to pad strings. Thus this excerpt from constants.h
.
#define LABEL_SIZE1 1
#define LABEL_SIZE2 2
#define LABEL_SIZE3 3
#define LABEL_SIZE4 4
#define LABEL_SIZE5 5
#define LABEL_SIZE6 6
#define LABEL_SIZE7 7
#define LABEL_SIZE8 8
#define LABEL_SIZE9 9
#define LABEL_SIZE10 10
#define LABEL_SIZE11 11
#define LABEL_SIZE12 12
#define LABEL_SIZE14 14
#define LABEL_SIZE15 15
#define LABEL_SIZE16 16
#define LABEL_SIZE21 21
#define LABEL_SIZE20 20
#define LABEL_SIZE23 23
#define LABEL_SIZE24 24
#define LABEL_SIZE25 25
#define LABEL_SIZE30 30
#define LABEL_SIZE31 31
#define LABEL_SIZE32 32
#define LABEL_SIZE33 33
#define LABEL_SIZE37 37
#define LABEL_SIZE39 39
#define LABEL_SIZE40 40
#define LABEL_SIZE43 43
#define LABEL_SIZE45 45
#define LABEL_SIZE48 48
#define LABEL_SIZE50 50
#define LABEL_SIZE53 53
#define LABEL_SIZE73 73
#define LABEL_SIZE80 80
#define LABEL_SIZE121 121
#define LABEL_SIZE100 100
#define LABEL_SIZE149 149
#define LABEL_SIZE150 150
#define LABEL_SIZE170 170
#define LABEL_SIZE175 175
#define LABEL_SIZE205 205
#define LABEL_SIZE208 208
#define LABEL_SIZE723 723
#define LABEL_SIZE725 725
#define LABEL_SIZE753 753
#define LABEL_SIZE825 825
#define LABEL_SIZE853 853
#define SPACE_1 " "
#define SPACE_2 " "
#define SPACE_3 " "
#define SPACE_4 " "
#define SPACE_5 " "
#define SPACE_8 " "
#define SPACE_10 " "
#define SPACE_11 " "
#define SPACE_12 " "
#define SPACE_13 " "
#define SPACE_14 " "
#define SPACE_15 " "
#define SPACE_19 " "
#define SPACE_20 " "
#define SPACE_21 " "
#define SPACE_23 " "
#define SPACE_25 " "
#define SPACE_29 " "
#define SPACE_30 " "
#define SPACE_31 " "
#define SPACE_32 " "
#define SPACE_37 " "
#define SPACE_39 " "
#define SPACE_41 " "
#define SPACE_53 " "
#define SPACE_57 " "
#define ZERO_1 "0"
#define ZERO_2 "00"
#define ZERO_3 "000"
#define ZERO_4 "0000"
#define ZERO_5 "00000"
#define ZERO_8 "00000000"
#define ZERO_10 "0000000000"
#define ZERO_11 "00000000000"
#define ZERO_14 "00000000000000"
#define ZERO_29 "00000000000000000000000000000"
#define NINE_11 "99999999999"
#define NINE_29 "99999999999999999999999999999"
// Min max values
#define MIN_MINUS1 -1
#define MIN_0 0
#define MIN_1 1
#define MIN_111 111
#define MAX_0 0
#define MAX_1 1
#define MAX_2 2
#define MAX_3 3
#define MAX_4 4
#define MAX_7 7
#define MAX_9 9
#define MAX_12 12
#define MAX_15 15
#define MAX_63 63
#define MAX_99 99
#define MAX_114 114
#define MAX_127 127
#define MAX_255 255
#define MAX_256 256
#define MAX_999 999
#define MAX_1023 1023
#define MAX_4095 4095
#define MAX_9999 9999
#define MAX_16383 16383
#define MAX_65535 65535
#define MAX_9_5 99999
#define MAX_9_6 999999
#define MAX_9_7 9999999
#define MAX_9_8 99999999
#define MAX_9_9 999999999
#define MAX_9_10 9999999999
#define MAX_9_11 99999999999
The full file is 500 lines long, all in this pattern. It's also delightfully meta: these constants are used for generating fixed-width files, and this file is itself a fixed-width file, thanks to the developer wearing their tab key out.
https://thedailywtf.com/articles/columns-of-a-constant-length
Метки: CodeSOD |
CodeSOD: Insertion |
Shalonda inherited a C# GUI application that, if we're being charitable, has problems. It's slow, it's buggy, it crashes a lot. The users hate it, the developers hate it, but it's also one of the core applications that drives their business, so everyone needs to use it.
One thing the application needs to do is manage a list of icons. Each icon is an image, and based on user actions, a new icon might get inserted in the middle of the list. This is how that happens:
///
/// Inserts an object (should be an Image) at the specified index
///
/// the index where to add the object at
/// the object (an Image) to be added to this ImageCollection
public void Insert(int index, object val)
{
object[] icons = new object[this.list.Count];
object[] newicons = new object[this.list.Count + 1];
for (int i = 0; i < newicons.Length; i++)
{
if (i != index)
newicons[i] = icons[i];
else
newicons[i] = val;
}
this.list.Clear();
this.list.AddRange(newicons);
}
A comment like "Inserts an object (should be an Image)", in a strongly typed language, is always a bad sign. And sure enough, the signature of this method continues the bad signs: object val
. Insert
, in this case, is not a method defined in an interface, there's no reason for it to be val
here other than the fact that the original developer probably had a snippet for handling inserting into lists, and just dropped it in.
Then we create two new arrays, both of which are empty. Then we iterate across those empty arrays, copying emptiness from the first one into the second one, except when we're at the index
supplied by the caller, in which case we put our input value into the array.
Fortunately, at this point, our program is guaranteed to throw an exception, since icons
has one fewer index than newicons
, and they're both being indexed by i
, which goes up to newicons.Length
. This is actually the best case for this block of code, because of what comes next: it empties the list and then inserts the newicons
(which is all nulls except for one position) into the list.
List, in this case, is an ArrayList
, which definitely doesn't already have an insert method which actually does what it's supposed to.
Shalonda replaced this method with a call to the built-in Insert
. This stopped the exceptions from being thrown, but that in turn uncovered a whole new set of bugs that no one had seen yet. As the song goes: "99 little bugs in the tracker, 99 little bugs. Take one down, patch it around, 175 bugs in the tracker!"
Метки: CodeSOD |
Classic WTF: Crazy Like a Fox(Pro) |
It's Labor Day in the US. We're busy partaking in traditional celebrations, which depending on who you ask, is either enjoying one of the last nice long weekends before winter, or throwing bricks at Pinkertons. So we dig back into the archives, for a classic story about databases. Original --Remy
“Database portability” is one of the key things that modern data access frameworks try and ensure for your application. If you’re using an RDBMS, the same data access layer can hopefully work across any RDBMS. Of course, since every RDBMS has its own slightly different idiom of SQL, and since you might depend on stored procedures, triggers, or views, you’re often tied to a specific database vendor, and sometimes a version.
And really, for your enterprise applications, how often do you really change out your underlying database layer?
Well, for Eion Robb, it’s a pretty common occurrence. Their software, even their SaaS offering of it, allows their customers a great deal of flexibility in choosing a database. As a result, their PHP-based data access layer tries to abstract out the ugly details, they restrict themselves to a subset of SQL, and have a lot of late nights fighting through the surprising bugs.
The databases they support are the big ones- Oracle, SQL Server, MySQL, and FoxPro. Oh, there are others that Eion’s team supports, but it’s FoxPro that’s the big one. Visual FoxPro’s last version was released in 2004, and the last service pack it received was in 2007. Not many vendors support FoxPro, and that’s one of Eion’s company’s selling points to their customers.
The system worked, mostly. Until one day, when it absolutely didn’t. Their hosted SaaS offering crashed hard. So hard that the webserver spinlocked and nothing got logged. Eion had another late night, trying to trace through and figure out: which customer was causing the crash, and what were they doing?
Many hours of debugging and crying later, Eion tracked down the problem to some code which tracked sales or exchanges of product- transactions which might not have a price when they occur.
$query .= odbc_iif("SUM(price) = 0", 0, "SUM(priceact)/SUM(" . odbc_iif("price != 0", 1, 0) . ")") . " AS price_avg ";
odbc_iif
was one of their abstractions- an iif
function, aka a ternary. In this case, if the SUM(price)
isn’t zero, then divide the SUM(priceact)
by the number of non-zero prices in the price column. This ensures that there is at least one non-zero price entry. Then they can average out the actual price across all those non-zero price entries, ignoring all the “free” exchanges.
This line wasn’t failing all the time, which added to Eion’s frustration. It failed when two very specific things were true. The first factor was the database- it only failed in FoxPro. The second factor was the data- it only failed when the first product in the resultset had no entries with a price greater than zero.
Why? Well, we have to think about where FoxPro comes from. FoxPro’s design goal was to be a data-driven programming environment for non-programmers. Like a lot of those environments, it tries its best not to yell at you about types. In fact, if you’re feeding data into a table, you don’t even have to specify the type of the column- it will pick the “correct” type by looking at the first row.
So, look at the iif
again. If the SUM(price) = 0
we output 0 in our resultset. Guess what FoxPro decides the datatype must be? A single digit number. If the second row has an average price of, say, 9.99
, that’s not a single digit number, and FoxPro explodes and takes down everything else with it.
Eion needed to fix this in a way that didn’t break their “database agnostic” code, and thus would continue to work in FoxPro and all the other databases, with at least predictable errors (that don’t crash the whole system). In the moment, suffering through the emergency, Eion changed the code to this:
$query .= "SUM(priceact)/SUM(" . odbc_iif("price != 0", 1, 0) . ")") . " AS price_avg ";
Without the zero check, any products which had no sales would trigger a divide-by-zero error. This was a catchable, trappable error, even in FoxPro. Eion made the change in production, got the system back up and their customers happy, and then actually put the change in source control with a very apologetic commit message.
https://thedailywtf.com/articles/classic-wtf-crazy-like-a-fox-pro
Метки: Feature Articles |
Error'd: Just Doer It |
Testing in production again, here's five fails for the fifth day of the week. Or the sixth. Or is it the fourth?
Anonymous Ignoronymous declares "Dude! Science tests were never popular!"
While pronouncedly polyonymous Scott Christian Simmons proffers "How do doers forget to do their ONE JOB"
And probably pseudonymous Like H. explains it's because "More doing, less testing"
But querulous Quentin quibbles "Only shown in test mode. Oh well, what the hell! Nothing like testing the full user base. "
Finally, longtime reader, rare contributor Robin Z. shares: "While trying to see if tickets for UK Games Expo are available yet, I saw they had a shop section - but nothing to see there, apart from some good old testing in production."
Метки: Error'd |