Number.toFixed() rounding errors: broken but fixable

Original gist for this post

I found a rounding bug in Number.toFixed() in every JavaScript environment I’ve tried (Chrome, Firefox, Internet Explorer, Brave, and Node.js). The fix is surprisingly simple. Read on…

Warm up

I found this version of the rounding bug in toFixed() while revising a number-formatting function that performs the same kind of thing as Intl.NumberFormat#format().

(1.015).toFixed(2) // returns "1.01" instead of "1.02"

The failing test is on line 42 here: https://gist.github.com/dfkaye/0d84b88a965c5fae7719d941e7b99e2e#file-number-format-js-L42. I had missed it until yesterday (4 Dec 2017), and that spurred me to check for other problems.

See my tweets about it:
+ Bug Alert
+ Compare with Intl.NumberFormat
+ Wrap Up
+ Polyfill

Bug reports

There is a long history of bug reports with respect to rounding errors using toFixed().

In general, these point out a bug for a value, but none reports a range or pattern of values returning erroneous results (at least none that I have found, I may have missed something). That leaves the programmers to focus on the small without seeing a larger pattern. I don’t blame them for that.

Finding the pattern

Unexpected results based on input must arise from a shared pattern in the input. So, rather than review the specification for Number.toFixed(), I focused on testing with a series of values to determine where the bug shows up in each series.

Test function

I created the following test function to exercise toFixed() over a series of integers ranging from 1 to a certain size, adding the fraction such as .005 to each integer. The fixed (number of digits) argument to toFixed() is calculated from the length of the fraction value.

    function test({fraction, size}) {

      // Happy side-effect: `toString()` removes trailing zeroes.
      var f = fraction.toString()
      var fixed = f.split('.')[1].length - 1

      // All this to create the expectedFraction message...
      var last = Number(f.charAt(f.length - 1))
      var digit = Number(f.charAt(f.length - 2))

      last >= 5 && (digit = digit + 1)

      var expectedFraction = f.replace(/[\d]{2,2}$/, digit)

      return Array(size).fill(0)
        .map(function(v, i) {
          return i + 1
        })
        .filter(function(v) {
          // Compares 1.015 to 1.0151 b/c fixing by more than one decimal place rounds correctly.
          var n = v + Number(fraction) // number 1.015
          var s = n.toFixed(fixed) // string "1.015"
          var s1 = Number(n + '1').toFixed(fixed) // string "1.0151"
          return s != s1
        })
        .map(function(v, i) {
          return {
            given: (v + Number(fraction)).toString(),
            expected: (Number(v.toFixed(0)) + Number(expectedFraction)).toString(),
            actual: Number(v + fraction).toFixed(fixed)
          }
        })
    }

Usage

The following example executes on integers 1 through 128, adding the fraction .015 to each, and returns an array of “unexpected” results. Each result contains a given, expected, and actual field. Here we consume the array and print each item.

test({ fraction: .015, size: 128 })
  .forEach(function(item) {
    console.log(item)
  })

Output:

For this case, there are 6 unexpected results.

Object { given: "1.015", expected: "1.02", actual: "1.01" }
Object { given: "4.015", expected: "4.02", actual: "4.01" }
Object { given: "5.015", expected: "5.02", actual: "5.01" }
Object { given: "6.015", expected: "6.02", actual: "6.01" }
Object { given: "7.015", expected: "7.02", actual: "7.01" }
Object { given: "128.015", expected: "128.02", actual: "128.01" }

Findings

I found the bug consists of three parts:

  1. The last significant digit in the fraction must be 5 (.015 and .01500 produce the same result).
  2. The fixing length must shorten the fraction by only one digit.
  3. The bug appears inconsistently as different integer values are applied.

Inconsistently?

