9
\$\begingroup\$

Here is my solution to a series of problems in JS fundamentals. I've made it passing but I know this could be more efficient. I'm not looking for anyone to rewrite the code, but just asking what methods should be used when writing similar code in the future. Thanks.

The goal is basically to input two parameters, both have to be positive numbers and typeof numbers otherwise return ERROR. As well form an array that adds said two integers and all numbers between them. For example: if you enter 1,4 the answer would compute 1 + 2 + 3 + 4 = 10, and return 10.

const sumAll = function(lowEnd, highEnd) {
    let total = [];
    let sum = 0;
    let first = lowEnd;
    let second = highEnd;
    if (typeof first === 'number' && typeof second === 'number') {
        if (lowEnd >= 0 && highEnd >= 0) {    
            if (lowEnd <= highEnd){
                for (let i = lowEnd; i <= highEnd; i++) {
                    total.push(i);  
                }
            } 
            else {
                for (let i = highEnd; i <= lowEnd; i++) {
                    total.push(i);  
                }
            }
            for (let i = 0; i < total.length; i++) {
                sum += total[i];
            };
            return sum;
        } 
        else if (lowEnd < 0 || highEnd < 0)
            return 'ERROR';
    } 
    else {
        return 'ERROR';
    }
}

and here are the tests,

const sumAll = require('./sumAll')

describe('sumAll', function() {
  it('sums numbers within the range', function() {
    expect(sumAll(1, 4)).toEqual(10);
  });
  it('works with large numbers', function() {
    expect(sumAll(1, 4000)).toEqual(8002000);
  });
  it('works with larger number first', function() {
    expect(sumAll(123, 1)).toEqual(7626);
  });
  it('returns ERROR with negative numbers', function() {
    expect(sumAll(-10, 4)).toEqual('ERROR');
  });
  it('returns ERROR with non-number parameters', function() {
    expect(sumAll(10, "90")).toEqual('ERROR');
  });
  it('returns ERROR with non-number parameters', function() {
    expect(sumAll(10, [90, 1])).toEqual('ERROR');
  });
});
\$\endgroup\$
  • 2
    \$\begingroup\$ You can consider using indentation for your if-satements and maybe using conditional operators to reduce the amount of them needed. This task can also be solved using a formula (see this answer I recently wrote if you're curious) \$\endgroup\$ – Nick Parsons Jul 16 at 6:54
  • 3
    \$\begingroup\$ As an overall remark - please add appropriate nesting. It's very hard to see where each if statement ends and thus which piece of code belongs in which block. Something that's crucial to understanding how the function works. \$\endgroup\$ – VLAZ Jul 16 at 6:54
  • 2
    \$\begingroup\$ @NickParsons just as an addition to that - I also touch upon arithmetic progression in my post here. \$\endgroup\$ – VLAZ Jul 16 at 6:57
  • 2
    \$\begingroup\$ The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Jul 16 at 7:26
  • 2
    \$\begingroup\$ Re efficiency: how about return (hiEnd - lowEnd + 1) * (hiEnd + lowEnd) / 2? \$\endgroup\$ – vnp Jul 16 at 16:11
8
\$\begingroup\$

A good addition is to add a check at the start of your function to check if your parameters are correct. This way you don't need extra if statements which helps for readability.

const isValidNum = (n) => typeof n === "number" && n >= 0;

const sumAll = function(lowEnd, highEnd) {
  if (!isValidNum(lowEnd) || !isValidNum(highEnd)) return "ERROR";

  let total = [];
  let sum = 0;

  if (lowEnd <= highEnd) {
    for (let i = lowEnd; i <= highEnd; i++) {
      total.push(i);
    }
  } else {
    for (let i = highEnd; i <= lowEnd; i++) {
      total.push(i);
    }
  }

  for (let i = 0; i < total.length; i++) {
    sum += total[i];
  };

  return sum;
}

console.log(sumAll(1, 4));
console.log(sumAll(4, 1));
console.log(sumAll("1", 4));
console.log(sumAll(1, "4"));
console.log(sumAll(1, -4));
console.log(sumAll(-1, -4));

Another option is to check for the highest and lowest number via Math.min() and Math.max(). This way the order of input doesn't matter and you don't have to write the loop twice.

const isValidNum = (n) => typeof n === "number" && n >= 0;

const sumAll = function(lowEnd, highEnd) {
  if (!isValidNum(lowEnd) || !isValidNum(highEnd)) return "ERROR";

  let total = [];
  let sum = 0;
  
  const lowest = Math.min(lowEnd, highEnd);
  const highest = Math.max(lowEnd, highEnd);

  for (let i = lowest; i <= highest; i++) {
    total.push(i);
  }

  for (let i = 0; i < total.length; i++) {
    sum += total[i];
  };

  return sum;
}

console.log(sumAll(1, 4));
console.log(sumAll(4, 1));
console.log(sumAll("1", 4));
console.log(sumAll(1, "4"));
console.log(sumAll(1, -4));
console.log(sumAll(-1, -4));

At last you could add a formula to calculate the total sum of a range of numbers. Which removes the need of loops. (More info about the formula here)

const isValidNum = (n) => typeof n === "number" && n >= 0;

const sumRange = (low, high) => ((high - low + 1) * (low + high)) / 2;

const sumAll = function(lowEnd, highEnd) {
  if (!isValidNum(lowEnd) || !isValidNum(highEnd)) return "ERROR";
  
  const lowest = Math.min(lowEnd, highEnd);
  const highest = Math.max(lowEnd, highEnd);

  return sumRange(lowest, highest);
}

console.log(sumAll(1, 4));
console.log(sumAll(4, 1));
console.log(sumAll("1", 4));
console.log(sumAll(1, "4"));
console.log(sumAll(1, -4));
console.log(sumAll(-1, -4));

Total revision with comments:

// Return true if "n" is positive and of type number, else return false
const isValidNum = (n) => typeof n === "number" && n >= 0;

// Formula for adding a range of numbers
const sumRange = (low, high) => ((high - low + 1) * (low + high)) / 2;

function sumAll(lowEnd, highEnd) {
  // Return "ERROR" if arguments are invalid
  if (!isValidNum(lowEnd) || !isValidNum(highEnd)) return "ERROR";

  // Find highest and lowest number
  const lowest = Math.min(lowEnd, highEnd);
  const highest = Math.max(lowEnd, highEnd);

  // Return added numbers
  return sumRange(lowest, highest);
}

console.log(sumAll(1, 4));
console.log(sumAll(4, 1));
console.log(sumAll("1", 4));
console.log(sumAll(1, "4"));
console.log(sumAll(1, -4));
console.log(sumAll(-1, -4));

PS: I really like the way your tests are written. nicely clean and concise.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ As a side-note - in the cases when I need to take two parameters and then get the highest/lowest from those, I tend to do it in one line with destructuring const [min, max] = [param1, param2].sort((a, b) => a - b). Using a sort is slightly over the top but I find a single line to be better than two. It's also mostly my preference not that big of an improvement but I wanted to mention it. It does become more useful if you have three inputs and you want low, high and middle one. \$\endgroup\$ – VLAZ Jul 16 at 9:15
  • 1
    \$\begingroup\$ Thanks for the reply! These are not my tests, I got them from a free source webdev project called Odin. ie TheOdinProject. Take a gander if you're bored! \$\endgroup\$ – James Ross Jul 16 at 10:16
  • \$\begingroup\$ just a tiny nitpick but the first sentence has "add" instead of "at" :^) \$\endgroup\$ – adrian Jul 16 at 23:35
  • \$\begingroup\$ @VLAZ A way to avoid the sort could be: const min = Math.min(param1, param2), max = param1 + param2 - min; \$\endgroup\$ – RoToRa Jul 17 at 12:05
  • \$\begingroup\$ @RoToRa param1 = Number.MAX_SAFE_INTEGER will break that. Or if those are floating point numbers, as well, so it's a lot more error prone. I don't think the sort is that big of an issue over all - it's going to swap two items at most. With three items it might do two swaps but it at least ensures correctness. The performance benefit is negligible. \$\endgroup\$ – VLAZ Jul 17 at 13:19
