30
\$\begingroup\$

I was given a homework and I have 2 solutions: one that uses this and other one that doesn't.

I tested both solutions on jsPerf but sometimes it says the version with this is faster or sometimes the other one. So I think I cannot really rely on jsPerf. Anyway, should performance be always the top priority?

By the way, here is the problem statement:

Write a function addg that adds from many invocations, until it sees an empty invocation.

Something like:

addg(); // undefined

addg(2)(); // 2

addg(2)(7)(); // 9

addg(3)(0)(); // 3

addg(1)(2)(4)(8)(); // 15

addg(5)(4)(-2)(); // 7

Here are my 2 solutions:

  1. version with this:

function add(a,b){
  return a+b;
}

function addg(val){
  var numbers = [];

  this.addToCollection = function(val){
    numbers.push(val);

    if(typeof val !== 'number'){  // in case of ()
      var sum = 0, 
          l = numbers.length;

      numbers.forEach(function(num, i){
        if(i === l-1){
          return;
        } else {
          sum = add(sum, num);
        }
      });

//       console.log(numbers);

      if(numbers.length === 1 && numbers[0] === undefined){
        return undefined;
      } else {
        return sum;        
      }

    } else {  // in case of (n)
      return this.addToCollection;  // return the function each time so you can apply it to other numbers in the chain      
    }

  };

  return this.addToCollection(val);
}

console.log(addg());                // undefined
console.log(addg(2)());             // 2
console.log(addg(2)(7)());          // 9
console.log(addg(3)(0)());          // 7
console.log(addg(1)(2)(4)(8)());    // 15
console.log(addg(5)(4)(-2)());      // 7

  1. version without this:

function add(a,b){
  return a+b;
}

function addg(val){
  var numbers = [];

  function addValues(val){
    numbers.push(val);

    if(typeof val !== 'number'){  // in case of ()

      var sum = 0, 
          l = numbers.length;

      numbers.forEach(function(num, i){
        if(i === l-1){
          return;
        } else {
          sum = add(sum, num);
        }
      });

//       console.log(numbers);

      if(numbers.length === 1 && numbers[0] === undefined){
        return undefined;
      } else {
        return sum;        
      }

    } else {  // in case of (n)
      return addValues; // return the function each time so you can apply it to other numbers in the chain 
    }
  }

  return addValues(val);

}

console.log(addg());                // undefined
console.log(addg(2)());             // 2
console.log(addg(2)(7)());          // 9
console.log(addg(3)(0)());          // 7
console.log(addg(1)(2)(4)(8)());    // 15
console.log(addg(5)(4)(-2)());      // 7

I would like to know which version is better and why. If you have any suggestions for improving my code, please feel free to let me know.

\$\endgroup\$
  • 9
    \$\begingroup\$ Your use of this in the first implementation is just wrong. In fact, it won't even work if you run your code in strict mode. When not in strict mode, this === window in the browser. So, you're just using window.addToCollection which is not a particularly good way to do things. this will be set to an object if you use new SomeFunc(). \$\endgroup\$ – jfriend00 Dec 4 '15 at 3:07
  • \$\begingroup\$ An array to sum up numbers is a bit of overkill now isn't it? Just keep a running total. \$\endgroup\$ – Voo Dec 4 '15 at 21:01
  • 3
    \$\begingroup\$ This is cool, I made a really similar script some time ago for fun, it might help you shortening your code since it feels overly complicated for me \$\endgroup\$ – Francisco Presencia Dec 4 '15 at 23:40
  • \$\begingroup\$ @FranciscoPresencia. Hey that looks similar to what I said:) \$\endgroup\$ – tkellehe Dec 6 '15 at 3:27
  • \$\begingroup\$ @FranciscoPresencia There is a small problem in your code. When you call sum function, there should be an empty invocation at the end. like sum(1)(2)(3)(4)(5)(). And for this, I see that it is returning NaN. \$\endgroup\$ – Rahul Desai Dec 6 '15 at 19:02
28
\$\begingroup\$

First and foremost:

Anyway, should performance be always the top priority?

Definitely not. Don't write things that are needlessly unperformant, but only be overly concerned with performance when you have a reason to be. Most of the time JS execution time is irrelevant, because you're waiting for user interaction, or for a network request, or something else that makes the execution time of the JS pretty unimportant.

I'd be more concerned with writing code that's clear, expressive, and maintainable than performant.


Regarding your usage of this, though, you definitely shouldn't be using this as you are in the first example. You're leaking into the global scope. When addg is run, what is this? If you were calling obj.addg(), this would be obj, but since you're just calling addg(), this is the global scope A.K.A. window (in the browser).