For example, (value).toFixed(2) with different 3-digit fractions ending in 5, for integers 1 though 128, produces these results:

  • fixing numbers ending with .005 ALWAYS fails (!!)
  • fixing numbers ending with .015 fails for 1, then 4 through 7, then 128
  • fixing numbers ending with .025 fails 1, 2, 3, then 16 through 63
  • fixing numbers ending with .035 fails for 1, then 32 through 128
  • fixing numbers ending with .045 fails for 1 through 15, then 128
  • fixing numbers ending with .055 fails for 1, then 4 through 63
  • fixing numbers ending with .065 fails for 1, 2, 3, then 8 through 15, then 32 through 128
  • fixing numbers ending with .075 fails for 1, then 8 through 31, then 128
  • fixing numbers ending with .085 fails for 1 through 7, then 64 through 127 (!!)
  • fixing numbers ending with .095 fails for 1, then 4 through 7, then 16 through 128

Those of you with more binary and floating-point math knowledge than me can probably reason out the underlying cause. I leave that as an exercise for the reader.

Fixing toFixed()

Fixing a value by more than one decimal place always rounds correctly; e.g., (1.0151).toFixed(2) returns “1.02” as expected. Both the test and polyfill use that knowledge for their correctness checks.

That means there’s a simple fix for all implementations of toFixed(): If the value contains a decimal, append “1” to the end of the string version of the value to be modified. That may not be “to spec,” but it means we will get the results we expect without having to revisit lower-level binary or floating-point operations.

Polyfill

Until all implementations are modified, you can use the following polyfill to overwrite toFixed(), if you’re comfortable doing that. – (Not everyone is.)

;(1.005).toFixed(2)=="1.01"||(function(p,f){
 f=p.toFixed, p.toFixed=function(d,s){
  s=(''+this).split('.')
  return f.call(+(!s[1]?s[0]:s.join('.')+'1'), d)
 }
}(Number.prototype));

Then run the test again and check that the length of the results is zero.

test({ fraction: .0015, size: 516 }) // Array []
test({ fraction: .0015, size: 516 }).length // 0

Or just run the initial conversion that started off this post.

(1.015).toFixed(2) // returns "1.02" as expected

Thank you for reading πŸ™‚

Advertisements

Why input string format matters for JavaScript (and moment.js) dates

Why input string format matters for JavaScript (and moment.js) dates

see original gist, 2017-06-17, at https://gist.github.com/dfkaye/e46b2f44ea5f624224602b51a7d4f6bb#file-why-format-matters-md

Given this format, ‘2017-05-01’, I want the date of May 1, 2017

Try it:

new Date('2017-05-01')

Turns out that Date() processes that format as UTC based, not local based.
And that returns Sun Apr 30 2017 17:00:00 GMT-0700 – 7 hours behind – which is not what we want.

Try it without the leading zero on the month,

new Date('2017-5-01')

and that returns Mon May 01 2017 00:00:00 GMT-0700 which is what we want.

Try it with stroke/slash characters in place of the hyphens:

new Date('2017/05/01')

That works, too: Mon May 01 2017 00:00:00 GMT-0700

Recommended format: YYYY/MM/DD

    Recommended format for parsing is: YYYY/MM/DD – user local settings independent. It always assumes
    local timezone offset for date string given as a parameter.

More on this at “JavaScript Date: a Bad Part”, https://jj09.net/javascript-date-a-bad-part/, by Jakub Jedryszek.

JavaScript Date vs. Moment.js

This came up today as I was debugging some Moment.js date/time library warnings. The recommended ‘YYYY/MM/DD’ format of the date string param, which gives us what we want from the JavaScript Date() constructor, produces something unexpected in Moment.

Moment('2017/05/01')

Result is a long console warning.

Deprecation warning: value provided is not in a recognized RFC2822 or ISO format. moment construction falls back to js 
Date(), which is not reliable across all browsers and versions. Non RFC2822/ISO date formats are discouraged and will be 
removed in an upcoming major release. Please refer to http://momentjs.com/guides/#/warnings/js-date/ for more info.

Turn that off by using Forgiving Mode, i.e., by adding a second parameter, indicating the format, possibly “non-standard”, to be expected by Moment.

Moment('2017/05/01', 'YYYY/MM/DD')

