Death by radix
13-Jul-2011This post is reinforcement of a programming trap I should avoid in future, and punishment for selecting poor test cases. It’s not a bug of epic proportions, so move along if you have better things to laugh at than a bug in my Daily WTF-candidate code.
I threw together a simple booking calendar that uses a touch of JavaScript to make it a little more animated than a basic web form on cheapo PHP hosting. Part of the magic of this calendar is that it creates a string of the form “Your booking is for one night from Wednesday July 13th 2011, departing Thursday July 14th 2011″ to help reinforce to the user that the correct dates have been understood by the booking system.
When I implemented this booking calendar it appeared to work nicely. Nonetheless, I wrote test cases for it. When there were plenty of tests and they all passed I was feeling smug that my JavaScript was probably provably better quality than most of the JavaScript in existence given it had tests.
It operated for a few months with no complaint. Then there were several reports in one week that the code wasn’t generating the correct weekday.
“Inconceivable!” I thought.
I tried it myself with the dates users complained about and found it was generating crazy weekdays. I checked, and my regression tests were passing just fine. I then plugged the reported problem dates into my test suite. My old test cases succeeded and the new ones failed.
So, down to diagnosis.
The “bulk” of the code is pulling the date string apart so I can use date-related utility functions. The problem was my use of parseInt().
parseInt() parses a string and returns an integer. I discovered that it works fine for some months (01, 02, 03, 04, 05, 06, 07, 10, 11, & 12) and not for 08 and 09. This is because parseInt() assumes that if the string it is parsing begins with a 0, then it’s an octal number. That works for 01-07 because they generate the same number whether you parse them as octal or not. 11 and 12 work because parseInt() correctly guesses that they are base 10. 08 and 09 are automatically detected as octal, but they fail to parse as octal numbers. I used the error return value to populate the month in a Date object which dutifully interpreted the value as December. The weekdays were right… for December.
Sure enough, none of my test data contained a date in August or September.
The main fix is that the parseInt() function takes a second optional argument that sets the radix to use and stops it from guessing.
I’m still trying to think of a situation when you might want parseInt() to guess the radix for you.
Octal drives me crazy. The Java lexer even includes a special exception to ensure that a lone “0″ digit is interpreted as a decimal zero, and not one of those crazy octal zeroes:
http://java.sun.com/docs/books/jls/second_edition/html/lexical.doc.html#48282
This is PHP’s fault. Fuck PHP.
OK thinking more logically now, a better critique of PHP is that PHP needs a variant of every function which reads:
[function]AndDontBeSmartAboutIt(…)
Ah, Sunny. This is a bug you can trip up on in almost any language. And I think Chris was using JS to implement it in this particular case.
For C/C++, you’ve got strtol(char*,char**,int) which optionally takes 0 for the last arg to let the system interpret the value for itself. For Java, you’ve got Integer.valueOf(String) which does the same. I’m sure PHP can do it too, but I’m bored of researching broken parsing APIs already :)
As for why on earth you might do this, the only reason I can think of is that you’ve decided to let the user pass in numbers in whatever format they prefer, and you’re just going to handle it. This could be for yet another calculator app, or a bytecode generator, packet editor, whatever. Still, it’s crazy to use anything other than base 10 if you don’t need this flexibility.
I’ve also been a victim of over-exuberant octal conversion by std::cin many times. This is an open invitation to participate in ritual public flogging next time I fall for this.
Eh? Don’t be draggin’ c++ down with the PHP riffraf!
std::cin, along with all c++ iostreams, is defined to use decimal numbers on construction. [ref]. Of course if you change the base you can sometimes forget to change it back, and affect other parts of the code (damn global state).
However, as far as I know there’s no way to do this funky radix-sniffing which, as we can all agree, is an anti-feature anyway….
I apologise for maligning you and Bjarne with my quip about std::cin with only one comment between it and a dissin’ of PHP. iostream certainly behaves itself now.
Late last millenium, VC++ was not being so reasonable.