2
\$\begingroup\$

Forgive me if this is too simple, but I'm a beginner trying to learn and understand. I came across an exercise to create these three basic functions, and this is what I came up with.

Any glaring errors?


This program defines three functions:

  • sum(x,y)
  • range(x,y,step)
  • rangeSum (start, end)

sum will give you the sum of two numbers.

range will give you an array of numbers from x to y, with an optional step argument that dictates the increment to use when listing the range. If no step is given, the default value is 1.

rangeSum will give you the sum of a range of numbers from start to end.

// sum(x,y) takes the sum of two numbers
var sum = function(x,y){
  return x + y;
};
// range(x,y,step) gives an array of the range of x and y, with an optional step argument
var range = function(x, y, step){
    if (step == null) step = 1;
  var arr = [];

  if (step > 0) {
    for (var i = x; i <= y; i += step)
      arr.push(i);
  } else {
        for (var i = x; i >= y; i += step)
        arr.push(i);
  }
    return arr;
};
// rangeSum(start,end) gives the sum of the range from start to end.          
var rangeSum = function(start,end){
  var arr = [];
  for (var i = start; i <= end; i++) 
    arr.push(i);
  return arr.reduce(function(a,b){
    return a + b;   
  });
};
\$\endgroup\$
  • \$\begingroup\$ Is the indentation intentional or a messy copy/paste? \$\endgroup\$ – elclanrs Apr 2 '16 at 6:23
  • \$\begingroup\$ I'm still bad with the indentation, that's me doing that :O I'm going to fix it really quick. \$\endgroup\$ – Alex Gray Apr 2 '16 at 6:26
  • \$\begingroup\$ is that better? \$\endgroup\$ – Alex Gray Apr 2 '16 at 6:29
  • \$\begingroup\$ Still looks a bit wonky to me. \$\endgroup\$ – elclanrs Apr 2 '16 at 6:40
3
\$\begingroup\$

First of all, your code is pretty good. No big errors as you fear, there are comments, right indentation, anyone can understand it easily, and... it works !!!

Yet let me suggest a few improvements you could make to make it shine :

For all your functions : use a function declaration : it's easier to use since you can use them anywhere in your code (and not only after the var statement was actually evaluated), and the global namespace pollution is the same anyway.

The sum() function just doesn't make sense as it stands : Why one would use sum(i, 3) instead of i+3 ?
It makes sense again with one or both of those features : type checking or accepting any number of arguments.
To type-check or not to is a big debate i won't enter into. I did here since it might be a good way to learn to be cautious about types even in 'typeless' Javascript.
So following code does both type checking and accepting any number of arguments :

// returns the sum of its arguments (0 if none). 
// throws if any of them is not a number.
function sum() {
   var result = 0;
   for (var i=0; i<arguments.length; i++) {
      var thisNumber = arguments[i];
      if ((+thisNumber) !== thisNumber)  {
              throw('Error : argument ' + i + ' of sum is not a number ('+thisNumber + ').');
      }
      result+=thisNumber;
   }
   return result;
}

For range() : change the arguments name to start and end makes understanding the function goal easier.
To give step a default value, you don't want to test step against null, but rather against undefined. (it worked because because you used ==, but notice that by using '==null', you're just testing is ?? falsy ?, so the wrong input 0 for step will silently be changed to 1.).

You might want to check :
• that start, end, and step (if provided) are meaningful numbers (see above).
• that sign(end-start)==sign(step) so end might indeed be reached.
• that start-end is a multiple of step. Otherwise you don't get end in the range, which i find most annoying.
• that step is not === 0. In fact the previous check already checked that.

// returns an array filled with the range of numbers between start and end,
//   using a optional step that defaults to 1.
// throws if the range cannot be built with the provided arguments.
function range(start,end, step){
    // step defaults to 1
    if (step === undefined) step = 1;
    // type-check start, end, step here if you wish
    // ...
    // checking that step sign allows to reach the end
    if ((end-start)*step<0) {
       throw ('the sign of step does not allow to reach end value');
    }
    // checking that step value allows to reach the end value
    // btw will check that step !==0 also
    var stepCount = (end-start) / step;
    if ( stepCount - Math.floor(stepCount) !==0) { // division has remainder ?
       throw('cannot reach '+end+' starting from '+start+' with the step ' + step);
    }
  // build result array
  var result = [];
  for (var i = start; i != end ; i += step) {  result.push(i);  } 
  return result;
};

