Relative relationships

This is a question in Udacity JS course. I am starting to like Javascript. I feel its a misunderstood language :)


We learned about relational operators and how they can classify the
relationship between two values. Your job is to write the function
getRelationship(x,y) function, which should return a string
representing whether x is >, < or = y. For example:

var rel = getRelationship(2, 3);
console.log(rel); // <

If one or both of the values aren't numbers, your function should
output:



"Can't compare relationships because [this value] and [that value] [is]/[are] not [a] number[s]."



where [this value] and [that value] are replaced with the
non-numerical values. The sentence should be grammatically correct by
outputting either is or are and pluralizing number if necessary.


For example:

var rel1 = getRelationship("this", 2);
console.log(rel1); // "Can't compare relationships because this is not a number"

var rel2 = getRelationship("that");
console.log(rel2) // "Can't compare relationships because that and undefined are not numbers"

Notice in the second example, because the y value was missing, the
output said that undefined was not a number.

Here is my stab at the solution.

```
function getRelationship(x, y) {
var respObj = validateArgAsNumber(x,y);
if (respObj.x && respObj.y && !respObj.isXNaN && !respObj.isYNaN) {
console.log("Both are numbers");
if (x > y) {
return ">";
} else if (x < y) {
return "<";
} else {
return "=";
}
} else {
console.log("Both are NOT numbers");
//console.log(respObj);
if ((!respObj.x && !respObj.y) || (respObj.isXNaN && respObj.isYNaN)){
return "Can't compare relationships because " + x + " and " + y + " are not numbers";
}
if (!respObj.x || respObj.isXNaN) {
return "Can't compare relationships

I'm confused as to why you treat the results of isNaN and typeof as separate things, since failing either test would invalidate the argument.

So you could simplify validateArgAsNumber a lot if you just gave it one argument and let it return a simple boolean. Its name actually already implies only one argument, since it's called "validate arg", not "validate args". However, I'd rather call it isValidNumber - why care if it's an argument for something else?

function isValidNumber(value) {
    return typeof value === 'number' && !Number.isNaN(value);
}

If typeof value isn't 'number', we immediately know something's wrong. The isNaN check is only necessary because JavaScript has this funny thing about NaN itself being a number (it's a long story and has to do with IEEE-754 64-bit floating point numbers).

From there, the rest of the code becomes a lot simpler, since you don't have to worry about checking two separate booleans each time you want to check one argument.

I'd also be tempted to simply make the validation function an inner function in getRelationship, since its purpose is somewhat specific to that task.

Of course, the function is now so simple that it almost doesn't need to be a function, but let's roll with it. It avoids a trivial bit of repetition.

With the above you might get something like this:

function getRelationship(x, y) {
  function isValidNumber(value) {
    return typeof value === 'number' && !Number.isNaN(value);
  }

  var xValid = isValidNumber(x),
      yValid = isValidNumber(y);

  if(!xValid && !yValid) {
    return "Can't compare relationships because " + x + " and " + y + " are not numbers";
  }

  if(!xValid || !yValid) {
    return "Can't compare relationships because " + (xValid ? y : x) + " is not a number";
  }

  if(x === y) {
    return '=';
  }

  return x ';
}

Note that I've combined the check for "x or y is invalid" into a single return. That way, I don't have to repeat a line of mostly-hardcoded text.

The last bit - after checking the arguments - can be handled in many ways. I chose the above, because it puts an explicit return on the last line, which I think reads well. It only returns early from inside a branch if it has to. But you could of course also use if..else if..else, or even a switch:

switch(true) {
  case x > y: return '>';
  case x < y: return '<';
  default: return '=';
}

But that's a little too tricky.

StackExchange Code Review Q#93243, answer score: 4