Midnight-minus-one-hour Conclusions

  • Specifications (RFC2822/ISO) and implementations (Date(), Moment.js) are two different things.
  • Time formatting is really hard to codify once and be done with.
  • It can take a lot of time to read up and understand the issues completely.
  • (Author’s message) Use test-driven development to catch this stuff. I first detected these issues today using TDD (For the record, I’m transforming a JSON response into a proper structure to be consumed by d3.js for some line/series charts. TDD, which had seemed a bit costly here, actually cut short the guesswork today).

Using only Month and Year in jQueryUI Datepickers

User interface components in general are built with an expected user behavior.
We used the jQuery-UI datepicker, to save time, but in an unusual way, which
cost us a lot of time discovering what we’d missed. We re-learned a key insight,
namely, month and year are navigation, not selection controls.

The rest of this post describes the implications we worked out. Short version
of the code is at the bottom of this post. There are other files attached in a gist version of this post at https://gist.github.com/dfkaye/f1efc4be64699df2d65c55e27b41f725

Background

Recently (November 2016) we ran into a design requirement that a dashboard report should cover a time period, with start and end dates and that the report should update when users change a month or a year in either the start or end datepicker.

filter-date

In the interest of saving time, rather than roll our own control for that, we
chose to use the JQueryUI datepicker for each of these, but with an unusual
approach that we would not show the calendar control (the table with all the
clickable date links).

Of course, that unexpected approach led to unexpected and undesired behavior.

Setup

In our implementation, we used changeMonth: true and changeYear: true in the
datepicker(options) argument, in order to display the month and year “
elements, and set let the datepicker display “inline” (i.e., always, without a
dialog – see http://jqueryui.com/datepicker/#inline).

We also set the defaultDate to the current year and month, and use 1 as the
day, so that we’re always comparing the first of any month.

We also needed to keep the two datepickers synchronized, such that a user could
not set the start date to something greater than the end date. We do that in the
onChangeMonthYear handler, re-setting either start’s maxDate or end’s
minDate after a change to the end or the start date, respectively.

Problem

The prev and next arrows worked just fine, as they are decrement/increment
handlers only. But we noticed that sometimes the onChangeMonthYear handler
was called multiple times when we changed a date by using the month and year
select elements. For example, we’d change the start year, then change the
end year. On that second change, we’d see the update to the start date’s
maxDate – which seemed to trigger a onChangeMonthYear call to the start
datepicker again, even though the start element had not been touched.
The result was that date ranges became unpredictable, even un-navigable.

Fix

It turns out the onChangeMonthYear event results only in re-drawing the
calendar – it does not calculate a new date selection.

Translation: month and year are navigation controls; the calendar’s date links are the selection controls.

The expected behavior in the datepicker is that users select a date link inside
the calendar. Because we are not using the calendar, we have to re-set the date,
in code, on the datepicker for which the onChangeMonthYear handler is called –
every time.

You can see most of the code fragments necessary to make the whole thing go by
looking at the other files posted in this gist. Here’s the condensed version of
the JavaScript we used:

onChangeMonthYear: function (year, month, instance) {
  ...

  // Build a new Date from the display date, decrementing the display
  // month by 1 for use in JavaScript's built-in Date().
  var newDate = new Date(year, Number(month) - 1, 1);

  // This one line prevents the buggy behavior noted above.
  $(instance.input.context).datepicker('setDate', newDate);

  ...
}

This took quite a bit of time to reason out, but that’s all there was to it.

3rd-party code still needs tests

[ original gist 28 March 2013 ]

“If there are no tests, it does not work,” a former colleague said, who could have added, “whether your code or someone else’s.”

I’m not the first to say it but it needs re-stating: 3rd-party libraries are no guarantee that your code continues to behave as expected.

If one of the duties of your work is making sure you’re not introducing bloat or cruft or inefficiencies or bugs or unexpected behavior, then you’re cutting corners if you don’t have tests, whether you use 3rd-party code or not.

Continue reading