With your first version if you run this in your console you'll see:

addg(); 
//undefined
addToCollection;
//function!  Whoops

That's not what you want. You don't need to assign the internal addToCollection function anywhere, so don't.


An add function is a bit pointless. You're adding undefined to the array of numbers in the () case, then adding special logic to exclude it from the sum, later, but instead you should just not add undefined at all. Only do numbers.push(val) if typeof val === 'number'.

But, then, do you really need to keep an array of numbers at all?

\$\endgroup\$
  • 7
    \$\begingroup\$ Wow, I messed this up :D Thank you for clearing my understanding about this. \$\endgroup\$ – Rahul Desai Dec 4 '15 at 3:19
  • 7
    \$\begingroup\$ As a way of knowing when this is being abused, 'use strict'; helps. \$\endgroup\$ – Darkhogg Dec 4 '15 at 13:52
  • \$\begingroup\$ this is better used with new constructor, and only in that case will point to an instance of your constructor \$\endgroup\$ – Zorgatone Dec 6 '15 at 12:22
22
\$\begingroup\$

You don't need fancy logic. Good old closure is good enough here.

function addg ( left ) {
   if ( left === undefined ) return undefined;
   return function addg_chain ( right ) {
      return right === undefined ? left : ( left += right, addg_chain );
   };
}

This simplistic approach creates an execution context, a closure, on the first call, with the parameter left. It will store our current count.

The returned value is a new function that adds to the counter and returns itself, thus preserving the context, until it receives no parameter.

This approach is highly efficient, since only one context and one function is created for each chain, regardless of depth. But it has the side effect that you can't reuse a partial calculation, because further calculations all share the same context.

e.g.

var nine = addg(5)(4);
var ten = nine(1)(); // 10
var twelve = nine(3)(); // Surprise, it is 13!

If you need this capability, the solution is also simple: use a new function, a new closure, for each call. (Thanks to 200_success for the new code.)

function addg ( left ) { 
   if ( left === undefined ) return undefined;
   return function addg_chain ( right ) {
      return right === undefined ? left : addg( left + right );
   }
}

Creating a new function with each step is obviously a lot less efficient than reusing the same function. So if you don't need to preserve the state, the first code should be sufficient.

Regardless, closure is still simpler than using object properties and worrying about this, don't you think so? Don't forget to vote up if you agree. :)

Some programmers commented that these code is hard to understand. I assure you that it is just as painful for me to not put them on one line like 200_success's solution, because my code compression tolerance is also pretty high.

I hope to striking a balance between too verbose and too condense. Thanks for your understanding.

\$\endgroup\$
  • 7
    \$\begingroup\$ Your second solution seems too complex. I came up with function addg(a) { return (a === undefined) ? a : function(b) { return (b === undefined) ? a : addg(a + b) } } \$\endgroup\$ – 200_success Dec 4 '15 at 7:57
  • 2
    \$\begingroup\$ this is of course opinion, but these solutions seem incredibly unreadable to me. \$\endgroup\$ – Darkhogg Dec 4 '15 at 13:54
  • 1
    \$\begingroup\$ This solution is pure, pure spaghetti. \$\endgroup\$ – abuzittin gillifirca Dec 4 '15 at 15:43
  • 4
    \$\begingroup\$ @Darkhogg: it is only opinion, and yours is wrong. :-) Seriously, if you take 200_success's solution and format it properly, it has a stark beauty to it and is far more readable (to an experienced programmer) and the proposed alternatives. \$\endgroup\$ – Malvolio Dec 4 '15 at 17:51
  • 5
    \$\begingroup\$ @abuzittingillifirca There is nothing remotely spaghettilike about parent's solution or 200_success's elegant solution. You should read this: steve-yegge.blogspot.com/2008/02/portrait-of-n00b.html \$\endgroup\$ – Jonah Dec 5 '15 at 1:31
12
\$\begingroup\$

Your use of this in the first implementation is just wrong. In fact, it won't even work if you run your code in strict mode. When not in strict mode, this === window in the browser. So, you're just using window.addToCollection which is not a particularly good way to do things.

this gets a meaningful value if you call a constructor with the new operator or you use .apply() or .call() or you call obj.method(). Otherwise, it will just be the global object (in regular mode) or undefined in strict mode.

I'd suggest this simplification which uses a function closure to store the state of the sum and returns an internal function to allow the chaining. This is OO-like solution where addg() creates a function object that stores the current sum in a private closure and f() on the returned function f is a means of extracting the sum and f(val) is means of adding to it and chaining is supported:

