patternjavascriptMinor
Extraction of numbers from currency string
Viewed 0 times
numbersextractionfromstringcurrency
Problem
I have the following code; whereas
In the currency variable, dots need to get eliminated and commas turned into floating points. I need to cater for cases when currency and views variables do not have a number in their string or do have a dash or a blank in them.
Would you say this script can be optimsed to be more efficient?
currency and views are in reality many JSON objects from an SAP export, that I can not rely upon to be in the same format. That is why I'm regexing over these inputs. My code looks clumsy to me. Can it be improved?var currency = "20.002,03 €"; // sometimes this String is "-" or " "
var views = "38"; // sometimes this String is "-" or " "
var reDigit = /[\d\,\.]+/g;
var map = {',':'.', '.':''};
if (typeof currency != 'undefined' && typeof views != 'undefined') {
currency = currency.match(reDigit);
views = views.match(reDigit);
if (currency != null && views != null ) {
currency = currency[0];
views = views[0];
currency = currency.replace(/[,.]/g, function(k) {
return map[k];
});
return currency / views;
} else {
return undefined;
};
} else {
return undefined;
}In the currency variable, dots need to get eliminated and commas turned into floating points. I need to cater for cases when currency and views variables do not have a number in their string or do have a dash or a blank in them.
Would you say this script can be optimsed to be more efficient?
Solution
Simplify the overall logical flow
Notice the multiple
These
It would be better to think of the logic as multiple levels of checks,
where you only return after all checks passed,
reaching the innermost level.
If at any checks fail,
execution drops out of the nested
More readable without the
Instead of this:
I would find it more readable to use two
You could even chain them together,
but that would be less readable.
Another reason why this is more readable is that in the original solution,
the
That can invite mistakes,
such as adding patterns in
or the other way around.
Naming
The word "digit" suggests
How about
That would make it clear that it's more than just digits.
Validation
Are you sure all inputs are valid?
The code will work and return values for clearly bad input too, such as:
For the last example it will return
Will all this be ok in your application?
Even if you think such inputs are impossible, I think you should check for them.
One day when you might corrupted inputs,
which, in the absence of solid defenses could propagate in your systems and manifest as strange problems that are extremely difficult to debug.
It's better to catch problems early,
as close to their source as possible.
The only way you can assume the inputs are always valid,
if they have been prevalidated before reaching this code.
And in that case, this code should be in a private method,
only callable from the place where the validation was performed,
to guarantee that it can't be called with invalid data.
Dividing strings
I noticed a bit late, but curiously, the code is actually dividing strings:
Ah, sweet JavaScript, only you can be capable of such madness.
You should use
to make them proper numbers.
Note that this will change some things.
For example:
Notice the multiple
else conditions in every if statement when some required condition is not true:if (...) {
if (...) {
return currency / views;
} else {
return undefined;
}
} else {
return undefined;
}These
else blocks are repetitive.It would be better to think of the logic as multiple levels of checks,
where you only return after all checks passed,
reaching the innermost level.
If at any checks fail,
execution drops out of the nested
if blocks and fall back to the return statement of the function at the end, like this:if (...) {
if (...) {
return currency / views;
}
}
return undefined;More readable without the
mapInstead of this:
currency = currency.replace(/[,.]/g, function(k) {
return map[k];
});I would find it more readable to use two
replace calls instead:currency = currency.replace(/\./g, '');
currency = currency.replace(/,/g, '.');You could even chain them together,
but that would be less readable.
Another reason why this is more readable is that in the original solution,
the
map is defined far away from where it's used.That can invite mistakes,
such as adding patterns in
map but forgetting to adjust the regex in the replace call,or the other way around.
Naming
reDigit is not a good name for /[\d\,\.]+/g.The word "digit" suggests
\d (0 ... 9), but you have more there.How about
reMoney or reMoneyDigits instead?That would make it clear that it's more than just digits.
Validation
Are you sure all inputs are valid?
The code will work and return values for clearly bad input too, such as:
var currency = "hello20.002,03 €";
var currency = "20.0.0..2,03 €";
var currency = "20.a0b0c2d,03 €";
var currency = "20.002,03... €";For the last example it will return
NaN.Will all this be ok in your application?
Even if you think such inputs are impossible, I think you should check for them.
One day when you might corrupted inputs,
which, in the absence of solid defenses could propagate in your systems and manifest as strange problems that are extremely difficult to debug.
It's better to catch problems early,
as close to their source as possible.
The only way you can assume the inputs are always valid,
if they have been prevalidated before reaching this code.
And in that case, this code should be in a private method,
only callable from the place where the validation was performed,
to guarantee that it can't be called with invalid data.
Dividing strings
I noticed a bit late, but curiously, the code is actually dividing strings:
'123' / 3
// -> 41Ah, sweet JavaScript, only you can be capable of such madness.
You should use
parseFloat for currency, and perhaps parseInt for view,to make them proper numbers.
Note that this will change some things.
For example:
'123..' / 3
// -> NaN
parseFloat('123..') / 3
// -> 41Code Snippets
if (...) {
if (...) {
return currency / views;
} else {
return undefined;
}
} else {
return undefined;
}if (...) {
if (...) {
return currency / views;
}
}
return undefined;currency = currency.replace(/[,.]/g, function(k) {
return map[k];
});currency = currency.replace(/\./g, '');
currency = currency.replace(/,/g, '.');var currency = "hello20.002,03 €";
var currency = "20.0.0..2,03 €";
var currency = "20.a0b0c2d,03 €";
var currency = "20.002,03... €";Context
StackExchange Code Review Q#69478, answer score: 3
Revisions (0)
No revisions yet.