TypeScript review

I have the following TS code:
const input: Array<string | number> = [3, "a", "a", "a", 2, 3, "a", 3, "a", 2, 4, 9, 3];

const convertArrayToMap = (inputArray: Array<string | number>): Map<string | number, number> => {
const convertedArray: Array<[string | number, number]> = inputArray.map((item) => [item, 0]);

return new Map<string | number, number>(convertedArray);
};
const input: Array<string | number> = [3, "a", "a", "a", 2, 3, "a", 3, "a", 2, 4, 9, 3];

const convertArrayToMap = (inputArray: Array<string | number>): Map<string | number, number> => {
const convertedArray: Array<[string | number, number]> = inputArray.map((item) => [item, 0]);

return new Map<string | number, number>(convertedArray);
};
I'm trying to take an array of strings and numbers as an input and convert it to a map. I got this code working, but it took me almost an hour and a half and seems incredibly convoluted for the task. Surely there's a better way?
22 Replies
vince
vince•9mo ago
Also feel free to roast this. Honestly embarassing how convoluted I made this so I would love some feedback. I converted array to map to object lol... feels really dumb
/**
* Write a JavaScript program to find the most frequent item in an array.
*
* Sample array: const arr = [3, 'a', 'a', 'a', 2, 3, 'a', 3, 'a', 2, 4, 9, 3];
* Sample Output: a: 5 times
*/

const input = [3, "a", "a", "a", 2, 3, "a", 3, "a", 2, 4, 9, 3];

const findMostFrequentItem = <T>(array: Array<T>): T => {
const map = convertArrayToMap<T>(array);

// Counts number of occurences
array.forEach((item) => {
map.set(item, (map.get(item) as number) + 1);
});

const mapObject = Object.fromEntries(map);

// Finds the max value from mapObject
const maxValue = Object.values<number>(mapObject).reduce((accumulator: number, current: number) =>
Math.max(accumulator, current)
);

// Finds the key using maxValue
const maxValueKey = Object.keys(mapObject).find((key) => mapObject[key] === maxValue);

return maxValueKey as T;
};

const convertArrayToMap = <T>(inputArray: Array<T>): Map<T, number> => {
const convertedArray: Array<[T, number]> = inputArray.map((item) => [item, 0]);

return new Map<T, number>(convertedArray);
};

(function run(array): void {
const result = findMostFrequentItem(array);
console.log(result);
})(input); // Let TypeScript infer the generic T type
/**
* Write a JavaScript program to find the most frequent item in an array.
*
* Sample array: const arr = [3, 'a', 'a', 'a', 2, 3, 'a', 3, 'a', 2, 4, 9, 3];
* Sample Output: a: 5 times
*/

const input = [3, "a", "a", "a", 2, 3, "a", 3, "a", 2, 4, 9, 3];

const findMostFrequentItem = <T>(array: Array<T>): T => {
const map = convertArrayToMap<T>(array);

// Counts number of occurences
array.forEach((item) => {
map.set(item, (map.get(item) as number) + 1);
});

const mapObject = Object.fromEntries(map);

// Finds the max value from mapObject
const maxValue = Object.values<number>(mapObject).reduce((accumulator: number, current: number) =>
Math.max(accumulator, current)
);

// Finds the key using maxValue
const maxValueKey = Object.keys(mapObject).find((key) => mapObject[key] === maxValue);

return maxValueKey as T;
};

const convertArrayToMap = <T>(inputArray: Array<T>): Map<T, number> => {
const convertedArray: Array<[T, number]> = inputArray.map((item) => [item, 0]);

return new Map<T, number>(convertedArray);
};

