Skip to content

Number: Refactor formatter to extract lots of variables #484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jzaefferer wants to merge 1 commit into from
Closed

Number: Refactor formatter to extract lots of variables #484

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
55 changes: 29 additions & 26 deletions src/number.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,33 @@ define([
numberFormatterFn, numberFormatProperties, numberNumberingSystem, numberParserFn,
numberParseProperties, numberPattern, numberSymbol, stringPad ) {

function validateDigits( properties ) {
var minimumIntegerDigits = properties[ 2 ];
var minimumFractionDigits = properties[ 3 ];
var maximumFractionDigits = properties[ 4 ];

var minimumSignificantDigits = properties[ 5 ];
var maximumSignificantDigits = properties[ 6 ];
Copy link
Member

@rxaviers rxaviers Aug 24, 2015

Choose a reason for hiding this comment

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

Coding style now allows onevar: false, which I personally like too. Although, the rest of the code still uses onevar: true. For consistency sake, I suggest this to be updated to onevar: true until #413 lands.

Copy link
Contributor Author

@jzaefferer jzaefferer Aug 24, 2015

Choose a reason for hiding this comment

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

We've been doing gradual migrations elsewhere. I see no point introducing new code with the old style just for consistency. Anything new shouldn't use onevar is easy enough to apply.

Copy link
Member

@rxaviers rxaviers Aug 24, 2015

Choose a reason for hiding this comment

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

Where?

Copy link
Contributor Author

@jzaefferer jzaefferer Aug 24, 2015

Choose a reason for hiding this comment

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

For example:

jquery/jquery-ui@803eaf2#diff-29e4c930be69ff962e7aee3f71878229R144
qunitjs/qunit@eaae69b#diff-586f09c9d3a303fcf4f478bd0b1d5ca4R1

Copy link
Member

@rxaviers rxaviers Aug 24, 2015

Choose a reason for hiding this comment

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

Well, the examples are from different projects. I wouldn't encourage having inconsistent code.

If it helps, feel free to cherry-pick rxaviers@26f7e10.

Copy link
Contributor Author

@jzaefferer jzaefferer Aug 24, 2015

Choose a reason for hiding this comment

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

I don't really mind, feel free to land this with your fixup commit.


// Validate significant digit format properties
if ( !isNaN( minimumSignificantDigits * maximumSignificantDigits ) ) {
validateParameterRange( minimumSignificantDigits, "minimumSignificantDigits", 1, 21 );
validateParameterRange( maximumSignificantDigits, "maximumSignificantDigits",
minimumSignificantDigits, 21 );

} else if ( !isNaN( minimumSignificantDigits ) || !isNaN( maximumSignificantDigits ) ) {
throw new Error( "Neither or both the minimum and maximum significant digits must be " +
"present" );

// Validate integer and fractional format
} else {
validateParameterRange( minimumIntegerDigits, "minimumIntegerDigits", 1, 21 );
validateParameterRange( minimumFractionDigits, "minimumFractionDigits", 0, 20 );
validateParameterRange( maximumFractionDigits, "maximumFractionDigits",
minimumFractionDigits, 20 );
}
}

/**
* .numberFormatter( [options] )
*
Expand All @@ -38,8 +65,7 @@ define([
*/
Globalize.numberFormatter =
Globalize.prototype.numberFormatter = function( options ) {
var args, cldr, maximumFractionDigits, maximumSignificantDigits, minimumFractionDigits,
minimumIntegerDigits, minimumSignificantDigits, pattern, properties, returnFn;
var args, cldr, pattern, properties, returnFn;

validateParameterTypePlainObject( options, "options" );

Expand All @@ -62,30 +88,7 @@ Globalize.prototype.numberFormatter = function( options ) {

cldr.off( "get", validateCldr );

minimumIntegerDigits = properties[ 2 ];
minimumFractionDigits = properties[ 3 ];
maximumFractionDigits = properties[ 4 ];

minimumSignificantDigits = properties[ 5 ];
maximumSignificantDigits = properties[ 6 ];

// Validate significant digit format properties
if ( !isNaN( minimumSignificantDigits * maximumSignificantDigits ) ) {
validateParameterRange( minimumSignificantDigits, "minimumSignificantDigits", 1, 21 );
validateParameterRange( maximumSignificantDigits, "maximumSignificantDigits",
minimumSignificantDigits, 21 );

} else if ( !isNaN( minimumSignificantDigits ) || !isNaN( maximumSignificantDigits ) ) {
throw new Error( "Neither or both the minimum and maximum significant digits must be " +
"present" );

// Validate integer and fractional format
} else {
validateParameterRange( minimumIntegerDigits, "minimumIntegerDigits", 1, 21 );
validateParameterRange( minimumFractionDigits, "minimumFractionDigits", 0, 20 );
validateParameterRange( maximumFractionDigits, "maximumFractionDigits",
minimumFractionDigits, 20 );
}
validateDigits( properties );

returnFn = numberFormatterFn( properties );

Expand Down