CodeSOD: Not Exceptional |
One of the powers of structured exception handling is that it lets you define your own exception types. That's useful, as your code can communicate a lot of information about what's gone wrong when you use your own custom exceptions.
But sometimes, the custom exception type leaves us asking more questions. Christophe found this Java code from a "big application for a big company".
public class K61Exception extends Exception {
/* ... */
public boolean isReallyAnException() {
return isReallyAnException;
}
}
We often describe inheritance as an "is-a" relationship. A K61Exception is a kind of Exception. Except, perhaps, not really. Who knows? Maybe it is, maybe it isn't. You have to check if it really is an exception.
I'm certain the developer who did this thought it was a good idea at the time, probably because there's some module which might throw exceptions which are entirely recoverable. I suspect that isReallyAnException is really meant more to communicate how bad this exception is. I may be giving them too much credit, though.
Regardless, when is an Exception not an Exception? When it's bad code posted here. That's not exceptional at all.
|
Метки: CodeSOD |
CodeSOD: Two Knowing Comments |
Sometimes, it really is the comment which makes the code. Sometimes, the comments make simple (but still more complex than it needs to be) code less clear.
For example, Thomas provides this code, and comment, which… I understand what is happening here, despite the comment:
///
/// A class file for find the mid value
///
public class a
{
string[] s2 = null;
///
/// mid function is to cut the some values from the text which we had given
///
/// The offset number
/// Number of chars to read from the test
/// The string that has to be splited
/// returns a string array
public string[] mid(int offset, int numcharstoread, string s)
{
int j = 0;
for (int i = numcharstoread; i < offset; i++)
{
char ch = s[i];
this.s2[j] = Convert.ToString(ch);
j++;
}
return this.s2;
}
}
It's a substring function. I understand that just from the name: mid. But if I read the comments, I get less confident that it's a substring function. It's also a substring function which relies on class state, treats the substring as an array, and at least with this snippet, wouldn't work (because s2 is null and never gets initialized).
Other times, however, the comment is a perfectly clear summary of how we should feel about some code.
Kerin writes: "This is in the production codebase for an application that I support. Every time I look at it, I die a little more."
function local_luxSheetVoodoo(){
return true; //yes i know, don't email me
}
I need to add this to pretty much every line of code I write, I think.
|
Метки: CodeSOD |
CodeSOD: A Big Raise |
Everyone likes getting a pay raise. Well, I suppose it depends on why. And HR isn't going to be too happy about your raise if it comes as the result of an easy-to-avoid software bug.
C'edric V's company makes payroll software mostly used in and around France. One of their customers had a problem: when paying employees, it would give them a significant raise- sometimes by two orders of magnitude, rarely by three or four.
What was surprising is that this happened to one customer, but none of their other customers. They couldn't replicate it on their test environment either. But once C'edric started digging into the code, it wasn't hard to understand what the root cause was.
double origin = GetDoubleFromRandomMethod();
string str = origin.ToString().Replace(".",",");
decimal result = decimal.Parse(str);
The good thing I can say about this C# code is that it does, eventually, handle currency values using decimal, which guarantees that we won't lose precision. Of course, that decimal derives from a double, so we might already have errors.
Those errors, of course, don't matter, because somebody made some terrible assumptions about locale and added in an unnecessary ToString operation with an equally unnecessary Replace.
The ToString method on a double, by default, uses format "G" (for "general"), which is locale specific. So, if you're in a French locale, it will use the "," as a decimal separator. "3.14" in a US locale would be "3,14" in a French locale.
So, if this is used in a French locale, the Replace does nothing, and the Parse successfully converts the value back into a decimal.
But the customer with the sudden raise problem didn't run their servers configured in a French locale. Since much of their business involved US customers, they ran their servers in a US locale. In that case, the Replace would change the "." to a ",". The Parse would interpret the comma as a "thousands separator", but the parser just ignores that symbol, even if it's in the wrong place. So "3.14" becomes "3,14", which gets parsed as "314". The number of significant figures the double has behind the decimal place determines how many orders of magnitude someone gets a raise by.
I suppose they should be happy that there values coming out of that GetDoubleFromRandomMethod aren't the result of any division operations. One 8/12 and you are paying out a satanic amount in quadrilions.
|
Метки: CodeSOD |
Error'd: 4'33" |
It's hard to define what makes today's batch of submissions so special. Is it just the futility? Or is it the certainty that nobody nowhere knows nothing?
Audiophile Neal S. tentatively asserts "Not sure this is where the lyrics are supposed to be on the Spotify UI."
Ansel Pol patiently poses "I received this very informative email from eBay. Does this mean that my null is going to be delivered soon? "
Graham Asher raises a knotty question we can't untangle. "If you undefine NaN does it become a number ?"
Milquetoast Motorcyclist muses "The YouTube app is pretty bad but I wouldn't go so far as to call it 'null'. "
A hushed Daniel C. whispers that he bought his wife a silent Sonata, but it's a mite pricey: "Not sure I can afford NULL for the first NULL months package. Will have to check my bank account. "
|
Метки: Error'd |
CodeSOD: A Range of Skills |
Ulvhamne works on a team with over a hundred other developers. It's a big group, working on a huge project. And some of the quality in that code base gets… variable. Worse, when a bug pops up, it can be tricky to even identify what in the code is triggering the bug, let alone what the root cause is.
For example, one of the config-file fields needed a number to specify the beginning and end of a range. If you put in a relatively short range- thousands or hundreds of values- everything worked fine. That was a pretty typical use case. But if you put in something closer to MAX_INT, everything worked fine for a little bit, but within moments the server would grind to a halt, memory would fill up, and the OS would hang as it ended up constantly thrashing pages to disk.
Ulvhamne joked with one of his co-workers. "Wouldn't it be funny if, instead of just tracking the range's endpoints, they populated an array with all the possible values instead?"
public void addRange(int min, int max)
{
int key, value;
for(int i = min; i <= max; ++i){
key = value = i;
myMap.put(key, value);
}
}
public void addValue(int value){
int key = value;
myMap.put(key, value);
}
public boolean execute(int input){
Iterator iter = myMap.values().iterator();
while(iter.hasNext()){
if(iter.next().intValue() == input)
return true;
}
return false;
}
As it turns out, that is almost exactly what they did. Ulvhamne was only wrong about it actually being a map. That this has an addRange and an addValue method also hints that they wrote a generic wrapper class around maps, and then reused that wrapper for any problem they could roughly hammer into the right shape to abuse the class. That it requires a per-element search to see if an item is contained in the range, at least as implemented here, is icing.
For bonus points: this was also within a 30,000 line "utility" file.
|
Метки: CodeSOD |
CodeSOD: A Type of Code |
Like the war between Emacs and Vim, developers also tend to wage a war between "strongly typed" and "loosely typed" languages. There are tradeoffs either way, and I think that's why you see things like TypeScript and Python's type annotations starting to creep into loosely typed languages- types when you need them, but not required. But if you're not comfortable with types, and don't really understand type casting, you might start writing some code like, well, like these examples.
Sashi found this C# code:
public void FillUserinfoFromReaderWithDetails(SqlDataReader reader)
{
SqlDataReader dr = (SqlDataReader)reader;
…
}
We get a SqlDataReader as an input, decide that we want to have a different variable which references it (probably for no good reason), and to be extra safe, we make sure to cast the SqlDataReader into a SqlDataReader. All in all, this is just harmless, but it makes you wonder what the developer was thinking.
And if you can do that in C#, imagine the hijinks you can get up to in C. "SvOlli" found this:
*((unsigned short *) &number) = number | 0x2000;
number is defined as an unsigned short. So this code gets the address of number, converts it into a pointer to unsigned short, then dereferences the pointer to get back to the original value, which it sets equal to number | 0x2000. "SvOlli" writes: "We Germans have a saying 'von hinten durch die Burst ins Auge', which translates to 'from behind through the breast into the eye'. It means something that could be done easy is done really complicated."
This line could be replaced by number |= 0x2000.
And finally, we flip over to JavaScript. Oh, sure, JavaScript doesn't require any type conversions, but Corey's co-worker had an object with boolean values in it, and wants to make sure those boolean values get copied over into another object. They might not need to tern one type into another, but "this isn't needed" isn't going to stop them:
function checkAll(checkname, exby)
{
for (i = 0; i < checkname.length; i++)
checkname[i].checked = exby.checked ? true : false
}
Now, this may not actually be a WTF, or at least, the code itself may not be. While exby.checked is supposed to be a boolean value, JavaScript is happy to evaluate the truthiness of everything, so if a non-boolean got slipped in, you might get unexpected results. Then again, checkname is almost certainly an array of HTMLInputElements, and the checked property on a checkbox obeys the same truthiness rules as JavaScript variables, so even if these weren't booleans, it'd be fine.
Which brings us to TRWTF, which forever and always will be how JavaScript handles types and type-coercion.
|
Метки: CodeSOD |
CodeSOD: Last One In |
A lesson that everyone learns at some point is "don't write your own authentication code." Authentication, like encryption, and like dates, is incredibly complex and has all sorts of ways you can subtly mess it up and not realize your mistake.
Take, for example, this code from Christopher. His peer wrote this code, added a single test record to the database, saw that it worked, and called it a day.
public void Login(string userName, string hashValue)
{
SqlDataReader dr = getUsers();
while (dr.Read())
{
if (dr["userName"].ToString() == userName && dr["hashPassword"].ToString() == hashValue)
{
this.authenticated = true;
}
else
{
this.authenticated = false;
}
}
}
Christopher spotted the core problem, and tried to explain it to the developer. When it wasn't sinking in, he quickly whipped up a script to add tens of thousands of users to the table, keeping the developer's username as the last row in the table. They could still log in, but logging in took much longer.
"Maybe I need to add an index?" was their response.
Christopher added one more row to the database, and now the developer was no longer able to log in.
"I don't understand? Why did it stop working?"
"You're looking at every row, and setting this.authenticated for each one. So the only one that ends up counting is the last one."
The lightbulb finally went on, but it wasn't a particularly bright one. "Oh, right, I should add a break if it matches!"
"Or," Christopher said, "let me tell you about a little thing called a WHERE clause…"
|
Метки: CodeSOD |
Error'd: And then I gave him my digits. |
Franz K. anonymously ponders the meaning of existence after this encounter
Deep thinker Pascal wagers he knows the difference between something and nothing
"I tried 1234 and it worked! exclaims bruncher Daryl D.
Chrononaut Paul T. highlights the nth hardest problem in computer science: "I've seen plenty of inaccurate progress meters, but never quite like this"
"I guess I can throw Trugreen's a few picopence to round up to 38!" flushes big spender Robert H. while we all sob silently in BCD.
https://thedailywtf.com/articles/and-then-i-gave-him-my-digits
|
Метки: Error'd |
CodeSOD: Double Your Value |
There are many ways to debug your code. You might use an actual graphical debugger, wrestle with GDB, just spam print statements, or rely on a logging framework to help you generate useful diagnostic output.
Since you're going to need some logging output no matter what, it's always good to heavily instrument your code. Using logging levels, you can control quite well what gets dumped when. Well, "LostLozz" had a co-worker who found an… interesting way to control logging.
if ( LOG.isDebugEnabled() ) {
try {
Integer i = null;
i.doubleValue();
}
catch ( NullPointerException e ) {
LOG.debug(context.getIdentity().getToken() + " stopTime:"
+ instrPoint.getDescription() + " , "
+ instrPoint.getDepth(), e);
}
}
If our logging is configured for debug… force a null pointer exception, then log it. I suspect that they're using the exception to capture the stack trace, and the logging framework will automatically expand that in some useful fashion within the log. That's the best theory I can come up with to explain what's going on here.
Of course, there are built in methods for that, and most logging frameworks probably have some wrapper that gets that information. And I'm just guessing at the intent here, which means they might have just done this because… they could? Because they had some deep belief about the role of logging and believed it should only happen in exception handlers? Because they were hoping to get featured on the site?
It's an exceptional mystery.
|
Метки: CodeSOD |
CodeSOD: The Secret to Success |
"I was once working for a company that primarily dealt with Oracle products," Tai writes.
That vendor, who shall not be named again, provided an installer. Tai ran it, and it failed. Since the installer was a shell script, she opened up the file and took a look.
./run_first_file.sh
#check_for_success()
./run_second_file.sh
check_for_success()
The first thing Tai noticed is the commented out check_for_success. It seemed odd to comment that out, unless you had some sort of expected failure, but that raised a bunch of questions about how that was being handled. Out of curiosity, Tai uncommented it, and sure enough, the whole thing failed before hitting the run_second_file.sh line. But why was it failing?
Well, as it turns out, the installer helper scripts were in a subdirectory. When Tai corrected the installer to this:
# This was my fix
./scripts/run_first_file.sh
check_for_success()
./scripts/run_second_file.sh
check_for_success()
everything worked fine.
In the end, we have a case of a major software vendor providing a trivially broken installer for a product with a price tag that would make your eyes water. At some point, they probably tested it, realized it was failing, and just commented out the check for success. It kept failing, and they just… probably stopped testing? That's my guess: "It fails on this line, but this line is only a safety check, so we just take that line out, now it'll stop failing. Great, ship it!"
I may be being generous in assuming that they tested it at all.
|
Метки: CodeSOD |
CodeSOD: A Terned Around Discount |
If you browse the Errords, it's easy to see that "giving customers a discount" is apparently harder than it looks.
Brian's company had one of those "discounts are hard" problems, way back when. Sometimes instead of a discount reducing the price, it would raise it. The root cause was that the sales team setting up the promotions weren't clear about whether the discount amount should be a negative or positive number. Instead of adding validation to ensure they always entered a negative (or at least, a zero amount), one of Brian's predecessors fixed the bug in their C# like this:
PromotionDiscount = n.PromotionAmount.ToString()[0] == '-'
? n.PromotionAmount.ToString() : n.PromotionAmount.ToString() == "0"
? n.PromotionAmount.ToString() : "-" + n.PromotionAmount.ToString()
Line-breaks added.
So, problem the first, of course, is that the input should have been validated so that everything happening here is unnecessary. Problem the second is that this code is meant to be taking any discounts that are greater than zero, and making them negative, and decides to do this through string manipulation. PromotionAmount is already a numeric type, so if n.PromotionAmount > 0… could do the job, or if you really want a one-liner, Math.abs(n.PromotionAmount) * -1.
Then, of course, it's all topped with a nested ternary. It's at least a short one, but it adds a wonderful something extra. Code that doesn't need to exist, implemented in the most awkward way, with a bonus of unreadability.
|
Метки: CodeSOD |
Worlds Collide |
George had gotten a new job as a contractor at a medium-sized book distributor. He arrived nice and early on Day 1, enthusiastic about a fresh start in a new industry.
His "office" turned out to be a huge warehouse stacked high with books. Upon greeting him, his manager pointed him to a PC in the corner of the warehouse, sitting on a desk with no partitions around it. The manager leaned over the machine and made a few double-clicks with the mouse until he opened up the H: drive. "There you go," he muttered, then left.
George stared after him, perplexed, wondering if the manager intended to bring over coffee or other coworkers to meet him. The way he was walking, though, seemed to convey that he had more important things to be doing than coddling greenhorns.
"You must be George. Hi, I'm Wally." Another gentleman came over with his hand poised to shake. "I handle the software we use to track inventory. Let me show you the ropes."
Wally used the nearby computer to demonstrate a handful of the 200-odd Delphi forms that constituted the inventory application. The source code was not in any kind of source control; it was all in a folder named Wally on the shared H: drive. They were using a version of Delphi from 1995 ... in 2010. Their database was some vague, off-brand SQL-esque construct that George later learned had been dropped from support as of 2003.
None of this inspired George's confidence, but he had a job to learn. Stifling a sigh, he asked Wally, "Could I have a copy of your database creation script? Then I could start with a fresh and empty database to learn on."
"No problem. Come with me."
Wally led George to another part of the warehouse where a different computer was set up; presumably, this was Wally's desk. Wally sat down at the machine and began typing away while tapping his foot and whistling a little tune.
This went on, and on ... and on. It certainly didn't seem like the quick typing one would do to create an email with an attachment. George shifted his weight uneasily from one foot to the other. As the rhythmic typing and whistling continued, it hit him: Wally was typing out the entire CREATE DATABASE code—from memory.
It took Wally a good 25 minutes to bang out everything needed to define 60-odd database fields including Title, ISBN, ISBN-19, Author, Publisher, etc. Finally, the one-man concert ceased; Wally sent the email. With a perfectly normal look on his face, he faced George and said, "There it is!"
In the moment, George was too flabbergasted to question what he'd witnessed. Later, he confirmed that Wally had never even thought to have a saved CREATE DATABASE SQL script on hand. Sadly, this was far from the last point of contention he experienced with his coworker. Wally could not comprehend why George might want some general utility functions, or a clean interface between modules, or anything more advanced than what one found in chintzy programming manuals. George's attempts at process improvement and sanity introduction got his building access card invalidated one morning about a month after starting. No one had expressed any sort of warning or reproach to him beforehand. George was simply out, and had to move on.
Move on he did ... but every once in a while, George revisits their old website to see if they're still in business. At the moment, said website has an invalid certificate. For a company whose whole business came down to head-scratching practices heaped upon 15 year-old unsupported tools, it's not so surprising.
|
Метки: Feature Articles |
Error'd: The Sound of an Unheard Treefall |
Brett N. starts us off today with a timely notification that he received late on the 24th
Cat F. reminds us that caching is one of the two hardest problems in computer science. "Turns out I was signed in and the credential error was because it needed refreshing..."
Lumberjack Tim B. poses an existential puzzler: "If an error is undetectable, is it really an error? How does the error page know to report it?"
While waiting on the waveform, Duncan muses on the superposition of simultaneous success and failure. "I'm glad someone is happy about denying me access to my gifts," he observes.
Finally, intrepid war reporter Ben checks in from the front lines of the battle between our future AI overlords: "In five minutes that yellow banner will say, Our Announcements section doesn't know what it's talking about!"
https://thedailywtf.com/articles/the-sound-of-an-unheard-treefall
|
Метки: Error'd |
CodeSOD: A Bit of Power |
Powers of two are second nature to a lot of programmers. They're nearly inescapable.
Equally inescapable are programmers finding new ways to do simple things wrong. Take Sander's co-worker, who needed to figure out, given a number of bits, what's the largest possible value you could store in that number of bits. You or I might reach for our language's pow function, but boy, in C++, that might mean you need to add an include file, and that sounds hard, so let's do this instead:
DWORD CBitOps::BitPower(WORD wNrOfBits)
{
DWORD Res = 1;
if (wNrOfBits == 0)
{
return true;
}
for (WORD counter = 0; counter < wNrOfBits; counter++)
{
Res = Res*2;
}
return Res;
}
Indenting as submitted, just for that extra little something.
The bit that puzzles me is that it wisely validates its inputs… but it returns true if you passed in a zero. And arguably, the input validation is incomplete, as a large value in wNrOfBits could easily create an output much larger than DWORD can hold. Still, the return true is going to cast a boolean to a DWORD which because this is clearly WinAPI code, I'm sure it's going to do something weird like actually return 0, which raises the question: why not return 0 in that case?
Sander did the "hard" work of throwing this code away and replacing it with a more standard call to the math libraries.
|
Метки: CodeSOD |
News Roundup: We're Going to Need a Bigger Boat |
You’ll have to stay patient with me on this post, since the point I will eventually get to really is the confluence of a number of different threads that have been going through my head the past few weeks.
Let’s start with my time in business school from 2007 to 2009. Charles Dickens couldn’t have penned a better dichotomy between the beginning and end of my time in school. In short: 2007 = the economy couldn’t be better, 2009 = the economy couldn’t be worse.
As my dream of a career in finance seemed further and further from reality while my time in school was coming to a close, I took a chance on a product development class for my last semester. It was the first time that the concept of ‘user experience’ was introduced to me, and judging from Google Trends, interest in user experience was at a real low point.
The class was really an exercise in patience by our professor; it involved us students continually frustrating the professor with our complete lack of creativity. For our final project, we were randomly assigned to teams to find a real user problem and an elegant solution. My team followed a pretty standard process:
Our team consisted of 5 of the most passive, easy-going individuals - which was not conducive to the sort of analytical and critical thinking necessary to build a great product. After waffling for weeks over an appropriate problem, one of the teammates revealed that his wife was about to give birth and that a recent concern had been finding a mobile baby unit that would hang over their crib that would be both fun and educational.
A little aside here: A point of emphasis from our professor was Henry Ford’s famous quote (which he may not have actually said), “If I had asked people what they wanted, they would have said faster horses”. People seem to take out of this quote what they want to hear, and it made little impression on me at the time, but I understand it to mean the following: “Linear solutions to complex problems don’t result in novel solutions for customer problems. They result in old solutions with more bells and whistles. What is oftentimes required is starting from scratch in building user-friendly solutions that solve for customer needs, even if they are unstated.”
Mobile baby units were already fun and engaging, so it was the educational part that hung us up. I’ll cut to the chase: our final deliverable was a monstrosity that combined every feature of every current mobile baby unit into one ugly mass. Think the chandelier from ‘The Phantom of the Opera’ with all of the theatrics of it swinging through the crowd (in fact our prototype fell from the ceiling into a crib we were using for testing). We fell into the trap of a linear solution to a complex problem, if it even was a problem to begin with! We took a solution already in place, added more functionality, and got a failing grade on our final project.
This whole experience helped me relate to a product that was recently brought to my attention. Even though user experience is at its apex in interest according to Google Trends, people continue to create linear solutions to complex problems. Case in point: The Expanscape, the all-in-one security operations workstation; a product that purports to solve every problem a security operations analyst may need.
...How about designed for no one?
The specs advertise 7 screens, but is only 60% of its 10 kg goal. What does getting a percentage of a weight goal even mean? Does it mean that it weighs 16.67 kgs, since 60% of that weight makes 10 kgs?
Honestly the weight itself isn’t even that egregious, since it’s in line with most gaming machines nowadays. The problem being solved for is quite straightforward: “Design and build a proper mobile Security Operations Center.” It is indeed mobile, with its ability to “fold down compactly to facilitate travel”. But is it proper? I think not.
This all-in-one, mobile bundle is trying to solve every problem linearly, by adding more features to an existing laptop. Judging by the sheer number of screens, I wonder if any real user testing was done. I can’t imagine any single human not feeling anxiety just looking at this machine. The problem being solved was not to increase efficiency and shrink a team to just one person, it was to design and build a proper security operations center! Who can focus on this many screens and information at once?
I don’t want to be harsh on the Expanscape; history is filled with examples of linear and poorly-executed tech solutions. Who can forget the Nokia N-Gage, which was a mash-up of every phone feature at the time? Or Google Glass, which was trying to allow users to engage with the world without even needing to look at a phone, trying to force the wrong solution to their stated problem? Or the Apple Newton, which while arguably was ahead of its time, focused too heavily on functionality over user experience?
I’m left thinking of the famous quote by Ian Malcolm. They are good words for us all to live by:
https://thedailywtf.com/articles/we-re-going-to-need-a-bigger-boat
|
Метки: News Roundup |
CodeSOD: A Lack of Progress |
Progress bars and throbbers are, in theory, tools that let your user know that a process is working. It's important to provide feedback when your program needs to do some long-running task.
Hegel inherited a rather old application, written in early versions of VB.Net. When you kicked off a long running process, it would update the status bar with a little animation, cycling from ".", to "..", to "...".
Private Sub StatusRunningText(ByVal Str As String)
If ServRun = True Then
Select Case Me.Tim.Now.Second
Case 0, 3, 6, 9, 12, 16, 19, 21, 24, 27, 30, 33, 36, 39, 42, 45, 48, 51, 54, 57, 59
Me.StatusBarPanel1.Text = Str + "."
Case 1, 4, 7, 10, 13, 17, 20, 22, 25, 28, 31, 34, 37, 40, 43, 46, 49, 52, 55, 58
Me.StatusBarPanel1.Text = Str + ".."
Case Else
Me.StatusBarPanel1.Text = Str + "..."
End Select
End If
End Sub
Now, you or I might have been tempted to use modulus here. Second % 3 + 1 is the exact number of periods this outputs. But how cryptic is that? It involves math. This developer has lovingly handcrafted their animation, specifying what should be displayed on each and every frame.
Modular arithmetic is math, but this code, this is art.
Bad art, but still, art.
|
Метки: CodeSOD |
CodeSOD: Self-Documented |
Molly's company has a home-grown database framework. It's not just doing big piles of string concatenation, and has a bunch of internal checks to make sure things happen safely, but it still involves a lot of hardcoded SQL strings.
Recently, Molly was reviewing a pull request, and found a Java block which looked like this:
if (Strings.isNullOrEmpty(getFilter_status())) {
sql.append(" and review in ('g','t','b','n','w','c','v','e','x','')");
} else if (!"a".equals(getFilter_status())) {
sql.append(" and review = '"+getFilter_status()+"'");
} else {
limit_results=true;
}
"Hey, I get that the database schema is a little rough, but that big block of options in that in clause is incomprehensible. Instead of magic characters, maybe an enum, or at the very least, could you give us a comment?"
So, the developer responsible went back and helpfully added a comment:
if (Strings.isNullOrEmpty(getFilter_status())) {
sql.append(" and review in ('g','t','b','n','w','c','v','e','x','')"); // f="Resolved", s="Resolved - 1st Call"
} else if (!"a".equals(getFilter_status())) {
sql.append(" and review = '"+getFilter_status()+"'");
} else {
limit_results=true;
}
This, of course, helpfully clarifies the meaning of the f flag, and the s flag, two flags which don't appear in the in clause.
Before Molly could reply back, someone else on the team approved and merged the pull request.
|
Метки: CodeSOD |
Error'd: The Timing is Off |
Drew W discovers that the Daytona 500 is a different kind of exciting than we ever thought.
It feels like the past year has been a long one, and based on this graph's dates, it's going to get a lot longer.
But time really does fly. Look how much earlier Scott Lewis's package is arriving.
Which, with the way time flies, it's good that this limited time offer on a video game will stick around long enough that Denilson will get a chance to take advantage of it.
|
Метки: Error'd |
CodeSOD: Spacious Backup |
Today's anonymous submitter works on a project which uses Apache Derby to provide database services. Derby is a tiny database you can embed into your Java application, like SQLite. Even though it's part of the application, that doesn't mean it doesn't need to be backed up from time to time.
Our submitter was handed the code because the backup feature was "peculiar", and failed for reasons no one had figured out yet. It didn't take too long to figure out that the failures were triggered by not having enough space on the device for a backup. But they definitely had a enoughFreeSpaceForBackup check, so what was going wrong?
private boolean enoughFreeSpaceForBackup() throws IOException {
final Path databaseDir = workingDirectory.resolve("database"); // resolve folder where database is written to
final FileStore store = Files.getFileStore(workingDirectory.getRoot()); // get the filestore
final long backupFreeSpace = store.getTotalSpace(); // get the ... yes ... total size of the file store
final long databaseSpace = sizeInBytes(databaseDir); // determine the complete size of the database
return backupFreeSpace > databaseSpace / 2; // if our databasesize, divided by 2, is smaller than the total size of the file store ... we have enough space for a backup.
}
As our submitter's comments highlight, it's the call to store.getTotalSpace(), which helpfully tells us the total size of the file store. Not, however, how much of that space is unallocated, which is what getUnallocatedSpace tell us. Or getUsableSpace, which tells us how much of the device is actually accessible by the JVM.
That is to say, there are three methods pertaining to size, and the developer managed to pick the one which was totally wrong.
But that's not the WTF. It's an easy mistake to make, after all, especially if you're not super familiar with the API. I mean, getTotalSpace sounds, well, like it's getting the total amount of space, not the amount I can use, but this is a simple enough mistake that it could happen to anyone.
But that's why you test. That's why you have code reviews. This software had been shipped, it was installed on people's devices, it was doing work. And in production environments, it was absolutely certain that it always had enough room to take a backup of itself, because the only time this wolud return false was if the database were two times larger than the drive it was on.
|
Метки: CodeSOD |