11

Find the largest number in each of the sub-array and then make an array of those largest numbers.[[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]]

I wrote some code, and I can't figure out what is wrong with it. Maybe the Array.push() method doesn't work or perhaps the for loops.

function largestOfFour(arr) {
    var main = [];
    for(k=0;k<arr.length;k++){
       var long= 0;
         for(i=0;i<arr[k].length;i++){
            if(arr[k][i]<long) {
                arr[k][i] = long;
            }
            main.push[long];
        }
    }
    return main
}

largestOfFour([[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]], "");
Spikatrix
  • 20,225
  • 7
  • 37
  • 83
Lavios
  • 1,169
  • 10
  • 22

4 Answers4

14

The problem is on the inner loop, when you try to find the max value for each array. On each iteration of the outer loop, you should reset long = arr[k][0]. It should not be reset to 0 since the max value may be smaller than 0. Note that this expects all subarrays to have at least one item.

As noted by @edc65, the declaration of long should occur at the start of the function to make it clear that long, as all local variables, has a function scope.

You only want one value per subarray. Therefore you should be adding one value for each iteration of the outer loop (main.push should be in the outer loop). The way it is currently, you are adding one value per subarray element.

In the if statement, your assignment is inverted. It should be

long = arr[k][i];

And the condition is also inverted. long stores the max value for each subarray. Therefore, you update it if you find a value greater than it:

if(arr[k][i]>long) {
    long = arr[k][i];
}

When pushing into the array use parenthesis, not brackets:

main.push(long);

Parenthesis are for calling methods. Brackets are for accessing properties in an object.

Final code

function largestOfFour(arr) {
    var main = [];
    var long;
    for(k=0;k<arr.length;k++){
       long = arr[k][0];
         for(i=0;i<arr[k].length;i++){
            if(arr[k][i]>long) {
                long = arr[k][i];
            }
        }
        main.push(long);
    }
    return main;
}

Math.max method

You can use Math.max to simplify your code

function largestOfFour(arr) {
    var main = [];
    for(k=0;k<arr.length;k++){
        var long = Math.max.apply(null, arr[k]);
        main.push(long);
    }
    return main;
}

As per @BillyMoon's and @Tushar's answers, this can be further simplified to an Array.map call.

Community
  • 1
  • 1
Fernando Matsumoto
  • 2,697
  • 1
  • 18
  • 24
  • I'd go with `long = arr[k][0]` instead of `long = 0` when you reset it. This will handle the case where the numbers are all negative. – Jason Sep 20 '15 at 12:28
  • having var long... inside the for is a (minor) problem too. All the local variables are at function scope and should be declared at top. Instead it seems that long in scoped inside to for(), that is not – edc65 Sep 20 '15 at 22:44
6

I know the question here is to find a bug in the existing code, in case if you may want to optimise the code

The original idea is of @thefourtheye. I'm just explaining this here.

No need of nested loops, you can achieve this in single line.

var arr = [[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]];

var result = arr.map(Math.max.apply.bind(Math.max, null));

document.write(result);
console.log(result);

How this works?

The array.map function is iterating over each of the elements from array on which it is called. The function passed to the map here is apply with its this context bound to the Math.max and first argument bound to null.

Math.max.apply.bind(Math.max, null) this basically calls the Math.max function on array as 

Math.max.apply(null, array);

Update:

With ES6, arrow function and spread operator, this can be made even smaller

arr.map(e => Math.max(...e))


var arr = [[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]];

var result = arr.map(e => Math.max(...e));

document.write(result);
console.log(result);
Community
  • 1
  • 1
Tushar
  • 85,780
  • 21
  • 159
  • 179
3

Potentially simpler method to achieve same result - simplicity is prerequisite for reliability...

function largestOfFour(arr){
    // assumes compatible browser, or shim: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/map#Browser_compatibility
    // map array each element into new value based on callback's return
    return arr.map(function(subarr){
        // sort to get highest value at front, and then return it
        return subarr.sort(function(a,b){
            return b-a;
        })[0];
    });
}

or with Math.max (see comments...)

function largestOfFour(arr){
    // assumes compatible browser, or shim: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/map#Browser_compatibility
    // map array each element into new value based on callback's return
    return arr.map(function(subarr){
        // sort to get highest value at front, and then return it
        return Math.max.apply(null, subarr);
    });
}
Billy Moon
  • 57,113
  • 24
  • 136
  • 237
  • You should use `Math.max` instead of sorting the array (both for efficiency and immutability reasons) – Bergi Sep 20 '15 at 12:30
0

I have been through the code and ended with other solution. The first for loop iterates through the big array and the second one through the components of the subarray.

function largestOfFour(arr) {
  var main = [];
  for(k=0;k<arr.length;k++){
     var long=0;
       for(i=0;i<arr[k].length;i++){
          if(long<arr[k][i]) {
              long=arr[k][i];
          }
       }
   main.push(long);
   }
  return main;
}
feralamillo
  • 819
  • 8
  • 10