1
\$\begingroup\$

This is the "classic" algorithm for calculating the sum of digits uses the modulo-operator together with the division-operator. I had the idea of using the slice-method with a negative index for accomplishing the task.

/**
 * Calculates sum of digits from a given number.
 * @param { number } num
 * @return { number }
 * @throws { exception } Throws exception in
 *   case given parameter is not a number.
 * Usage example:
 *   var CheckSum = calcCheckSum(328); // Returns 13
 */
function calcCheckSum(num) {
  var ret = 0;
  var i;

  if (typeof num != 'number' || isNaN(num)) {
    throw new Error('Number expected. { ' +
      typeof num + ' } ' +
      num + ' found.');
  }

  num = num.toString();

  for (i = num.length; i > 0; i--) {
    ret += parseInt(num.slice(i - 1, i));
  }

  return ret;
}

// -- TESTS -----------
for (var i = 0; i < 10; i++) {
  var randomNumber =
    Math.floor(
      Math.random() * (100000 - 100)
    ) + 100;

  var spacer = new Array(6);
  
  spacer = spacer.join(' ');

  console.log('%s : %s => %s',
               ('0' + i).slice(-2),
               (spacer + randomNumber).slice(-5),
               calcCheckSum(randomNumber)
  );
}

try {
  console.log(calcCheckSum('abcdef'));
} catch (e) {
  console.error(e);
}

try {
  console.log(calcCheckSum(NaN));
} catch (e) {
  console.error(e);
}
// ------------------------------

It seems to work as expected. Nevertheless I would appreciate your opinions: shall I stay with the well-known algorithm or I can use my self-figured out solution?

Moreover, is my parameter validation and my documentation done well? Or do I have to improve something?

\$\endgroup\$
4
  • \$\begingroup\$ Your error shoud say: 'Valid number expected...', instead of 'String expected...', as NaN is a number too, but not valid. \$\endgroup\$ – Mario Santini Nov 13 '16 at 10:55
  • \$\begingroup\$ "Shall I stay with the well-known algorithm or I can use my self-figured out solution?" It all depends on your use case. Correct me if I'm wrong, but the code looks like a toy/learning project, in which case it doesn't matter much at all. Try both! Performance-wise, arithmetic should be faster than string manipulation though. \$\endgroup\$ – jacwah Nov 13 '16 at 10:56
  • \$\begingroup\$ @MarioSantini You're right indeed. I've corrected it. Thanks a lot. :) \$\endgroup\$ – michael.zech Nov 13 '16 at 11:00
  • \$\begingroup\$ @jacwah I'm not really familiar with performance issue. But as far as I know are strings manipulation always "costly". In so far: You're right. Thanks for the hint. \$\endgroup\$ – michael.zech Nov 13 '16 at 11:04
1
\$\begingroup\$

Poor testing and vetting

There are a few problems with your approch.

  • Throwing an error seems to me to be the incorrect response. If the argument it not a number then the result should be NaN. Saves you having to wrap the function in a try catch which will basically stop the javascript engine from optimising the code.

  • Slice is a little over kill. You can use direct indexing str[index] or via str.charAt(index).

  • You had parseInt, which will remove any numbers after the decimal point. Use type coercion +num to convert from string to number and ""+num to convert to string.

  • You don't vet for exponent notation 123e-20

  • You don't vet - out of the number string and +,or .

  • And your testing is very poor.

Rewrite

Below is a rewrite. It fails one test calcCheckSum("0006") should return NaN but it does not which means that isNaN("0006") is failing. This was a surprise to me and I can not make a decision on what to do untill I work out the extent of this bug.

 function calcCheckSum (num) {
    var ret, i;
    if (isNaN(num) || num === "") { // Could be a string that is a number. No need to check if its a type number
                                    // Why throw this will stop the whole program. Return a value that reflects the input
        return NaN;
    }
    // what about negatives,pluses, decimal points, exponent notation, fractions????
    num = (""+num).replace(/\+|\-|\./g,""); // remove junk if any
    if(num.indexOf("e") > -1){  // if exponent then remove it
        num = num.split("e")[0];
    }
    
    ret = 0;
    for (i = 0; i < num.length; i ++) {  // simpler as no need to -1 each iteration
        ret += +num[i];  // + does the type conversion. Split is expensive so just lookup char by index
    }
    return ret;
}

// -- TESTS -----------
var fails = [];
var valuesToReturnNAN = [NaN,"0006","a","0xff","","+",".","-","-0-","-.38.",true,false,Infinity,-Infinity];
var valuesToReturn6 = [123,6,1.23e20,1.23e-20,111111,"123","6","+6","-6","-.6","111111","111.111",".123",.123,0.123,123.,-123,-.123,+123.,+123.0,-123.0,0.123,"0.12300"];
valuesToReturnNAN.forEach(v=>{
    if( ! isNaN(calcCheckSum(v)) ){
        fails.push("failed : " +(typeof v) + " : (" + v + ") did not return NaN");
    }
});
valuesToReturn6.forEach(v=>{
    if( calcCheckSum(v) !== 6 ){
        fails.push("failed : " +(typeof v) + " : (" + v + ") did not return 6");
    }
});
console.log("Failed "+fails.length + " tests.");
fails.forEach(str=>console.log(str));

\$\endgroup\$
0

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.