(function run(array): void {
const result = findMostFrequentItem(array);
console.log(result);
})(input); // Let TypeScript infer the generic T type
I know you can do this in like 1 or 2 lines as well I just don't know how without looking it up lol Oh you can just use 2 for loops 😅
Rägnar O'ock
Rägnar O'ock•9mo ago
You can remove the type on the input constant, it will be inferred automatically from the value. Same for the const convertedArray. And I'm pretty sure you don't need the generic parameters on the map you are returning either. As for the code itself, while map would have better performance on a huge array (multiple thousands of items) it just makes the code messier here. Like map.set(item, (map.get(item) as number) + 1) could be replaced by obj[item] += 1
vince
vince•9mo ago
Yes! Thank you so much. I knew the code was definitely quite shite but I wanted to try it without looking up a solution 😂
Rägnar O'ock
Rägnar O'ock•9mo ago
For the maxValue you can just do Math.max(...Object.values(obj))
vince
vince•9mo ago
That's so sick you can do that When I get time tomorrow I'll rewrite it and send it here
Rägnar O'ock
Rägnar O'ock•9mo ago
I'm sure there's other stuff that can be simplified, but on a phone and without syntax highlighting I don't see them
Joao
Joao•9mo ago
I'm not sure what the first function is supposed to do but one little trick to simplify the TS code is, as mentioned by Ragnar above, to let it infer as much as possible.
const convertArrayToMap = <T extends (string|number)>(inputArray: T[]) => {

const arr = inputArray.map((item) => [item, 0]);

const map = new Map<T, number>(arr);
}
const convertArrayToMap = <T extends (string|number)>(inputArray: T[]) => {

const arr = inputArray.map((item) => [item, 0]);

const map = new Map<T, number>(arr);
}
This code doesn't actually work but I suspect it's because the conversion that you are doing is somewhat off... However, from a typing perspective this simplifies things a lot. What it says is that convertArrayToMap accepts one parameter, an array, and it's going to infer it's type without you specifying it. The T extends whatever is a restriction to limit T to whatever type you want, strings or numbers in this case. It's also reusing this type for the key of the Map. Oh, I see. Didn't know you could construct a Map using an iterable. I personally would just use a for loop of some kind, to make it more readable.
Joao
Joao•9mo ago
Good exercise! As you can see most of the effort goes into removing TypeScript annotations 😅 Let it earn it's salary and infer all that it can, you only really want to give you hints when you're doing something wrong or explicitly checking for it i.e., hovering over some variable. Otherwise it gets really annoying, really fast. Also, I'm not using the convert array to map in this particular example as otherwise you'd be looping multiple times over the same array but this is not really needed. You should also consider what happens when an empty array is provided. If you absolutely expect something to be returned, better to throw an error early on.
Rägnar O'ock
Rägnar O'ock•9mo ago
I did it another way
Rägnar O'ock
Rägnar O'ock•9mo ago
it could be simpler but I wanted to try to skip ahead when possible by checking if the next set of items was longer than the current one in a sorted view of the input array and there's only 56 caracters total that are typescript annotations (excluding spaces)
vince
vince•9mo ago
Oh shoot I gotta look at all this stuff. Thanks guys! I'll get back to you sometime soon. Kind of got swamped all of a sudden with this contract work and some other work 😂
Rägnar O'ock
Rägnar O'ock•9mo ago
(please note that my implementation is uselessly complexe just for the sake of it)
vince
vince•9mo ago
😂 I had to paste it into vscode and use the debugger to understand what's going on. I think I understand most of it, I'll still need to review it a bit more. I need to look at Joao's too! I wouldn't have come up with that on my own but I like how efficient it is It should be O(n) right?
Rägnar O'ock
Rägnar O'ock•9mo ago
well... it might not be actually be efficient because of the sort function...
vince
vince•9mo ago
Yea that's true, I was thinking that as well. The for loop is efficient though
Rägnar O'ock
Rägnar O'ock•9mo ago
yeah
vince
vince•9mo ago
What's the runtime for .sort()? Is it in place? Idk how they implement it
Rägnar O'ock
Rägnar O'ock•9mo ago
it's at the discretion of the implementation IIRC as long as it sorts the array it's ok tho it must be something like O(n log n) so... depending on the entropy of the input, the number of items and the number of different elements my emplementation will be almost always slower because it has to sort first
vince
vince•9mo ago
Got it! I really like this. Super simple! Thank you 🙂 Sorry it took a while for me to get around to it
Joao
Joao•9mo ago
No worries!