Refactoring code smells with array functions

We spend a lot more time reading code than writing code, and sometimes what we write has a weird feel to it. Let's examine what are some common mistakes and code smells that we might make!

In this article I'll explore mainly array functions, but I'm aiming to add a few more articles and turn this into a series tackling other more general topics.

If we search for the definition of a code smell we find it described as:

Any characteristic in the source code of a program that possibly indicates a deeper problem.

In this series of articles, I'm going to include a few more things that some people may not find as being literal code smells, but improve the general feel of the code.

Let's start by seeing how some array functions can help us improve the quality of our code:

Example

function myAwesomeFunction(array) {
    const result = [];
    array.forEach(element => {
        if (businessLogic(element)) { // businessLogic returns a boolean
            result.push(element);
        }
    });
    return result;
}

Explanation and fix

The reason I consider this a code smell is because this entire thing is a glorified filter operation.

function myAwesomeFunction(array) {
    return array.filter(element => businessLogic);
}

Seems rather succint, right? There are other benefits to writing it like this, in spite of the fact that if I were to manually loop through the array using a for or while the peformance would be higher, namely:

1. The intent is a bit clearer
2. Less error-prone, unless you use snippets or some other form of code generation.
3. Easily parsed and understood by humans - if you know what the filter function does, which you do now!

If your code tends to start with declaring an empty array, you might want to use a reduce or filter. For more information on this topic I have a an article detailing some more of these fancy array functions.

Next, let's tackle some conditionals. Buckle up!

Example

function checkUserAllowed(userType) {
    if (userType === 'ADMIN' || userType === 'SUPER_ADMIN') {
        return true;
    }
    return false;
}

Explanation and fix

If you reverse the way you think of this problem you end up with something like this:

function checkUserAllowed(userType) {
    const allowedUserTypes = ['ADMIN', 'SUPER_ADMIN'];
    return allowedUserTypes.includes(userType);
}

Again the main benefit is readability. It also looks a lot more approachable.

Example

const users = [
    {name: 'Billy', role: 'ADMIN'},
    {name: 'Ray', role: 'USER'}
]
function checkAllUsersAreAdmins(users) {
    let areAllAdmins = true;
    for (let user of users) {
        if (user.role !== 'ADMIN') {
            areAllAdmins = false;
            break;
        }
    }

    return areAllAdmins;
}

Explanation and fix

You can make the argument that you can use return false instead of setting the variable to false and having a default return true. Some people might even not write the break statement! But I digress!

function checkAllUsersAreAdmins(users) {
    return users.every(user => user.role === 'ADMIN');
}

Now, you can also make the argument towards a higher-ordered function taking in a role, but this will probably be handled in another post.

So why this form? Well, every and some also quickly exit, so it's just as performant (ish), but more importantly, readability.

Next let's look at something that some people still do, albeit rarely.

Example

let data = [...];
data = data.map(...);
data = data.filter(...);
data = data.reduce(...);

Explanation and fix

const data = [...];
const result = data.map(...);
                   .filter(...);
                   .reduce(...);

We don't need to reassign the variable unless we need the intermediate values. Also one other thing to note, in this version of code we don't mutate the original array, which is something we generally want to avoid since we're human and might forget where we use that array reference.

The astute readers will also notice that we can do all three operations as part of a single reduce, this will speed things up, but might make the resulting code a bit harder to grok. Pick your poison wisely.

Additional thoughts

In some instances the imperative version may be desireable, especially when optimizing heavily. This shouldn't be the norm though, and clarity should be preferred. This, as you can probably notice, is a recurring theme across this article, prioritizing readability and clarity over explicitness.