2
\$\begingroup\$

I would like to improve everything that can be improved. I don't understand regex a lot and what could be wrong. The main improve that I am looking for is related with errors. I want make the code robust to deal well with all kinds of errors. I could try to run in production and wait to see the errors, but I can't. At my company, I need to try to imagine all possible scenarios, good or bad.

Codepen

'use strict';

var sum = function(text, regex) {

  var match;
  var regex = regex || /(\d+)/g;

  var result = []
  while (match = regex.exec(text)) {
    result.push(match[1]);
  }

  return result.reduce(function(prev, curr) {
    return parseInt(prev) + parseInt(curr);
  }, 0);
};

console.log(sum('1 12 40'));
console.log(sum('10 12 40'));
console.log(sum('1 12 510'));
console.log(sum('LLLLLLLL'));
console.log(sum('10 1'));
\$\endgroup\$
  • \$\begingroup\$ Is the first parseInt(prev) necessary? I mean: You start with a integer (0) and then add integer to it. So all elements already in the return array can be expected to be integer. \$\endgroup\$ – michael.zech Mar 18 '16 at 7:26
  • \$\begingroup\$ Maybe not, you are right! \$\endgroup\$ – FXux Mar 18 '16 at 12:47
2
\$\begingroup\$

Numbers or integers ?

First of all, I notice that your question title talks about numbers, while your code looks only for positive integers.

In other words, it's currently not intended to take in account numbers such as 12.34 or -123.
To also detect and use this kind of numbers, your function would need a much more complex process.

Let's say you really want to remain limited to integers (I think so especially because of your parseInts). Then you may simplify your code in several ways.

Utility of regexargument ?

First I don't see why you're using a regex argument: for the purpose of your function, seems that it can't be anything else than the default you gave to it.
But this is up to you, and I'll keep this argument in what follows.

Declaring regex variable.

So just regarding it, you don't need to declare a new variable to use it: the regex argument your received has its scope limited to the function, and you may modify it directly. So you can simply write:

regex || /(\d+)/g;

Choosing the more efficient regex expression

Now regarding the way you expressed the regex: with its capturing parentheses, /(\d+)/g it makes your regex.exec(text) to return only one matching block each time, forcing you to the while() where you extract it.

At the opposite, using /\d+/g, you can merely do text.match(regex) and it'll return all matching blocks at once. Then you can directly reduce() this resulting array, though having to take care of the case it's null (like with your 'LLLLL').

Casting to numbers

While reduce() works, passed blocks contain only digits but are strings, so you're right to ensure they're processed as number instead.
But rather than parseInt (really useful only to extract integer part from a string where there are also other characters), you can merely cast curr to a number with +curr.

Using reduce().

You can take advantage of this reduce() capability: if you omit its 2nd argument (initial value for prev), then the 1st step is that curr is automatically affected to prev.
Note that in this case you must alos cast +prev!

Final reduced suggestion.

Last point, you can reduce the whole code by directly using the match() result in the reduce() process, without having to use other intermediate variables, assumed you take care of the possible null result from match().

Here is the suggested version:

'use strict';

var sum = function(text, regex) {
  var match;
  return !!(match = text.match(regex || /\d+/g)) ?
    match.reduce(function(prev, curr) {
      return +prev + +curr;
    })
  : 0;
}

console.log(sum('1 12 40'));
console.log(sum('10 12 40'));
console.log(sum('1 12 510'));
console.log(sum('LLLLLLLL'));
console.log(sum('10 1'));

\$\endgroup\$
  • \$\begingroup\$ First, thank you for your answer. I'm wrong, i would like to use numbers, not just integers @cFreed. \$\endgroup\$ – FXux Mar 19 '16 at 1:52
  • \$\begingroup\$ @FátimaAlves Wow... that's need a quite different answer! And so you'd probably better asking a new question. \$\endgroup\$ – cFreed Mar 19 '16 at 1:59
  • \$\begingroup\$ Really? Oh :(. I didn't know about the parseInt. I will try formulate a new answer with this new code. \$\endgroup\$ – FXux Mar 19 '16 at 2:03
  • \$\begingroup\$ I can't ask anymore. I am seeing this this message that i can't ask anymore. Should i just change parseInt for parseFloat? \$\endgroup\$ – FXux Mar 19 '16 at 2:04
  • \$\begingroup\$ @FátimaAlves From the SO point of view, and to let things clear for users, it must be a different post (not polluted by the answers this one already got). More over, it can't be posted on Code Review, but rather Stack Overflow because now you don't have working code but are looking for how to do it! Since you accepted my answer (BTW, thanks), your new question might be based on this current solution, clearly stating that it works for integers and you're looking for a way to extend it to all kinds of numbers. \$\endgroup\$ – cFreed Mar 19 '16 at 2:12
0
\$\begingroup\$

If you want to try and look for errors (without actually waiting for errors), you could use the try {} catch (error) {} function:

try {
    // run your regular expression code
} catch(error) {
    console.error("Error: " + error);
}

With this, you don't have to worry about waiting for errors to pop out [of nowhere] and you can just run, thinking if there will be errors in your program.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.