current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

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
up vote 6 down vote favorite
1

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
up vote 5 down vote accepted

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
    
thanks. This is really helpful. good things to keep in mind as I grow – Alex G Jun 15 at 14:50
up vote 7 down vote

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 Jun 14 at 20:34
    
@Jonah ES6 and FreeCodeCamp don't always play nice together though. But yes, this could be re-writen like that. – Mast Jun 14 at 20:46
    
@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 Jun 14 at 21:18
    
@Jonah Care to explain that a little more? I am new to callback functions. – Alex G Jun 14 at 21:18
1  
Does this explanation help? – 200_success♦ Jun 14 at 21:21
up vote 1 down vote

Just to add to the answers above who have already said what i'd say, i just wanted to give you a few pointers how to help yourself along

Well, this is the solution i'd use...

function largestOfFour(arr) {
   var largest = [];
   for(var c=0;c<arr.length;c++) {
       largest.push(arr[c].sort(function(a,b){return a<b;}).shift());
   } 
   return largest;
}

I'd make more research in the native functions javascript provides instead of making your own functions for finding things. There really is no need to re-invent the wheel twice.

If you have chrome for example, assign a value to a variable, then type the variable and hit a .

Then see what google chrome autocompletes.

enter image description here

Then experiment with those functions and read up on them what they do. And you can come to those short solutions you've seen answered here.

share|improve this answer
    
Note that sorting the list is O(n log n), which is more work than the O(n) necessary to extract the maximum value of a list. – 200_success♦ Jun 15 at 9:00
    
true, but that limitation wasn't stated to keep in mind the complexity. If we'd be iterating hundreds of thousands of numbers i'd use a different algorithem. but for the small samples provided i'd use this. – Michael Dibbets Jun 15 at 9:03

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.