2
\$\begingroup\$

My revision:

const sumAll = function(lowEnd, highEnd) {
    let sum = 0;
    if (lowEnd >= 0 && highEnd >= 0) {
        const start = Math.min(lowEnd, highEnd);
        const end = Math.max(lowEnd, highEnd);

        for(let i = start; i <= end; i++) {
            sum += i;
        }
        return sum;
    } 
    return 'ERROR';
}

Some comments:

  • typeof is not necessary if comparing >= or ===

  • the total array is useless as you can just sum in one for cycle

  • Math.min and Math.max can remove the extra logic of chosing the right ending

  • the two return 'Error' can be joined

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks. Simple and to the point. \$\endgroup\$ – James Ross Jul 16 at 10:14
  • \$\begingroup\$ Try sumAll(true,true), it returns 1 instead of 'ERROR'. Still like your answer though ;) \$\endgroup\$ – konijn Jul 16 at 10:36
1
\$\begingroup\$

I will review just one small aspect of the entire question.

You have return 'ERROR';, which is ... confusing.

On the languages I'm familiar with, there are 2 ways to handle errors:

  1. Return null
  2. Throw an exception

You do option #3: return something unexpected with a different type than what you're expecting.

Instead, I suggest that you do this:

throw new TypeError('Both values must be numbers');

The TypeError exception is clear on what the problem is: the type of some value is wrong.

This way, it's easier to handle and can be caught by a try...catch block.


A tiny nitpick: your function is called sumAll. But sum all? What's this "all"?

