Another common bug in published code | Hacker News Reader

Another common bug in published code

google.com

29 pointsshawndumas17 comments

There's an important distinction between this code and the Math.random() casting that you posted earlier (http://news.ycombinator.com/item?id=2472223 in case anyone missed it).

With parseInt, we're talking about best practices: if you fail to specify the radix, your code may yield unexpected results (eg: parsing a number with a leading 0 may cause parseInt to assume an octal base). On the other hand, your code may still work fine if your inputs are properly formatted. In that case, specifying the radix just enhances readability.

With casting Math.random(), what you saw was an actual bug: people were using 0's in their code when they thought they were getting random integers. Unless the code was being written for an obfuscated Java contest, I'd wager it was buggy.

Not sure why you don't think the missing radix is a "real" bug, there are plenty of real use cases where a leading zero is proper formatting, for example, in spreadsheets that keep the same number of digits in each cell. Not to mention that since this is all web code there is a good chance this is all user input so anything goes.

My point is that specifying the radix is a best practice: there are still a number of situations where it's unnecessary for properly functioning code (ie: if you aren't using zero-padded numbers). Specifying the radix certainly helps readability, but you can't just do a regex search and say that all the code it finds is buggy.

On the other hand, casting Math.random() to an int is wrong unless you're trying to express 0 in an obfuscated way.

Agreed. If you're calling parseInt on a variable it's simply not a bug.

parseInt(1) seems like a whole different class of bug to me :-)

Well, say you're trying to get an int from .style.top, it'll never have leading zeroes.

There are also real use cases where octal and hex parsing are desired. To do a search for parseInt\(*[^,]\) and declare all as bugs is myopic.

If the input string begins with "0", radix is eight (octal). This feature is non-standard, and some implementations deliberately do not support it (instead using the radix 10). For this reason always specify a radix when using parseInt.

While the radix parameter is optional, always specify it to eliminate reader confusion and to guarantee predictable behavior. Different implementations produce different results when a radix is not specified. [1]

----

[1]: https://developer.mozilla.org/en/JavaScript/Reference/Global...

that's not a bug, that's a feature.

a questionable feature, yes, but not a real bug.

It's a defect, it leads to inconsistent and unpredictable behavior. A better default behavior would have been to use a default radix (base 10) and have an option for auto-detected radix (e.g. strings that begin with 0x). Considering how rare octal formatted integer strings are these days (essentially non-existent) and how much more common 0 prefixed decimal formatted integer strings are, this is definitely not a useful feature.

> A better default behavior would have been to use a default radix (base 10) and have an option for auto-detected radix (e.g. strings that begin with 0x).

Note that the option exists (provide `0` as radix). It's just stupid that the default radix is `0` (or `undefined`) and not `10`.

Not to mention, that would put parseInt in line with Number.toString(radix), which (sensibly) defaults to base 10 (side note: I was surprised and impressed to see that Number.toString can handle any base from 2 to 36 included)

But CSS style attributes like .style.left are never zero-leading, and in that case you don't need to pass the radix.

One of the lessons I took away from the static analysis tools I architected (PREfix and PREfast) was just how many bugs you can find with simple pattern-matching.

To make it work in practice, you need some UX support -- including explanations (so in this case, making it clear that the results are undefined if the integer starts with a 0) and recommended fixes (specify the radix). Otherwise, people are likely to dismiss the messages as a false positive.

There's an uncommon bug in the regexp in the link: sequence count metacharacters such as * are postfix, not prefix. (please ignore whitespace in following regexps as the YA markup requires whitespace to avoid making text italicized).

parseInt\(* [^,]\) means match "parseInt", then 0-or-more '(', then a character that is not a comma, then ')'.

The regexp should be something like parseInt\([^,()]* \) which returns code where the argument is more than a single character http://www.google.com/codesearch?hl=en&lr=&q=parseIn...

FYI: http://news.ycombinator.com/formatdoc

There is one exception where dropping the radix is the best option: when you're parsing a format or programming language with hex, decimal, and octal notations, such as Java in the case of processing.js.

The headline figure of 22 million "bugs" can be reduced somewhat by eliminating jQuery, which brings it down to 3% of this (600,000). Plus one.

Looks like a lot of these are in libraries. I see moo tools, tinymce and others like that as well. There are possibly specific reasons why a library author has chosen not to include the radix parameter, or they could just be old versions as well.

Reply on news.ycombinator.com