Cleaner code with no loops

A well-meaning friend of mine came to me with this mess and wondered why it was so bad. He is doing some online javascript training and this code is from a challenge he was doing. It looks to be returning the correct result but still being marked as failing. It looks to me like there is something not-quite-right about it.

I told him the only way you’re going to get a job in this industry (and he is looking), is to stop sucking so bad. (Okay, I didn’t actually say that and he told me to write it.)

var totalRes = []
function flattenArray(arr) {
  for(var i = 0;i<arr.length;i++) {
    if(Array.isArray(arr[i])) {
      flattenArray(arr[i])
    }
    else {
      totalRes = totalRes.concat(arr[i])
    }
  }
  return totalRes
}

function steamrollArray(arr) {
  return flattenArray(arr)
}

steamrollArray([1, [2], [3, [[4]]]])
// [1, 2, 3, 4]

Even though this appears to return the correct result, let’s just refactor it anyway and see if the problem resolves itself by mopping up this pile of turd.

I thought this would be a good example of how we can refactor away from loops and use functions instead. I’m not a fan of loops, especially in javascript where scoping can be confusing for plebs like me.

So, first things first — there is a variable declaration outside of all the functions. Since he already has a second function being called because he is using recursion, I’m just going to get rid of the variable declaration completely and pass the initial value in as an argument.

function steamrollArray(arr) {
  return flattenArray(arr, [])
}

function flattenArray(arr, res) {
  for (var i = 0; i < arr.length; i++) {
    if (Array.isArray(arr[i])) {
      res = flattenArray(arr[i], res)
    } else {
      res = res.concat(arr[i])
    }
  }
  
  return res
}

steamrollArray([1, [2], [3, [[4]]]])
// [1, 2, 3, 4]

This is already starting to look better while eliminating some possibilities for confusing scope issues.

If you are using a loop to iterate over a collection of some sort, another quick way to potentially get rid of weird scoping issues is to use an iterating function instead.

This is not really necessary here since we’ve already got the loop wrapped in a function and we’re not declaring any variables, but let’s do it just for the exercise. The easiest way here would be to switch out for for forEach

function steamrollArray(arr) {
  return flattenArray(arr, [])
}

function flattenArray(arr, res) {
  arr.forEach(function(item) {
    if (Array.isArray(item)) {
      res = flattenArray(item, res)
    } else {
      res = res.concat(item)
    }
  })
  
  return res
}

steamrollArray([1, [2], [3, [[4]]]])
// [1, 2, 3, 4]

That’s okay, but we’re still not done, since this is a good use case for reduce, let’s rewrite this using reduce.

function steamrollArray(arr) {
  if (!Array.isArray(arr)) {
    return arr
  }
  
  return arr.reduce(function(acc, item) {
    return acc.concat(steamrollArray(item))
  }, [])
}

steamrollArray([1, [2], [3, [[4]]]])
// [1, 2, 3, 4]

Now, that is a lot cleaner, in my opinion. No need for loops. This is a good solution for arrays that are not extremely large. If you were expecting a large array, then you might want to tweak a few things and we would also want to refactor this more to take advantage of ES6 tail call optimization (don’t ask me about current support for this).

It also now passes the challenge tests. ᕙ(⇀‸↼‶)ᕗ

Thanks for following along. Please leave any comments, questions, or feedback below.