For rangeSum i was surprised to see that you didn't re-use the range function you just wrote : In fact you re-wrote a simple version of range.
I think only two options are relevant :
1) re-use range, and add an optional step argument for free.
2) if you just want to do a most simple range sum function, go for a for loop and sum by yourself.

I choosed (2) :

You might want to check :
• that start, end are meaningful numbers (see above).
• that that end can be reached with a step of 1 <==> that (start < end) and start, end are integers. ( x === 0 | x )

// returns the sum of numbers ranging from start to end(included).          
function rangeSum (start,end){
  var result = 0;
  // you might want to check some things here.
  for (var i = start; i <= end; i++) { result += i; }
  return result;
};

... But in fact, with just a little bit of math, you can write the simple version of rangeSum in one line : :-)

function rangeSum(start, end) {
  // you might want to check some things here.
  return (end + 1 - start)*( end + start)/2 ;
}

Edit :
To allow easier understanding :
1) I used Math.floor to floor the stepCount. ( you might forget about the 0 | stepCount dirty trick for now ). So when i write if ( stepCount - Math.floor(stepCount) !==0) i test if the division has a remainder.
2) ((+thisNumber) !== thisNumber) is a trick to test for thisNumber not being a number or NaN. How ?
a) On the left side, we convert thisNumber to a number, so since we use !==, thisNumber has to be from a different type from number => for now it's quite like testing if (typeof thisNumber === 'number').
b) But NaN qualifies as a number ( ... ... ), so since NaN !== NaN, the expression above does also consider thisNumber as wrong if its value is NaN.

\$\endgroup\$
  • \$\begingroup\$ Wow, thanks for the detailed response. It sure is humbling lol. There's some syntax I don't understand but I'm not sure if it's because I forgot reviewing it, or I haven't came across it yet. I'm going to go back to chapter 1 of Eloquent JavaScript and start to take some detailed notes. These were exercises from chapter 4 and I'm not very happy with how i did :P \$\endgroup\$ – Alex Gray Apr 2 '16 at 17:10
  • \$\begingroup\$ You are welcome. I edited to clarify a few things. \$\endgroup\$ – GameAlchemist Apr 2 '16 at 17:36
  • \$\begingroup\$ After going over your code and referring to the MDN for each part I didn't understand, I now have a much better understanding of everything. Thanks for pointing me in the right direction. \$\endgroup\$ – Alex Gray Apr 2 '16 at 18:36
4
\$\begingroup\$

The code looks fine but has a few flaws and can be improved. I didn't solve the flaws and change the code: I've left that as an excercise for you.

In general:

  • Why do you use function expressions instead of function declarations?
  • I recommend to always use braces, even for single line if statements or for loops. Omitting braces will bite you eventually.
  • What happens when the parameters of your functions are not numbers or are undefined?

Your code, with my comments added as review points:

// sum(x,y) takes the sum of two numbers
var sum = function(x,y){
    return x + y;
};

// range(x,y,step) gives an array of the range of x and y, with an optional step argument
var range = function(x, y, step){
    if (step == null) step = 1;

    // why do you call this variable arr? 
    // It's better to use a meaningful name like result.
    var arr = [];

    // what happens when y is less than x?
    // what happens when step equals zero?

    if (step > 0) {
        for (var i = x; i <= y; i += step)
            arr.push(i);
    } else {
        for (var i = x; i >= y; i += step)
            arr.push(i);
    }
    return arr;
};

// rangeSum(start,end) gives the sum of the range from start to end. 
// why do you use other argument names than in the range function?
// it's better to use consistent names.
// By the way: the names of these argument are better than the arguments
// of the range function, so I'd change those to start and end.         
var rangeSum = function(start,end){

    // why so complex? You can iterate over the range once and keep a running total.

    var arr = [];
    for (var i = start; i <= end; i++) 
        arr.push(i);
    return arr.reduce(function(a,b){
        return a + b;   
    });
};
\$\endgroup\$
  • \$\begingroup\$ Good points and nice way to help me learn on my own. Thanks for your review and exercise! :) \$\endgroup\$ – Alex Gray Apr 2 '16 at 17:07
  • \$\begingroup\$ Hey, I just finished going over your points and redoing the code. It really helped me to get a better understanding of everything. Your comments led me to using the MDN to find new operators/methods/syntax, etc. You can see the code as I've updated it thanks to you here: github.com/agraymd/beginnerMathFunctions/blob/master/… I figure it isn't worth making a new question. I know theres still more ways I can improve the code to accept more input and stuff, but I'm not trying to reinvent a calculator. Just learn. So thanks again. Much better than when I started! \$\endgroup\$ – Alex Gray Apr 2 '16 at 23:41

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.