Refactoring function for the least amount of lines

I had the opportunity to review a small piece of code. From the first look, I knew this is a beautiful example ready for refactoring.

I took code review little bit personally and challenged myself to write the least amount of line; if I can write the shortest solution possible.

Following code is a method which based on the event input is searching through the list of roles and produce a filtered list of roles in the dropdown selection for Angular form.

searchRoles(event) {
    let roleList = [];
    this.rolesFiltered = User.ROLES.filter(role =>
        role.label.toLocaleLowerCase().includes(event.query.toLocaleLowerCase());
    if (this.editForm.value.roles) {
        this.rolesFiltered.forEach(filteredRole => {
            const isInForm = this.editForm.value.roles.some(formRole => {
                return filteredRole.value.includes(formRole.value);
            });
            if (!isInForm) {
                roleList.push(filteredRole);
            }
        });
    } else {
        roleList = this.rolesFiltered;
    }
    this.rolesFiltered = roleList;
}

The original code had 18 lines of code. Can you find anything odd with it? Do you think this is the shortest way possible? Read further and if you have any more suggestion or ideas for this piece of code, please share them in comments.

I see at least 3 things which are wrong with this code.

1. There is a loop in a loop. Therefore, finding the roles to display is equal to O(n²). I had a hunch that this can be decreased to down O(n).
2. It seems there is too much unnecessary if-else nesting.
3. Assigning a value on the end of the code seems also as an inappropriate solution. It is connected with a 2nd point, and I also had another hunch this can be mitigated.

searchRoles(event) {
    this.dropdownRoles = User.ROLES.filter(role =>
        role.label.toLocaleLowerCase().includes(event.query.toLocaleLowerCase())
    );
    if (this.editForm.value.roles) {
        let tempRoleList = [];
        this.dropdownRoles.forEach(dropdownRole => {
            if (!this.editForm.value.roles.includes(dropdownRole)) {
              tempRoleList.push(dropdownRole);
            }
        });
        this.dropdownRoles = tempRoleList;
    }
}

First of all, I tried to get rid of the temporary variable. It is necessary now. But only in case if you want to temporary store the case that there is already a role selected and you want to pick up roles from the shortened general list of roles.

The first iteration had 13 lines of code.

searchRoles(event) {
    this.dropdownRoles = User.ROLES.filter(role =>
        role.label.toLocaleLowerCase().includes(event.query.toLocaleLowerCase())
    );
    if (this.editForm.value.roles) {
        var i = this.dropdownRoles.length;
        while (i--) {
            if (this.editForm.value.roles.includes(this.dropdownRoles[i])) {
                this.dropdownRoles.splice(i, 1);
            }
        }
    }
}

Then I have decided that looping over all dropdown roles so it can help me save some more instructions if it is made correctly. Looping backward I also cut off all used roles and I was getting back only unselected roles in the dropdown menu.

Looping backward reduced the lines even more into 12 lines of code.

searchRoles(event) {
    this.dropdownRoles = User.ROLES.filter(role =>
      role.label.toLocaleLowerCase().includes(event.query.toLocaleLowerCase())
    );
    if (this.editForm.value.roles) {
        for (var i = this.dropdownRoles.length - 1; i >= 0; i--) {
            if (this.editForm.value.roles.includes(this.dropdownRoles[i])) {
                this.dropdownRoles.splice(i, 1);
            }
        }
    }
}

I googled the possibility to make this code shorter and eventually it is possible by refactoring while loop and by using TypeScript syntactic sugar to assign variable for dropdown list in for cycle.

Rewriting while to for in TypeScript brought reduction to 11 lines of code.

searchRoles(event) {
    function filterRoles(role, editForm) {
        if (!editForm.includes(role)) {
            return role;
        }
    }
    this.dropdownRoles = User.ROLES.filter(role =>
        role.label.toLocaleLowerCase().includes(event.query.toLocaleLowerCase())
    );
    if (this.editForm.value.roles) {
        this.dropdownRoles = this.dropdownRoles.filter(dropdownRole => filterRoles(dropdownRole, this.editForm.value.roles));
    }
}

When I didn’t know how to minimize it further I asked myself a different question – Is possible to rewrite this code in functional paradigm?

So I turned to functional programming and decided to use the same idea for filtering as in initial querying of user input. I have created a standalone local function which took 2 arguments.

Rewriting the solution in a functional way increased method back to 12 lines of code.

searchRoles(event) {
    this.dropdownRoles = User.ROLES.filter(role =>
        role.label.toLocaleLowerCase().includes(event.query.toLocaleLowerCase())
    );
    if (this.editForm.value.roles) {
        this.dropdownRoles = this.dropdownRoles.filter(dropdownRole => {
            if (!this.editForm.value.roles.includes(dropdownRole)) {
                return dropdownRole;
            }
        });
    }
}

However, removing the function and making it anonymous decreased the code length to 11 lines. As a side-effect, the solution is rewritten more robustly and functionally.

While I played with other versions of functional refactoring, I picked up the last one because it was the most readable solution. You know, less code is better (easier maintenance, less prone to errors, etc.). But we need to remember that not every code is readable code.