CodeSOD: A Blacklisted Senior

Damien has the "pleasure" of supporting JavaScript code which runs, not in a browser or Node, but inside of a proprietary runtime specialized in handling high-performance collection datatypes. You can write extremely fast code, so long as you're using the collection types correctly. This is good, because a lot of those JavaScript blocks have to be executed for every single request. Milliseconds of execution time add up faster than you think.

One of Damien's senior peers needed to add some code that would filter fields out of a response. Data fetched from the database would be compared against a blacklist of fields to exclude- those fields should be removed from the collection.

This is a relatively simple task, and one that has, in other variations, already been implemented in the codebase. Some of those proprietary enhancements are faster implementations of types like Set, so the basic approach would be:

1. Load the blacklist into a Set
2. Iterate over the items in reverse order
3. Check if each item is in the set, and if so, remove it from the list using removeAt

The Set is fast. By iterating in reverse order, you can change the length of the list without disrupting the for loop. removeAt is a direct access, which is also fast. Since performance is a concern, this is a pretty good solution.

The senior developer came up with this:

function getFilteredItems() { var items = getItems(); var blacklist = config.getBlacklist(); Object.keys(blacklist).forEach(function (blacklistElement) { if (blacklist[blacklistElement]) { items.toArray().forEach(function (item) { if (item.tag === blacklistElement) { items.remove(item); } }); } }); return items;}

getBlacklist was implemented by this developer, and thus they could have done anything they wanted. What they chose to do was return an object where keys were the field names, and values were booleans. The blacklist could contain hundreds of items.

So we iterate across each key, keeping in mind that some of these keys may potentially be false, and thus should be skipped anyway. Then, for each key in the blacklist, we iterate across each item in our items array, turning something that should have been a linear operation into a quadratic operation. The items array could have hundreds or hundreds of thousands of items, depending on the operation.

But we don't just iterate across the items array. We iterate across items.toArray(), a copy of the items array. So that bloats the runtime and the memory footprint, especially since we need to do that for each blacklist item. But then that's necessary, since we use items.remove to remove the item in place- something that we couldn't do from inside of a forEach because changing the size of a collection while iterating across the collection can be tricky (if you don't use the solution we mentioned above).

But items.remove is what makes this even worse. Because items.remove does a linear search through the array to find the target reference. So, we iterate across the blacklist. For each item in the blacklist, we iterate across the entire list of items, then for each item we need to remove, we iterate across the items again. And all this assumes that the toArray cast is cleverly implemented using a memcopy approach and doesn't have to actually iterate the array, which is probably true.

Even if performance doesn't matter, that's a pretty WTF way to implement that, even if you're a "senior developer".

[Advertisement] ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!