I have this function that calculates average. I don't want it to look like this. I am hoping for a less cliche average function because I see that most people's look like this.
function mean(array) {
var num=0;
for (var i=1;i<=array.length;i++) num=num+array[i];
var divide=num/array.length;
alert(divide);
}
Can anyone change this up to be better than before? It doesn't have to be more efficient but just look better.
4 Answers 4
First of all, your code is incorrect. Arrays have a starting index of 0
, not 1
. Furthermore, array.length
is one more than the last filled index (because the length is 1 based, and the array is 0 based. Yes, this is confusing.) So you're starting by not including the first item, and then also adding up an undefined item.
And what happens if you get an empty array? You divide by zero, which is MADNESS!! MADNESS I TELL YOU!
Second, there's a reason the code looks the same as everywhere: You have followed the definition of the average; the sum of components divided by the number of components. There's no shame in this, you should actually be proud that you've managed to conform to the standard.
If you really want, you can make the loop implicit. ES5 (ECMAScript 5, the latest js standard) defined an Array.prototype.reduce
function (click link for the reference.) It's a perfect fit for your needs, but doesn't change the core algorithm.
Third, your mean
function is doing something unexpected: Instead of just calculating the mean, it displays it as well. This does not follow expectations (or at least my expectation), as a function which calculates the mean should do that, and just that. That translates to a very important design aspect, which says that a function should do one thing, one thing only, and do it well. Because imagine if Math.sqrt
, for instance, starts displaying the square roots instead of giving you them. Like division by zero, it's madness all over again!
-
\$\begingroup\$ Example code would probably be nice. \$\endgroup\$Inkbug– Inkbug2012年07月29日 12:20:19 +00:00Commented Jul 29, 2012 at 12:20
-
4\$\begingroup\$ @Inkbug Since the OP is new to js, imo it'd be better to let him try first before giving him an already baked solution \$\endgroup\$Zirak– Zirak2012年07月29日 16:33:07 +00:00Commented Jul 29, 2012 at 16:33
-
\$\begingroup\$ Dividing by 0 results in
Number.POSITIVE_INFINITY
, which is not altogether unreasonable, thoughNumber.NaN
would be more appropriate in this case. \$\endgroup\$200_success– 200_success2015年01月19日 18:43:26 +00:00Commented Jan 19, 2015 at 18:43
Zirak's answer is all the advice you need, but since he didn't actually write any code, here's how I might re-write it.
function mean(arr) { // it's ok to use short variable names in short, simple functions
// set all variables at the top, because of variable hoisting
var i,
sum = 0, // try to use informative names, not "num"
len = arr.length; // cache arr.length because accessing it is usually expensive
for (i = 0; i < len; i++) { // arrays start at 0 and go to array.length - 1
sum += arr[i]; // short for "sum = sum + arr[i];"
} // always use brackets. It'll save you headaches in the long run
// don't mash your computation and presentation together; return the result.
return sum / len;
}
Note that this code is optimized for arrays with at least 1 element; if the majority of its uses are on 0-length arrays, you'd see a performance boost if you added a short return.
Alternatively you can use Array.prototype.reduce. I highly recommend that you not use this method - while it is more "unique", it is also much harder to understand and much less efficient. And good code strives to be readable and efficient (and robust), not unique.
var mean = (function () { // Use an IIFE to get private storage
// cache this function - it's very expensive to re-create it every time you call mean()
function addLast(prev, current) {
return prev + current;
}
// return an anonymous function. When we call mean(), this is what is called. The reason
// we can't set it directly is because we need something with access to addLast, and we
// don't want addLast in the global scope.
return function (arr) {
return arr.reduce(addLast, 0) / arr.length;
};
})(); // end of IIFE. executes the function and sets mean to the return value.
-
\$\begingroup\$ +1, but I would do
sum += +arr[i]
to coerce a number out of the item. I also see no problem with returningNaN
for an empty list - the mean of an empty list is actually not 0. :) \$\endgroup\$ANeves– ANeves2012年07月30日 10:30:13 +00:00Commented Jul 30, 2012 at 10:30 -
\$\begingroup\$ @ANeves -
n / 0
isInfinity
, notNaN
\$\endgroup\$Joseph Silber– Joseph Silber2012年07月30日 14:11:56 +00:00Commented Jul 30, 2012 at 14:11 -
2\$\begingroup\$ @ANeves whether to coerce a number is certainly an important consideration, but my preference is to provide documentation and assume the caller reads it. Passing in something other than numbers is a caller error, and it's a bad idea to correct for some errors (
['1', '2', '3']
) but not all (['thing']
). (actually, I don't think this function should be correcting any user errors, but we don't need to get into that). Also, it bothers me to take a (slight) performance hit in correct-use cases for the sake of code that doesn't even use the function correctly. \$\endgroup\$delete me– delete me2012年07月30日 17:42:31 +00:00Commented Jul 30, 2012 at 17:42 -
\$\begingroup\$ Well the mean of
[]
is neither0
norInfinity
.0
seemed like a convenient default to me, but I guess it would be more informative to returnNaN
. Any thoughts? If there's some consensus then I'll edit the code, if necessary. \$\endgroup\$delete me– delete me2012年07月30日 17:54:36 +00:00Commented Jul 30, 2012 at 17:54 -
\$\begingroup\$ @JosephSilber but it won't divide by 0, it divides by 1. So for an empty array, it returns 0/1 = 0. (0/N = NaN; N/0 = Infinity. 0/0 seems to be NaN too.) \$\endgroup\$ANeves– ANeves2012年07月31日 08:05:30 +00:00Commented Jul 31, 2012 at 8:05
More efficient: No; More readable: Yes
function mean( list, more ) {
if ( more ) {
list = [].slice.call( arguments );
} else if ( !list || list[0] === undefined ) return;
var a = list,
b = list.length;
return (function execute() {
if ( !a.length ) return 0;
return ( a.pop() / b ) + execute();
})();
}
What this function basically does is use recursion to divide each number in the array by the length of the array. Since this is true...
\$\dfrac{a + b + c}{d} = \dfrac{a}{d} + \dfrac{b}{d} + \dfrac{c}{d}\$
I use that logic in the function and it works just fine.
-
\$\begingroup\$ Though this is a nifty implementation, I feel that it would be a bad choice to ever actually put it in production code. Just my 2 cents though, I suppose. \$\endgroup\$Corbin– Corbin2012年07月29日 07:39:48 +00:00Commented Jul 29, 2012 at 7:39
-
\$\begingroup\$ I know; that's why I mentioned it was less efficient. For loops are very fast so it's better to use (well, an improved version) of the OP's function than mine. But if he's looking for a different/readable way to write this, I think the above is the best way. \$\endgroup\$David G– David G2012年07月29日 12:11:25 +00:00Commented Jul 29, 2012 at 12:11
-
6\$\begingroup\$ Your definition of elegant is obviously not the same as mine. \$\endgroup\$luiscubal– luiscubal2012年07月29日 14:09:15 +00:00Commented Jul 29, 2012 at 14:09
-
6\$\begingroup\$ @David I think this is significantly less readable than a simple loop (and I suspect a large majority would agree). Fancy code is not always better code, especially when the fancy code is implementing a slower algorithm. \$\endgroup\$Corbin– Corbin2012年07月30日 00:43:35 +00:00Commented Jul 30, 2012 at 0:43
-
3\$\begingroup\$ Additionally to what has been said, I find this far less readable than the question's code. :( I also frown upon re-declaring the function
execute
on every call. \$\endgroup\$ANeves– ANeves2012年07月30日 10:26:43 +00:00Commented Jul 30, 2012 at 10:26
Maybe use some array methods like reduce
? This will add up all the elements, starting with an initial sum of 0 (the second argument to reduce
), then the sum is divided by the array's length to compute the mean.
function mean(array) {
const sum = array.reduce((a, b) => a + b, 0);
return sum / array.length;
}
-
\$\begingroup\$
const
is an ECMAScript addition that was not available in 2012, when this question was posted. \$\endgroup\$200_success– 200_success2017年11月07日 19:16:26 +00:00Commented Nov 7, 2017 at 19:16 -
\$\begingroup\$ It is available now, so I don't see the issue. \$\endgroup\$kamoroso94– kamoroso942017年11月07日 19:17:11 +00:00Commented Nov 7, 2017 at 19:17
-
\$\begingroup\$ Reviewing old code according to modern standards can be acceptable. It's just a caveat that is worth noting in the interest of fairness. \$\endgroup\$200_success– 200_success2017年11月07日 19:19:44 +00:00Commented Nov 7, 2017 at 19:19