I suggest the name sumRange or similar, with the variables start and end.

lowEnd and highEnd sounds like you're talking about the lower or higher bits from a single number that was split to fit into smaller values (like a 16-bit number into 2 8-bit variables).


Another tiny nitpick, why don't you do function sumAll(...) instead of const sumAll = function(...)?

This created an unnamed anonymous function, while sumAll creates a named function that belongs in the current scope, and can be called before it is declared, as long as it is available in the same scope (also known as "hoisting").

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Hey appreciate the nitpicks! The reason for a couple of these questions can be answered by the fact this is a programming challenge, and there were sets of instructions given that ask for such things as sumAll and ERROR to be used. Noted your point on anything that wasn't part of the instructions! \$\endgroup\$ – James Ross Jul 17 at 1:58
1
\$\begingroup\$

Others have already mentioned great simplifications like returning early, using Math.min() and Math.max(). The code already uses const for the function sumAll and that is great. const can also be used for any variable that should not be re-assigned - e.g. total. While the variable can still be mutated (e.g. with Array.push()) it prevents accidental re-assignment.

The last for loop could be optimized by saving total.length in a variable in the initialization step, and has an excess an excess semi-colon after the closing brace:

for (let i = 0; i < total.length; i++) {
    sum += total[i];
};
//^ no need for semi-colon here, 
//  though it doesn't hurt

The length property can be stored in a temporary variable to reduce property lookups on each iteration, making it run faster in some browsers:

for (let i = 0, limit = total.length; i < limit; i++) {

Better yet, since the order of iteration doesn't matter, start i at total.length and decrease it to 0:

for (let i = total.length; i--; ) {

notice that the third condition is intentionally blank because the loop condition contains the operation to decrement i and there is no need to have a post-loop condition.

One could also use a functional approach (e.g. with const sum = total.reduce((a, b) => a + b ) but that would likely be slower because of the iterator functions.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Well, if you just want to get the sum, you can simply do this:

const sumAll = (lowEnd, highEnd) =>
  (lowEnd + highEnd) * ((Math.abs(highEnd - lowEnd)) + 1) / 2;

However, if you still want to sum them one by one, I prefer to create a function called range first .

function range (start, end) {
  return Array.from({ length: end - start + 1 }, () => start).map((v, i) => v + i);
}

Then create a function to sum the range:

function sumRange (start, end) {
  return range(start, end).reduce((a, b) => a + b);
}

But what if end is smaller than start? Let's fix it:

function sumRange (start, end) {
  if (end < start) return sumRange(end, start);
  return range(start, end).reduce((a, b) => a + b);
}

But what if params are not valid number? Let's create a function to do this:

function isValidNumber (num) {
  return typeof num === 'number' && num >= 0;
}

I don't want to hardcode it into sumRange, so I create a high order function:

function checkParams (validFn) {
  return fn => {
    return (...args) => {
      if (args.some(v => !validFn(v))) throw new Error('error');
      return fn.apply(null, args);
    } 
  }
}

Finally we can get sumAll:

const sumAll = checkParams(isValidNumber)(sumRange);

Put them together:

function checkParams (validFn) {
  return fn => (...args) => {
    if (args.some(v => !validFn(v))) throw new Error('error');
    return fn.apply(null, args);
  }
}

function isValidNumber (num) {
  return typeof num === 'number' && num >= 0;
}

// you can simply replace `range` and `sumRange` with 
// const sumRange = (lowEnd, highEnd) =>
//   (lowEnd + highEnd) * ((Math.abs(highEnd - lowEnd)) + 1) / 2;

function range (start, end) {
  return Array.from({ length: end - start + 1 }, () => start).map((v, i) => v + i);
}

function sumRange (start, end) {
  if (end < start) return sumRange(end, start);
  return range(start, end).reduce((a, b) => a + b);
}

const sumAll = checkParams(isValidNumber)(sumRange);

sumAll(1, 4); // 10
sumAll(1, 4000); // 8002000
sumAll(123, 1); // 7626
sumAll(-10, 4); // Error
sumAll(10, '90'); // Error
sumAll(10, [90, 1]); // Error
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

what methods should be used when writing similar code in the future. Thanks.

I recommend taking a more declarative approach. It's more concise, and easier to read. As explained here:

imperative code is where you explicitly spell out each step of how you want something done, whereas with declarative code you merely say what it is that you want done. In modern JavaScript, that most often boils down to preferring some of the late-model methods of Array and Object over loops with bodies that do a lot of comparison and state-keeping.

For example:

const sumAll = (a, b) => {
  const [from, to] = [a, b].sort();
  return ([from, to].some(n => typeof n != 'number' || n < 0))
    ? 'ERROR'
    : new Array(to - from + 1)
      .fill()
      .map((_, i) => from + i)
      .reduce((sum, n) => !(sum += n) || sum);
}
| improve this answer | |
\$\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.