Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am working my way through FreeCodeCamp's Javascript challenges and Return Largest Numbers in Arrays took me a minute. In this one the goal was to create an array with the highest values from each array as its elements.

I am currently using for loops to iterate through the various arrays and arrays of arrays. I wanted to see if there was a better way to do this challenge.

function largestOfFour(arr) {
  // You can do this! 
  var largestNum = 0;
  var newArray = [];
  for(var i = 0; i < arr.length; i++){
    for(var k = 0; k < arr.length; k++){   
      console.log(arr[i][k]);
      //create comparison
      a = arr[i][k];
      if(a>largestNum){
        largestNum = a;
        newArray[i] = largestNum;
      }
    }
    largestNum=0;
  }
  console.log('complete Test' + newArray);
  return newArray;
}

largestOfFour([[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]]);

share|improve this question

The challenge says that you will be given exactly four subarrays, but it doesn't specify the length of each subarray. You've assumed a square matrix, which is not stated in the problem.

To find the maximum of 4, 5, 1, and 3, you could write Math.max(4, 5, 1, 3). If those numbers are given to you as an array, you could use Math.max.apply(null, array).

To transform each element of the outer array, use arr.map().

function largestOfFour(arr) {
  return arr.map(function(subArr) {
    return Math.max.apply(null, subArr);
  });
}

console.log(largestOfFour([[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]]));

share|improve this answer
    
Nice. You could even get rid of one more line if you use es6 syntax on the map callback :) – Jonah 6 hours ago
    
@Jonah ES6 and FreeCodeCamp don't always play nice together though. But yes, this could be re-writen like that. – Mast 6 hours ago
    
@200_success: what does the .apply() do to make that work? I have read the documentation and it still isn't clicking for me – Alex G 5 hours ago
    
@Jonah Care to explain that a little more? I am new to callback functions. – Alex G 5 hours ago
    
Does this explanation help? – 200_success 5 hours ago

Bug

  for(var i = 0; i < arr.length; i++){
    for(var k = 0; k < arr.length; k++){   
      console.log(arr[i][k]);

There is a bug here.

  for (var i = 0; i < arr.length; i++) {
    for (var k = 0; k < arr[i].length; k++) {   
      console.log(arr[i][k]);

It only worked because both dimensions were the same. But that doesn't have to be true.

I would also prefer more whitespace to make it easier to read the code.

Nitpick

      console.log(arr[i][k]);

This looks like debugging code to me. You should remove debugging code before sending out for review (or going into production).

Avoid stomping globals

      a = arr[i][k];

If there is an a variable that is already visible in this scope, this will assign over that. That won't break anything in this code, but it could break other code, as its variables may change unexpectedly.

      var a = arr[i][k];

If you use var, it will create a new variable in this scope.

      if(a>largestNum){
        largestNum = a;

In this case, even if you don't switch to the @200_success solution, it would be even easier to just get rid of a.

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

Don't do extra work

You can simplify your interaction with largestNum:

function largestOfFour(arr) {
  var newArray = [];
  for (var i = 0; i < arr.length; i++) {
    var largestNum = 0;
    for (var k = 0; k < arr[i].length; k++) {   
      if (arr[i][k] > largestNum) {
        largestNum = arr[i][k];
        newArray[i] = largestNum;
      }
    }
  }

  return newArray;
}

This both reduces the scope and reduces the number of assignments. You clear largestNum one more time than is actually necessary.

Naming

/* Given a 2D array, return the maximum value from each row.  */
function findMaximums(arr) {
  var maximums = [];
  for (var i = 0; i < arr.length; i++) {
    maximums[i] = 0;
    for (var j = 0; j < arr[i].length; j++) {
      if (arr[i][j] > maximums[i]) {
        maximums[i] = arr[i][j];
      }
    }
  }

  return maximums;
}

I prefer a descriptive name like maximums to a generic name like newArray.

It is more common to use j as a second loop variable and k as the third.

I also prefer findMaximums to LargestOfFour, although that may be set by the challenge.

This seems like the simplest version of your original algorithm.

share|improve this answer

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.