HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Extraction of numbers from currency string

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numbersextractionfromstringcurrency

Problem

I have the following code; whereas 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 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 map

Instead 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
// -> 41


Ah, 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
// -> 41

Code 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.