What's more important in JS: performance, or the understandability of the code?

I did functions for those a long time ago as well - including disregarding of non-numeric values and multiple mode values.

var mean = Math.average = function() {
var cnt, tot, i;
cnt = arguments.length;
tot = i = 0;
while (i < cnt;) tot+= arguments[i++];
return tot / cnt;
}
var median = function() {
var ary, numA, i;
ary = Array.prototype.slice.call(arguments);
for (i = ary.length-1; i >= 0; i--) {if (ary[i] !== +ary[i]) ary[i] = Number.NEGATIVE_INFINITY;}
numA = function(a, b){return (a-b);};
ary.sort(numA);
while (ary.length > 1 && !isFinite(ary[0])) ary.shift();
return ary[Math.floor(ary.length/2)];
}
var mode =  function() {
var ary, i, max, mode, str;
ary = Array.prototype.slice.call(arguments);
max = 0;
mode = [];
str = ary.sort();
str = "~" + str.join('~~') + "~"
str.replace( /(~\-?\d+~)\1*/g, function(a,b){
    var m = a.length / b.length;
    if (max <= m ) {
        if (max < m) {mode = [];max = m;}
        mode.push( +b.replace(/~/g,""));
    } 
});
return mode;
}

Let’s see what techniques we can apply to improve this code to be something more usable.

First, indent the code so that the different structures are easier to see.
Next, replace one-line if and for statements with braces surrounding the code block instead.

The variable initialization can be looked at next. The following can be confusing for some to understand:

var mean = Math.average = function() {

In keeping with the rest of the code, we’ll assign the function to just one variable and then later on assign Math.average.
In fact, why are they being assigned to variables? They’re just functions.

function mean() {
    ...
};
Math.average = mean;

The multiple assignments on one line can be simplified:

var cnt, tot, i;
cnt = arguments.length;
tot = i = 0;

Using a series of var statements helps to make everything there clearer. Also, we don’t need to restrict ourselves to three character variable names. They can be spelled out and made crystal clear.

var count = arguments.length;
var total = 0;
var i = 0;

Next, we have a while loop, with an increment happening inside.

while (i < count;) {
    total += arguments[i++];
}

It’s far easier to understand how that section works if we make it a for loop instead. Any tiny fraction of performance benefit is lost when coders have to stop and think about what’s happening there.

for (i = 0; i < count; i += 1) {
    total += arguments[i];
}

And if we don’t mind using ES6 techniques, we could even use Array.from along with Array.reduce to help us sum up the total.

total = Array.from(arguments).reduce(function (total, num) {
    return total + num;
}, total);

Prefixing something with the plus symbol is a hack:

(array[i] !== +array[i])

and should be replaced with easier to understand coding techniques instead. Taking an overall look at the use of infinity in the median function, this can all be vastly simplified by the use of the splice method.

for (i = array.length - 1; i >= 0; i -= 1) {
    if (array[i] !== Number(array[i])) {
        array.splice(i, 1);
    }
}

And if we don’t mind using ES5 techniques like Array.filter, this can all be simplified to:

array = array.filter(function (num) {
    return num === Number(num);
}

Next with the mode function, code that requires you to add “~” symbols all over the place and then needs the use of regular expressions such as /(~\-?\d+~)\1*/g is too complex for its own good.

We can improve all this by keeping a counter tally for each number that we see:

for (i = 0; i < array.length; i += 1) {
    num = array[i];
    if (num === Number(num)) {
        if (counter[num] === undefined) {
            counter[num] = 0;
        }
        counter[num] += 1;
    }

and tracking if that amount is the most that we’ve seen. Whenever the max is increased we reset the array with that number:

if (counter[num] > max) {
    max = counter[num];
    modes = [num];
}

and if any counter matches that max we can just add it on to that array.

} else if (counter[num] === max) {
    modes.push(num);
}

This way the code is a lot clearer than it would be otherwise, allowing us to more easily understand how it works too.

The resulting code after this cleanup is:

function mean() {
    var count = arguments.length;
    var total = 0;
    var i;
    for (i = 0; i < count; i += 1) {
        total += arguments[i];
    }
    return total / count;
}
function median() {
    function numA(a, b) {
        return (a - b);
    }
    var array = Array.prototype.slice.call(arguments);
    var i;
    for (i = array.length - 1; i >= 0; i -= 1) {
        if (array[i] !== Number(array[i])) {
            array.splice(i, 1);
        }
    }
    array.sort(numA);
    return array[Math.floor(array.length / 2)];
}
function mode() {
    var array = Array.prototype.slice.call(arguments);
    var max = 0;
    var counter = [];
    var modes = [];
    var i;
    var num;
    for (i = 0; i < array.length; i += 1) {
        num = array[i];
        if (num === Number(num)) {
            if (counter[num] === undefined) {
                counter[num] = 0;
            }
            counter[num] += 1;
        }
        if (counter[num] > max) {
            max = counter[num];
            modes = [num];
        } else if (counter[num] === max) {
            modes.push(num);
        }
    }
    return modes;
}
Math.average = mean;
4 Likes

I always declare functions as variables to avoid hoisting.

1 Like

It also allows you to redefine functions when necessary

How does using function expressions avoid hoisting, as compared with using function declarations?

Aren’t function declarations handled first before any variables? As functions are hoisted before declared variables, we should always be defining out functions before other code, right?

The other issue that I have is that using var for both functions and variables only serves to muddy the waters. When you see a var statement, that should be a variable that you’re seeing there, instead of a function.

This can be all personal choice of course, which is where using style guides can help to remove conflicting matters of taste and style.

So how would you rewrite the following:which defines three addEvent functions the first of which rewrites itself to one of the other two depending on which version of the function the browser supports? You can’t define a function in an if statement unless you assign values to it in order to avoid it being hoist.

var addEvent = function(ob, type, fn) {
   if (window.addEventListener)
      addEvent = function(ob, type, fn ) {
         if ('string' === typeof ob) ob = document.getElementById(ob);
         ob.addEventListener(type, fn, false );
         };
   else if (document.attachEvent)
       addEvent = function(ob, type, fn ) {
          var eProp = type + fn;
          if ('string' === typeof ob) ob = document.getElementById(ob);
          ob['e'+eProp] = fn;
          ob[eProp] = function(){ob['e'+eProp]( window.event );};
          ob.attachEvent( 'on'+type, ob[eProp]);
          };
  addEvent((ob, type, fn);
}

[quote=“felgall, post:23, topic:222472, full:true”]
So how would you rewrite the following:[/quote]

In that type of situation, the addEvent variable is not ending up being defined as the whole function. Instead, it’s being reassigned using lazy evaluation to one type of function or the other.

So, I would use an IIFE to make it clear from the outset that something more than just a primitive variable or a function is going on there.

Inside the IIFE I would start off with declared functions, and then decide which one to return from the IIFE so that the addEvent variable ends up becoming one of the functions or the other.

That gets rid of the lazy evaluation issue, keeps the functions separate from the code that uses those functions, and makes it easier to see how they’re being used.

For example:

if (window.addEventListener) {
    return eventListenerFunc;
} else if (document.attachEvent) {
    return attachEventFunc;
}

The example in full:

var addEvent = (function () {
    function eventListenerFunc(ob, type, fn) {
        if ("string" === typeof ob) ob = document.getElementById(ob);
        ob.addEventListener(type, fn, false);
    }
    function attachEventFunc(ob, type, fn) {
        var eProp = type + fn;
        if ("string" === typeof ob) ob = document.getElementById(ob);
        ob["e" + eProp] = fn;
        ob[eProp] = function () { ob["e" + eProp](window.event);};
        ob.attachEvent("on" + type, ob[eProp]);
    }
    if (window.addEventListener) {
        return eventListenerFunc;
    } else if (document.attachEvent) {
        return attachEventFunc;
    }
}());

It’s easier to understand what the above code is doing than the initial code that was presented.

There are other things I’d do too such as unpack the if statements in to multi-line blocks, but I’ve left them as-is for the sake of easy comparison.

I find the logic clearer, as functions aren’t magic things that get lifted to the top. It makes more sense to me when passing functions as arguments too.

fn();
function fn() {
  console.log(fn2());
  function fn2() {
    return 'running';
  }
}

If you always define functions on variables then you enforce having to put things in order.

var fn = function() {
  var fn2 = function() {
    return 'running';
  }
  console.log(fn2());
}
fn();

It’s something that CoffeeScript enforces too, it’s just one less thing to think about and has no downsides.

1 Like

[quote=“markbrown4, post:25, topic:222472, full:true”]
I find the logic clearer, as functions aren’t magic things that get lifted to the top.[/quote]

Hold on hold on - functions don’t get lifted to the top? is that right?

What do you mean there?

This runs

fn();
function fn() {
  console.log(fn2());
  function fn2() {
    return 'running';
  }
}

This doesn’t, because you’re calling functions that haven’t been created yet.

fn();
var fn = function() {
  console.log(fn2());
  var fn2 = function() {
    return 'running';
  }
}

I like this restriction because having to put them in order makes a lot more sense to me.

var fn = function() {
  var fn2 = function() {
    return 'running';
  }
  console.log(fn2());
}
fn();
1 Like

Ahh, so you mean that function expressions don’t get hoisted, where as function declarations do. That is a fundamental aspect of JavaScript that is important to know.

And I agree, even though function declarations do get hoisted, you shouldn’t code with that type of behaviour in mind.

The hoisting issue aside - I find that using var for functions and other variables serves to muddy and confuse the code.
By defining the functions first, followed by variables - that helps to provide a structure that is easier for others to follow.

That code is still assigning functions to variables - just not in such a clear and obvious way.

You cannot see as easily from looking that the code is returning a function.

Plus you are testing for which function to return every single time addEvent is called whereas my code only does it once the first time it is called - so your code is extremely inefficient testing something over and over that never changes. It is equivalent to two of my functions but completely leaves out the processing done by the outer function.

No Felgall, you are wrong. The IIFE executes only the once, where the appropriate function is returned and assigned to the addEvent variable. You’re welcome to use console.log to confirm that fact.

This is not the first time that you have been wrong in recent discussions. I don’t know if this is a deliberate attempt to tar me with false claims, or if it’s a genuine mistake on your part. Either way though, it’s something that needs to be watched closely.

I’d missed seeing the effect of the IIFE and for that I apologise.

Anyway, your version still assigns as many functions to variables as mine does - the only difference is that it names some of them first so as to assign them by name instead of directly. That’s in addition to the function declarations you added - and there’d be even more function declarations if you’d declared all of the functions in the code before assigning them to variables.

Anyway, my version can be easily extended to include removeEvent as follows:

var removeEvent;
var addEvent = function(ob, type, fn) {
   if (window.addEventListener) {
      addEvent = function(ob, type, fn ) {
         if ('string' === typeof ob) ob = document.getElementById(ob);
         ob.addEventListener(type, fn, false );
         };
      removeEvent = function(ob, type, fn ) {
         ob.removeEventListener(type, fn, false );
         };
  } else if (document.attachEvent) {
       addEvent = function(ob, type, fn ) {
          var eProp = type + fn;
          if ('string' === typeof ob) ob = document.getElementById(ob);
          ob['e'+eProp] = fn;
          ob[eProp] = function(){ob['e'+eProp]( window.event );};
          ob.attachEvent( 'on'+type, ob[eProp]);
          };
       removeEvent = function(ob, type, fn ) {
          var eProp = type + fn;   
          ob.detachEvent('on'+type, ob[eProp]);   
          ob[eProp] = null;
          ob["e"+eProp] = null;
          };
  }
  addEvent(ob, type, fn);
}

I can’t see how you could amend your version to do this without ending up with more assignments of functions to variables than in my version (in addition to all the declarations).

That’s simple - with a second IIFE.

and an extra if statement?

Anyway both your version and mine assign just as many functions to variables - the only difference is that yours declares some of the functions first and then assigns them by name whereas mine defines them directly.

Anyway, I’d like to see your version of the add/remove code using your second IIFE but still only using one if statement to select which version of the add/remove functions to create - if you have a way of doing that as I can’t see any way to avoid a second if/else when using your more convoluted approach.

[quote=“felgall, post:33, topic:222472, full:true”]
Anyway, I’d like to see your version of the add/remove code using your second IIFE but still only using one if statement to select which version of the add/remove functions to create[/quote]

The amount of time gained by not using one if statement cannot make up for the amount of time spent in this discussion.

I do not consider it more convoluted to declare two functions, and use an if statement to determine which one of those that gets assigned. The use one if statement for removeEvent and another if statement for addEvent helps to aid clarity.

Using one if statement for both results in spooky action at a distance, where a function has side-effects that occur elsewhere, and is something best to be avoided.

This conversation helps to demonstrate two different types of coding paradigms.

Felgall focuses on raw JavaScript performance, and I focus on the understandability of the code.

Performance pros:

  • Not a cycle of interpretation time is wasted
  • Attempts to match variable hoisting paradigm

Performance cons:

  • Code can be not as easily understand
  • Side-effects occur (functions set other variables) in the aim to improve performance
  • Less flexible variable names, such as when reassigning external variables

Understandability pros:

  • Less time is spent figuring out how the code works
  • Premature optimization is avoided

Understandability cons:

  • Cpu spends a few more cycles processing the code

Separate from the above, we have different view on how to write functions too. Felgall prefers to always use the function expression notation so that all of his functions have the same consistant format. Whereas I prefer to use function declarations, function expressions and IIFE’s to give clues to the reader about the intention of the code.

None of these things are right or wrong in their own right. It’s quite remarkable that JavaScript allows such a wide variety of coding techniques, which can help to suit the sensibilities of the coder.

My apologies go to @KookieYolo for us having overtaken your thread with this discussion.

4 Likes

Here’s another variant that is part way between the two - retaining the IIFE of Paul’s version but without the two function statements. It has the IIFE assigned to the variable as in Paul’s version and returns the function as in Paul’s version but uses function expressions to create all the functions as in my version.


var addEvent = function() {
   if (window.addEventListener)
      return function(ob, type, fn ) {
         if ('string' === typeof ob) ob = document.getElementById(ob);
         ob.addEventListener(type, fn, false );
         };
   else if (document.attachEvent)
       return function(ob, type, fn ) {
          var eProp = type + fn;
          if ('string' === typeof ob) ob = document.getElementById(ob);
          ob['e'+eProp] = fn;
          ob[eProp] = function(){ob['e'+eProp]( window.event );};
          ob.attachEvent( 'on'+type, ob[eProp]);
          };
}();

[quote=“felgall, post:36, topic:222472, full:true”]
Here’s another variant that is part way between the two - retaining the IIFE of Paul’s version but without the two function statements. It has the IIFE assigned to the variable as in Paul’s version and returns the function as in Paul’s version but uses function expressions to create all the functions as in my version.[/quote]

Good one, though it can be difficult for someone reading the code to determine that more than just a function assignment is occurring there without having to reread through it a couple of times.

What do you think of also using the surrounding parenthesis around the IIFE, to help inform people from the start that something more than an ordinary function expression is being assigned to the addEvent variable?

var addEvent = (function() {
   if (window.addEventListener)
      return function(ob, type, fn ) {
         if ('string' === typeof ob) ob = document.getElementById(ob);
         ob.addEventListener(type, fn, false );
      };
   else if (document.attachEvent)
       return function(ob, type, fn ) {
          var eProp = type + fn;
          if ('string' === typeof ob) ob = document.getElementById(ob);
          ob['e'+eProp] = fn;
          ob[eProp] = function(){ob['e'+eProp]( window.event );};
          ob.attachEvent( 'on'+type, ob[eProp]);
       };
}());

The thought process being:

  • spot the parenthesis at the start of the function
  • quick scan to the end, to see the }()) close of an IIFE
  • back to the top to read through the function
  • knowing now that addEvent will be assigned not the function, but what is returned instead