Find top two clothing sizes in the array

Question

I was recently asked this question in an interview. Here is the solution I came up with. Please let know if I could have done this differently or in a more efficient manner.

The question is as follows:

Determine The Top Two Clothes Sizes In The Following Array

["XS","S","S","M","L","XL","XLL","XL","XL","M","M","M","XS","XS","XS","XS"]

The solution should be [ 'XS', 'M' ] since XS occurs 5 times and M occurs 4 times.

function determineTopTwo(arr){
let map = {};
let resultArr = [];
let result = [];
for(let i = 0; i < arr.length; i++){
    if(!map[arr[i]]){
        map[arr[i]]=1;
    }else{
        map[arr[i]]++;
    }
}

for(key in map){
    resultArr.push([map[key],key]);
}
let newArr = resultArr.sort((a,b)=>{
    return a[0]-b[0];
});

result.push(newArr[newArr.length-1][1], newArr[newArr.length-2][1]);
return result;
}

Answer 1

You didn't specify if you were allowed to use ecmascript-6 features or not but presumably you were, since you used let and arrow functions. As an interviewer, I would note that you used those features, yet you iterated over the array using a regular for loop instead of using for...of. That isn't necessarily a bad thing since it demonstrates that you know how to increment a counter in a standard loop and then use that for indexing into the array, but you don't have to if you use a for...of loop.

The technique for sorting the array is good, though because Array.prototype.sort() "sorts the elements of an array in place and returns the array"¹ there isn't really a need to store the value in newArr because resultArr is sorted.

let newArr = resultArr.sort((a,b)=>{
    return a[0]-b[0];
});

You could have just returned the first two elements of resultArr.

Also, const could have been used for any variable that is never re-assigned - including arrays that merely have elements pushed into them - to avoid accidental re-assignment.

¹_{https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort}

Answer 2

Two bugs

There is a bug when you have only less than two unique sizes. Trying to access an undefined array newArr[newArr.length-1][1], newArr[newArr.length-2][1] throws if newArray.length < 2

Bug in hiding waiting to pounce. The variable key has not been declared and thus has been created in the global scope. This is bad mistake and can result in very hard to find bugs in totally unrelated code. You should use "use strict" to catch such errors early.

General review notes

• Spaces between operators , spaces after for, if, else, ,, and => and befor else and =>
• There is no need to create the array result, you can create it as you return it. EG return [newArr[newArr.length-1][1], newArr[newArr.length-2][1]]; or return [newArr.pop()[1], newArr.pop()[1]];
• The arrays and map should be constants as they do not change.
• Try to avoid using for in loops they have some problems that make them next to useless. Rather use for of
• Converting the map to an array and then sorting, just to find the top to is overkill. Sorts are complex, and you have already passed over each item, and thus know what the max values are.
• Naming mapis not a great name. It is a map that holds counts, so counts is the better name
• For simple if else statements use the simplest form you can. eg you have if (!foo) {/* do not foo */} else {...} the not ! is redundant if you reverse the order to if (foo) {... } else {/* do not foo */}
• Use the shorter form of arrow function if you are just returning a value. .sort((a, b) => { return a[0] - b[0] }); can be .sort((a, b) => a[0] - b[0]);
• Sort sorts in place, you don't need to passing it to another array.

Rewrite using your approach

If there are no items to produce any of the top two then returns undefined in its place. Needs to check the sorted array size to see if it can return any values.

Note that because it pushes the entries directly to the sorted array the count and items have switched places in the arrays.

function findTopTwo(arr) {
    "use strict";
    const counts = {}, sorted = [];
    for (const item of arr) {
        counts[item] = counts[item] ? counts[item] + 1 : 1;
    }

    sorted.push(...Object.entries(counts));
    if (sorted.length === 0) { return [] }
    if (sorted.length === 1) { return [sorted[0][0]] }

    sorted.sort((a, b) => a[1] - b[1]);    
    return [sorted.pop()[0], sorted.pop()[0]];
}

Avoiding the sort

This looks for the top two as you count them. It avoids the sort so is less complex and uses less memory.

function findTopTwo(arr) {
    var maxA = 0, maxB = 0, itemA, itemB;
    const counts = {};
    for (const item of arr) {
        const count = (counts[item] = counts[item] ? counts[item] + 1 : 1);
        if (count > maxA && item !== itemB) {
            if (item !== itemA && maxA > maxB) { [maxB, itemB] = [maxA, itemA] }
            [maxA, itemA] = [count, item];
        } else if (count > maxB && item !== itemA) { [maxB, itemB] = [count, item] }
    }
    return maxA > maxB ? [itemA, itemB] : [itemB, itemA];
}