Hey. I’ve just completed Return the largest numbers in array and would love to hear some feedback about my code below:
function largestOfFour(arr) {
var firstPiece=[];
for (var x=0; x<arr.length;x++) {
arr[x].sort(function(a, b) {
return b - a;
});
firstPiece.push(arr[x][0]);
}
return firstPiece;
}
largestOfFour([[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]]);
Hey, if it works, right?
I would question whether or not a sort would be the best practice here. It seems like overkill. It seems that just finding the largest would be sufficient. It may not seem like much, but if this were a very large array, that could make a huge difference.
Looking back at my original code, I have:
function largestOfFour(arr) {
for (var i = 0 ; i < arr.length ; i++) {
var biggest = 0;
for (var j = 0 ; j < arr[i].length ; j++) {
biggest=Math.max(biggest, arr[i][j]);
}
arr[i] = biggest;
}
return arr;
}
It’s similar to what you’re doing it’s just that in the inner loop I’m searching for the largest instead of sorting. And then I’m writing the answer right back onto the original array. They do the same basic thing, I just still think looking for the largest is going to be O(n) and sorting is going to be O(n log n) at best. On a monstrously large array that could make a difference. It’s worthwhile to think about these things sometimes.
Knowing what I know now, I might have gone for
function largestOfFour(arr) {
return arr.map(function (subArr){
return subArr.reduce(function (prev, cur) {
return (cur > prev) ? cur : prev;
}, 0);
});
}
Using the map function instead of a for loop isn’t necessarily any better or faster (I think) but one could argue that it’s a little cleaner.
Of course, we can use Math.max instead:
function largestOfFour(arr) {
return arr.map(function (subArr){
return Math.max.apply(null, subArr);
});
}
That works too.
I don’t think ES6 is allowed on the fCC challenges, but if it were, we could use the spread operator:
function largestOfFour(arr) {
return arr.map(function (subArr){
return Math.max(...subArr);
});
}
Which is the best? I don’t know. Someone smarter than me could figure out which is faster - though again it won’t make enough of a difference to matter with small arrays like this. And a big question is what is more readable? Sometimes compactness isn’t the best option if you sacrifice readability. I just thought I’d show you some other options.
ES6 is just JS, there nothing stopping you using it in FCC. IMO the map/…Math.max is the most readable by a fairly long distance, though better probably as return arr.map(subarr => Math.max(...subArr)). You need to know the syntax, but it’s clear + has absolutely the least amount of code between input->output, so much less chance of buggy code or errors.
A for/while loop with no sorting is likely fastest; there is no need to run sort if at all concerned about efficiency - the entre array has to be traversed anyway, so pointless doing what could be an expensive sort. Sort will be very fast for integer arrays though, so of all the solutions, the OPs may be the quickest (tho depending on how optimised Math.max is; .call and .apply are definitely slow, but if the spread operator has had native optimization by now it might be quick).
HUGE thanks to you for this detailed explanation. Helped me to see the task a little differently than before.
I am a bit clumsy with map right now, will learn it, but tnx anyway.
Yeah, things like map and the like took a while to get my head around. I just spent a day using them. Anytime you’re looping through and array, there’s probably a way to do it with a function.
That’s true, but there is a warning that pops up when you try the spread operator in the FCC challenge. It still works, but it warns that it is ES6.
/* jshint esversion: 6 */ at the top of the code to remove the warnings, you just need to tell the linter that you’re using ES6; as you say, the code still works
It is my first long answer
function largestOfFour(arr) {
// You can do this!
var nar = [];
var max1 = [0];
var max2 = [0];
var max3 = [0];
var max4 = [0];
var i = 0;
for (var n = 0; n < 4; n++) {
if (arr[i][n] > max1) {
max1 = arr[i][n];
}
}
var j = 1;
for (n = 0; n < 4; n++) {
if (arr[j][n] > max2) {
max2 = arr[j][n];
}
}
var k = 2;
for (n = 0; n < 4; n++) {
if (arr[k][n] > max3) {
max3 = arr[k][n];
}
}
var l = 3;
for (n = 0; n < 4; n++) {
if (arr[l][n] > max4) {
max4 = arr[l][n];
}
}
nar.push(max1);
nar.push(max2);
nar.push(max3);
nar.push(max4);
return nar;
}
largestOfFour([[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]]);
Wow, yes this is quite long! Maybe think about how you can refactor this to DRY up your solution with just one for loop or (maybe a nested for loop?). The Math object would also be something worth looking into.
Good luck and happy coding!
Returing to the same task after year when i was completed it i completely forgot how i did it last time so after review your advice i did less code and new trick 
function largestOfFour(arr) {
// You can do this!
var arra = [];
for (var i = 0; i < 4; i++) {
var maxi = Math.max(...arr[i]);
arra.push(maxi);
}
return arra;
}
largestOfFour([[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]]);