function addg(val) {
    var sum = 0;

    // if you really want addg() all by itself to return undefined
    // instead of 0, you can uncomment this next line
    // if (typeof val === "undefined") return undefined;

    function sumNext(val) {
        if (typeof val === "undefined") {
            return sum;
        } else {
            sum += val;
            return sumNext;
        }
    }
    return sumNext(val);
}

log(addg());
log(addg(2)());
log(addg(2)(7)());
log(addg(3)(0)());
log(addg(1)(2)(4)(8)());
log(addg(5)(4)(-2)());


// logging function
function log(x) {
    var div = document.createElement("div");
    div.innerHTML = x;
    document.body.appendChild(div);
}

This uses a very useful feature in Javascript called a "closure". When addg() returns the inner function sumNext(), the code in sumNext retains access to the sum variable even though addg() has already finished executing. This allows you to privately accumulate the sum with successive calls to sumNext() without using any globals.

Anyway, should performance be always the top priority?

No, performance is never the top priority. I tend to prioritize things as:

  1. Correct (produces the desired result)
  2. Reliable (always produces the desired result)
  3. Robust (handles errors appropriately and is not brittle when things change around it)
  4. Readable and Maintainable (try working on someone else's code or even working on your own code 3 years after you wrote it)
  5. Appropriate Performance (most code does not need to be optimized for performance)

There are times when you work on going for performance, but even then you don't lose sight of the priorities that come before it.


Here's a second version that, instead of a closure uses a property of the returned function. Same results, different way of doing things. This actually exposes the sum property as a public property whereas the first version keeps the sum hidden in a closure.

function addg(val) {
    function sumNext(val) {
        if (typeof val === "undefined") {
            return sumNext.sum;
        } else {
            sumNext.sum = (sumNext.sum || 0) + val;
            return sumNext;
        }
    }
    return sumNext(val);
}

log(addg());
log(addg(2)());
log(addg(2)(7)());
log(addg(3)(0)());
log(addg(1)(2)(4)(8)());
log(addg(5)(4)(-2)());

// logging function
function log(x) {
    var div = document.createElement("div");
    div.innerHTML = x;
    document.body.appendChild(div);
}

This version is coded to return undefined for addg(), but that can easily be modified to return 0 if desired (which seems more logical to me).

\$\endgroup\$
  • 2
    \$\begingroup\$ typeof val === "undefined" || !arguments.length Isn't the !arguments.length part redundant? If no arguments are passed then val will be undefined. \$\endgroup\$ – Peter Olson Dec 4 '15 at 5:56
  • \$\begingroup\$ @PeterOlson - Yes, it was redundant - I removed it. \$\endgroup\$ – jfriend00 Dec 4 '15 at 6:06
  • \$\begingroup\$ Just a little nitpick, you solution isn't Correct. addg() produces 0 instead of the expected undefined. I like yours better and i think it makes sense to do it that way, but it should be pointed out and perhaps explained why it i better. \$\endgroup\$ – Tibos Dec 4 '15 at 6:10
  • 1
    \$\begingroup\$ @Bergi - What you're calling a side effect seems like the point of the exercise. This creates a function object that accumulates a total internally. Each time it is called, it adds to the prior total. Anytime you call it with no value, it returns the total stored internally. It's like x(val) is x.add(val) and x() is x.getCurrentSum(). Do you see anything in the OP's description of the problem that says this behavior is either wrong or undesirable? Yes, there is an alternative design that behaves differently, but I don't see why that would be either required or more desirable. \$\endgroup\$ – jfriend00 Dec 4 '15 at 15:42
  • 1
    \$\begingroup\$ @Bergi - I've modified the description of this solution to describe is an OO solution where addg() creates a function object that stores the sum state in a private closure and essentially has two methods (one for retrieving the current sum and one for adding to it). Hopefully that makes it more clear this is an intended OO design that behaves in a manner consistent with an object with two methods, not as an undesirable side effect. \$\endgroup\$ – jfriend00 Dec 4 '15 at 16:04
6
\$\begingroup\$

I am going on a limb here and tell you that both versions look great, but there's some German Overengineering™ going on here.

You can easily do this with a variable, a closure and some imagination.

To verify if a value was passed, you can either use arguments.length or check if the value is undefined.
You should prefer value === undefined over arguments.length, as it can help to limit some optimizations.

But now we have a problem: undefined is a global variable, which can be set by anyone, anything at any time.
You can solve that issue by 'use strict'; before the code or by declaring var undefined; inside.
I prefer the latter one, since one could run window.undefined = 5;, which would cause you to indirectly check and return 5.
I still recommend adding 'use strict'; since it also adds other basic checks and security features.

Here's my simplified version:

'use strict';
function addg(value) {
    var undefined;

    if(value == undefined)
    {
        return undefined;
    }

    var total = value / 1 || 0;

    var fn = function(value){
        if(value == undefined)
        {
            return total;
        }

        total += value / 1 || 0;

        return fn;
    };

    return fn;
}

Proper number handling is left as an exercise.


After all, my answer is: Neither! Simplify it.

\$\endgroup\$
  • 1
    \$\begingroup\$ arguments.callee will not work in strict mode and is generally not recommended because it limits some optimizations that the interpreter can do. \$\endgroup\$ – jfriend00 Dec 4 '15 at 3:09
  • \$\begingroup\$ Douglas Crockford recommended not to use arguments. \$\endgroup\$ – Rahul Desai Dec 4 '15 at 3:13
  • \$\begingroup\$ @jfriend00 Totally forgot about that. I have added that to the answer. \$\endgroup\$ – Ismael Miguel Dec 4 '15 at 3:14
  • 1
    \$\begingroup\$ @RahulDesai I am really sorry, but I don't know who Douglas Crockford is. But if you want, I can provide a more complete version without arguments. It's just that it simplifies your work so much. \$\endgroup\$ – Ismael Miguel Dec 4 '15 at 3:15
  • 1
    \$\begingroup\$ Don't do var undefined. It's unnecessary, a bit confusing, and is protecting against a case that, realistically, should never happen. \$\endgroup\$ – Retsam Dec 4 '15 at 4:22
4
\$\begingroup\$

Just to throw my hat into the ring, I'll propose this:

function addg(value) {
  var sum;

  function adder(value) {
    if(typeof value === 'undefined') return sum;
    sum = (sum || 0) + value;
    return adder;
  };

  return adder(value);
}

As has already been pointed out, there's no reason to store a collection of numbers; all you need is the previous result. Which calls for a closure.

Technically you could argue that calling addg(undefined) isn't an empty invocation: You're passing in undefined, yet it's treated as though you didn't pass anything in. If that's a concern, you can do this instead:

function addg(value) {
  var sum;

  function adder(value) {
    if(!arguments.length) return sum;
    sum = (sum || 0) + value;
    return adder;
  };

  return adder.apply(null, [].slice.call(arguments));
}

Also, technically the (sum || 0) would trip on false, NaN and other false'y values, so for instance addg(3)(Number.NaN)(3)() would return 3, since 3 + NaN => NaN, but the NaN gets replaced by zero, meaning you get 0 + 3. So if that's a concern, do this:

function addg(value) {
  var sum;

  function adder(value) {
    if(!arguments.length) return sum;
    sum = (typeof sum === 'undefined' ? 0 : sum) + value;
    return adder;
  };

  return adder.apply(null, [].slice.call(arguments));
}
\$\endgroup\$
2
\$\begingroup\$

Since @Retsam did a good job at explaining this for what you are doing, I would like to show a much simpler way to create addg that is quite fast.

function addg( arg ) {
    // If arg cannot be interpreted as a number then process the data collected.
    // Note: Can change to ->
    // if(arg === undefined) {
    // or if need to just look for empty arguments...
    // if(!arguments.length) {
    if(isNaN(arg = +arg)) {
        // Temp storage.
        var sum = addg.sum;
        // Resets the function.
        // Can also do ->
        // addg.sum = undefined;
        delete addg.sum;
        // Returns the sum collected.
        return sum;
    }
    // Checks to see if addg has sum yet.
    addg.sum = (addg.sum) ? addg.sum + arg : arg;
    // Returns the function for more args.
    return addg;
};

Some examples:

addg();       // => undefined
addg(1)();    // => 1
addg(1)(2)(); // => 3
addg(0)();    // => 0

// Invocation of addg
addg(1);
// Another invocation of addg...
addg(1);
// Empty invocation so return sum!
addg(); // => 2
\$\endgroup\$
  • 1
    \$\begingroup\$ Call addg(1) several times (which returns the function). Then call addg(1)(). What do you get? \$\endgroup\$ – Octopus Dec 4 '15 at 23:06
  • \$\begingroup\$ @Octopus. You get the desired behavior. The question says "write a function addg that adds from many invocations, until it sees an empty invocation." So, if you invoke the function addg(1) then addg(1)() you should get 2 because addg is supposed to add up every invocation until an empty invocation occurs:) Unless somewhere it states specifically that is wrong please point that out:) \$\endgroup\$ – tkellehe Dec 5 '15 at 1:50

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.