Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(numberFilter): format large numbers correctly #8705

Closed
wants to merge 1 commit into from
Closed

fix(numberFilter): format large numbers correctly #8705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/ng/filter/filters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,16 @@ function numberFilter($locale) {
};
}

function shiftDecimalPlace(number,fractionSize) {
var numberArray = number.toString().split('e');
var fractionUsed = +fractionSize;
//If number was already an exponent, adjust the exponent value rather than adding new exponent.
if(numberArray[1]) {
fractionUsed = +numberArray[1] + fractionUsed;
}
return +(numberArray[0] + 'e' + fractionUsed);
}

var DECIMAL_SEP = '.';
function formatNumber(number, pattern, groupSep, decimalSep, fractionSize) {
if (number == null || !isFinite(number) || isObject(number)) return '';
Expand Down Expand Up @@ -151,7 +161,10 @@ function formatNumber(number, pattern, groupSep, decimalSep, fractionSize) {
// safely round numbers in JS without hitting imprecisions of floating-point arithmetics
// inspired by:
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/round
number = +(Math.round(+(number.toString() + 'e' + fractionSize)).toString() + 'e' + -fractionSize);
number = shiftDecimalPlace(Math.round(shiftDecimalPlace(number,fractionSize)),-fractionSize);
if(isNaN(number)) {
number = shiftDecimalPlace(shiftDecimalPlace(number,-fractionSize),fractionSize);
}

var fraction = ('' + number).split(DECIMAL_SEP);
var whole = fraction[0];
Expand Down
20 changes: 20 additions & 0 deletions test/ng/filter/filtersSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,26 @@ describe('filters', function() {
expect(num).toBe('1.1112');
});

it('should format large number',function() {
pattern.gsize = 2;
var num = formatNumber(12345868059685210000, pattern, ',', '.', 2);
expect(num).toBe('12,345,868,059,685,210,000.00');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, test some additional inputs --- try to find corner cases and make sure they work as you expect

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure will add additional test cases,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to write some additional test cases is when i found out that .. When we apply the number filter to 879832749374983274928 we get result 879,832,749,374,983,200,000.00

Which is not a correct result since all the digits after 16 digits is being converted to zero, which is wrong.

This is happening because
They are 64-bit floating point values, the largest exact integral value is 253, or 9007199254740992
any number greater than this number will get the zeros after 16 digits and there by loose precision at the least.
this loose of precision is outside of the scope of number filter

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one more test case i found, but this doesn't look like a valid case also:
8798327493749832749 | number:3342
If this is the filter.

Output returned will be NAN.000000000000000000000000000000000000000

Problem comes when we, try to shift the number by the facction size . fractionSize here is very high, so shifting yields a infinity. which goes and converts into NAN on further processing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like behaviour is also browser of javascript engine specific when it goes out of precision.

Travis build failed at:
IE 9.0.0 (Windows 7) filters formatNumber should format large number FAILED
Expected '879,832,749,374,983,100,000.00' to be '879,832,749,374,983,200,000.00'.

Chrome gets the result 879,832,749,374,983,200,000.00 and IE to 879,832,749,374,983,100,000.00

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall i just remove the testcase .. and this will be an acceptable and useful fix ?

for the case going to infinity, i just switched the order i was shifting forward and shifting backwards.

and moreover there was an unnecessary math.round function i removed it. now it seems like those cases are handled well..
Seems like some of the tests are failing now . i will look why are they failing , and are they legitimate failure. mostly they are failing because it expected 1.12 but returning 1.13

" Expected '1,234.5' to equal '1,234.6'.
Error: Expected '1,234.5' to equal '1,234.6'.
at null. (/home/gaurav/Work/Angular/myFork/angular.js/test/ng/filter/filtersSpec.js:167:35)
Expected '1,234.56' to equal '1,234.57'.
Error: Expected '1,234.56' to equal '1,234.57'.
at null. (/home/gaurav/Work/Angular/myFork/angular.js/test/ng/filter/filtersSpec.js:168:35)
Expected '1.2' to equal '1.3'.
Error: Expected '1.2' to equal '1.3'.
at null. (/home/gaurav/Work/Angular/myFork/angular.js/test/ng/filter/filtersSpec.js:170:35)
Expected '1.25' to equal '1.26'.
Error: Expected '1.25' to equal '1.26'.
"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall i just remove the testcase ..

no.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are there kind of cases handle in general.. is there a way to write test-cases for such scenario or handle these scenario more gracefully ? i have not worked on how to handle these corner cases . it will be great if you can guide me a little with this(maybe by providing pointers to places i can learn more or similar cases which are handled in angular) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
This is used in other tests

if (msie < 9) {
...
} else {
...
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank @lgalfaso i will try that out in my test case.

For the number going to infinity, when exponent is very high. i tried reversing the shift back and sift forward order so that number never reach infinity.
But there is issue in that because rounding of the number is not done correctly.

Example being: 1.11119 is converted to 1.1111 instead of its correct output 1.1112

So what i am planning to do is keep the logic as it is . but when the number goes to infinity and we get number as NAN i will again do shift back and shift forward in different order.
Will push the code in a while

num = formatNumber(79832749837498327498274983793234322432, pattern, ',', '.', 2);
expect(num).toBe('7.983274983749832e+37');
num = formatNumber(8798327498374983274928, pattern, ',', '.', 2);
expect(num).toBe('8.798327498374983e+21');
num = formatNumber(879832749374983274928, pattern, ',', '.', 2);
var msie = +((/msie (\d+)/.exec(navigator.userAgent.toLowerCase()) || [])[1]);
var msie11 = (/Trident.*7.0/.exec(navigator.userAgent.toLowerCase()));
if(msie || msie11) {
expect(num).toBe('879,832,749,374,983,100,000.00');
} else {
expect(num).toBe('879,832,749,374,983,200,000.00');
}
num = formatNumber(879832749374983274928, pattern, ',', '.', 32);
expect(num).toBe('879,832,749,374,983,200,000.00000000000000000000000000000000');
});

it('should format according different separators', function() {
var num = formatNumber(1234567.1, pattern, '.', ',', 2);
expect(num).toBe('1.234.567,10');
Expand Down