Hacker News Reader

Comment by jrs95 | Hacker News Reader

It's obvious to some of us, but not all of us. I see things way worse than this on a fairly regular basis.

I guess - but I feel like you'd call it an "error" rather than a "smell". Better to just teach people data structures rather than which constructs they should be afraid of. Approaching it "sideways" like this just leads to people going "I heard that loops can be bad, so I copied and pasted that line 25 times instead".

Its a smell and not an error because it works perfectly fine and it actually is perfectly fine for now because the n for that linear search is low.

Sets have a very specific use case: they only contain unique values and (unless using a specific implementation) they don't have any defined order. Switching from arrays to sets just to get rid of an iteration doesn't sound like fixing a smell to me. Also, insertion, iteration and memory usage are less efficient for a set.

Importantly Sets have different semantics than Lists. Creating a set is an information losing process. A list converted to set cannot be recovered, but a set converted to a list is easily recovered.

I don't think this is handled well in TFA. Because of some dubious iteration at the call site, the author changes the semantics of getLoadNames(), blithely assuming that duplicates should not be allowed and order is not important.

The author mentions that the function is called at two other places. For all we know the original author was aware of Sets but chose List because it more correctly matched the use of the structure.

Reply on news.ycombinator.com