Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sometimes the best way to teach is show what you feel is a shiny approach instead of letting a student muddle through coding and potentially loose a future brilliant coder.

This is what I would show for 'Determine the largest averaged array`. Please review any and all aspects.

'use strict';
//Return the average of 1 array
function arrayAverage(array) {
  var length = array.length,
      sum = 0,
      i;
  for (i = 0; i < length; i++) {
    sum += array[i];
  }
  return sum / length;
}

//Compare 2 arrays, return the array with the largest average
//If the averages are equal, return array2
function largestAveragedArray(array1, array2) {

  return arrayAverage(array1) > arrayAverage(array2) ? array1 : array2;
}

//Figure out which array is largest, return `first` or `second` accordingly
function compare(array1, array2) {
  var largest = largestAveragedArray(array1, array2);
  return largest == array1 ? 'first' : 'second';
}

var smaller = [100, 80],
    larger = [100, 100];

document.write(compare(larger, smaller) + '<br>' + compare(smaller, larger));

share|improve this question
4  
Yay Stack Snippets! –  Mat's Mug 8 hours ago

3 Answers 3

First of all, a minor complaint, but be careful when using 'use strict'; at a file level—when concatenating JavaScript files for minification, this can have adverse affects. Whenever possible, use it at the top of a function instead.

Anyway, on to the actual code. Your arrayAverage function is fine, but it could be improved and made more explicit by using a more functional style. The .reduce function is perfect for this case, turning the function into a simple one-liner:

function arrayAverage(array) {
  return array.reduce(function (a, b) { return a + b; }) / array.length;
}

Your compare function is, frankly, rather odd. I see three main problems with it:

  • It does not handle arrays that have equal averages very well. Simply preferring the second array is not a good compromise.
  • Returning strings for the result is a little strange and unnatural. While there are no enums in a language like JavaScript, using string literals is not a good replacement.
  • The name compare is very generic and doesn't explain what the function does at all.

To fix all these problems, I propose the following function:

function arrayCompare(arrayA, arrayB) {
  var averageA = arrayAverage(arrayA),
      averageB = arrayAverage(arrayB);
  return averageA < averageB ? -1
       : averageA > averageB ?  1
                             :  0;
}

This is clearer, and it handles the equality case more elegantly. If you really hate nested conditional expressions with a passion, you could use a set of if statements instead, but I think this is perfectly readable when properly formatted.

This change eliminates the need for the largestAveragedArray function entirely, simplifying the code somewhat. As a final note, document.write makes me cringe, but I understand that it's useful in code snippets, so I'll let it slide here. Just don't use it in a real project.

(function () {
  'use strict';
  
  // returns the average of a single array
  function arrayAverage(array) {
    return array.reduce(function (a, b) { return a + b; }) / array.length;
  }
  
  // compares two arrays;
  // returns -1 if avg(a) < avg(b), 1 if avg(a) > avg(b), or 0 if avg(a) = avg(b)
  function arrayCompare(arrayA, arrayB) {
    var averageA = arrayAverage(arrayA),
        averageB = arrayAverage(arrayB);
    return averageA < averageB ? -1
         : averageA > averageB ?  1
                               :  0;
  }

  var smaller = [100, 80],
      larger = [100, 100];
  
  document.write(arrayCompare(smaller, larger) + '<br>' + arrayCompare(larger, smaller) + '<br>' + arrayCompare(smaller, smaller));
})();

share|improve this answer
1  
Very good points, though I would leave functional programming to Coding 102 ;) Also what are your thoughts on handling equal averages ? –  konijn 8 hours ago
2  
@konijn I think they should be handled explicitly, as I did above. Admittedly, I cheated a bit—floating-point equality is trickier than that. If you wanted to handle equal averages correctly, you'd need to do some kind of fuzzy equality. Still, the result would be the same. –  Alexis King 8 hours ago
    
My bad, I missed that and I agree that is far more elegant –  konijn 8 hours ago

It contains a bug!

var smaller = [100, 80], larger = [];

The secound appears twice, because 0 / 0 is a NaN, and 90 > NaN is false.

length = array.length

This is worses the readability. In complicated expressions new variable could help, but array.length too simple to create a new variable.

function largestAveragedArray(array1, array2)

There is no reason to put comparing expression into a new method. KISS.

document.write(compare(larger, smaller) + '<br>' + compare(smaller, larger));

More fancy to use console.log(compare(larger, smaller), compare(smaller, larger)) ;) // console has many cool methods

I think your solution is very regular, I mean it's nice that arrayAverage() is callable from anywhere, but from the performance view it's ugly. Arrays could be very big, so if possible you have to avoid unnecessary iterations.

function compare(array1, array2) {
  var firstSum = 0;
  for (i = 0; i < array1.length; i++) {
    firstSum += array1[i];
  }
  var firstAvg = array1.length ? firstSum / array1.length : 0;

  var secSum = 0;
  for (i = 0; i < array2.length; i++) {
      secSum += array2[i];
      if (secSum && secSum / (i + 1) > firstAvg) return 'second'
  }
  return 'first'; // or equals
}

And what about if equals?

share|improve this answer
    
Can you elaborate on unnecessary iterations? –  konijn 8 hours ago
    
The best solution is depends on the number of elements in each array, and the use case. If you are developing a public library for example, and the size of the arrays are not predictable I think you have to break the iteration as soon as possible. On the other hand pre-optimization is the devil :) –  mhmxs 8 hours ago
    
+1, dividing by length without checking whether it's 0 is a major bug! –  Brian S 6 hours ago
    
This still gets it wrong. The average of a list of no elements is not 0. Throw an exception, don't use a default if there isn't one. Also, your faster algorithm will fail if negative numbers are permitted. –  Alexis King 6 hours ago

Actully thats a best algorithm for it

var smaller = [100, 80],
    same = [ 100,100],
      larger = [100, 100];


function largestAveragedArray( a, b ){
    var c = a.length > b.length ? a : b
        , i = 0
        , sumA = 0
        , sumB = 0;

    for( ; i < c.length; i++ ){
        sumA += a[ i ] ? a[ i ] : 0;
        sumB += b[ i ] ? b[ i ] : 0;
    }

    return sumA == sumB ? 0 : ( sumA > sumB ? 1 : -1 );
}

console.log( largestAveragedArray( smaller, larger ) );
console.log( largestAveragedArray( larger, smaller ) );
console.log( largestAveragedArray( same, larger ) );
share|improve this answer
    
why is that Bruno? –  Malachi 7 hours ago
    
It have O(n) complexity being 'n' the length of the biggest array. First find the biggest array then iterate through it saving the sum for the first and the second array provided, and then i compare them. –  Bruno Queiroz 7 hours ago
1  
would you mind incorporating that into your answer? it would make your answer much better and more likely to get upvotes. –  Malachi 7 hours ago
    
Both your solution and the original solution have the same complexity, O(m*N), m being the number of arrays to compare. If anything, the original solution is better because it is trivial to add a third, fourth, or fifth array to the comparisons. Your solution would require quite the rewrite! –  Joe Frambach 6 hours ago
    
You are wrong, mines does not iterates through both arrays, instead it access them directly by índex, for more arrays you just need to find the biggest of all to iterate –  Bruno Queiroz 5 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.