3
\$\begingroup\$

I am currently learning JavaScript and wanted to fellow coders to review a function that I had written. I am looking for feedback particularly on:

  • Whether I have followed best practice and JavaScript conventions
  • Any suggestion on doing it better.

Things I know I need to implement but don't know how:

  • Validate if array values are numbers

Purpose of function: Return all scores above a certain threshold. Function accepts two arguments. First is an array of scores and second argument is the threshold.

My code:

UPDATED code after putting it through JS lint - I have ignored some erros like moving vars in the loops to the top or not declaring vars in the for loop etc (before JSLint code is here http://jsbin.com/UtOMAyU/1/)

function scoresAbove(scoresArg, thresholdArg) {
    "use strict";
    var output = [],
        nums = scoresArg,
        threshold = thresholdArg;
    //check if scores Arg provided is an array
    if (typeof nums !== "object" || !nums.hasOwnProperty('length')) {
        return "Error: Number(s) not in the expected format. Please provid number(s) in an array format";
    }else if (typeof threshold !== "number") { //check if threshold Arg provided is an array
        return "Error: Threshold number not in the expected format. Please provid a threshold";
    }
    for (var i = 0, len = nums.length; i < len; i++) {
        var currentNum = nums[i];
        if (currentNum > threshold) {
            output.push(currentNum);
        }
    }
    //What to return from func
    if (output == "") {
        return "No scores were above the " + " threshold you set as " + threshold;
    }
    return "The following scores were above the threshold: " + output.join(", ");
}
\$\endgroup\$
  • \$\begingroup\$ JavaScript is weakly typed. The convention is to do duck typing: do what you want to do without checking typeof. If it works, great! If it causes an exception to be thrown, well, it was destined to fail anyway. There's usually not much sense in manually trying to turn it into a strongly typed language. Therefore, I'd skip your precondition checks. \$\endgroup\$ – 200_success Nov 6 '13 at 17:33
3
\$\begingroup\$

My 2 cents:

  • var output should probably be highScores or some such, since it never contains what is returned, nor does it get outputted

  • nums = scoresArg, threshold = thresholdArg; makes little sense, just use the arguments

  • function scoresAbove(scoresArg, thresholdArg) , I am not sure what the value is of postpending 'Arg' to the argument names, I would go for simply function scoresAbove(scores, threshold)

  • The check for Array should really be a separate function, it is completely re-usable

  • Ditto for Number

  • For the actual filtering , I would use the built-in filter :

    highScores = scores.filter( function(v){ return v > threshold });

  • Since output is an array, I would not compare with ""

  • Why the double concatenation here: return "No scores were above the " + " threshold you set as " + threshold;

  • Arguments of the wrong type should be rare enough that you can throw an exception since something is clearly wrong with the program.

You could try

function scoresAbove( scores, threshold ) 
{
  "use strict";
  var highScores;

  if( notArray( scores ) )
    throw "scores is not an array";
  if( notNumber( treshold ) )
    throw "treshold is not a number"

  highScores = scores.filter( function(v){ return v > threshold });

  if( !highScores || !highScores.length )
    return "No scores were above the threshold you set as " + threshold;

  return "The following scores were above the threshold: " + output.join(", ");
}

You would have to write the notNumber and notArray function of course. JSLint migh complain about missing curly braces, to each his own ;)

\$\endgroup\$
  • \$\begingroup\$ Thanks very much, a lot of actionable feedback and learnt couple of tricks such how to check any empty array (test if it returns the FALSE value, instead of checking for an empty string) and the whole idea of "throwing an exception" as it's something I need to read on and kind of avoided. \$\endgroup\$ – Abs Nov 7 '13 at 9:14
2
\$\begingroup\$

The only criticism I can think to make of this is that you could/should maybe return a success parameter to identify if an error occurred or not.

maybe:

var result = {
  success=true,
  message="The following scores were above the threshold: " + output.join(", ")
};

or

var result = {
  success=false,
  message="Error: Number(s) not in the expected format. Please provid number(s) in an array format"
};

then return result;. Allowing you to then do something like

var result = scoresAbove (scoresArg, thresholdArg);
if (result.success)
{

}
etc...

You could even then extend this object to include other information, maybe even build the string in the calling method. But this depends on your use case.

Apart form that I can't see any issues.

\$\endgroup\$
1
\$\begingroup\$

In addition to what has been said, I suggest:

1) Separation of concerns. You have two concerns. a) checking if any number at all is above the treshold; and if so, return a message and b) to do the acual calculation. I would recommend for that using two functions.

If you separate your concerns, there is no need for a success variable from the filtering: the result is clearly saying either you have an empty array or not. An "errorstate" is given by the exception.

2) Instead of returning "Error"-Messages you should use Exceptions. E.g.:

if(!Array.isArray(numbers)) throw new TypeError("numbers is not an Array");
\$\endgroup\$
  • \$\begingroup\$ Thanks. Going to read on the use of exceptions. It's not something am familiar with yet but from your feedback I will look into it. Also, I understand your point about "separation of concerns" and will review my code. \$\endgroup\$ – Abs Nov 7 '13 at 9:18

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.