PR ReviewThe simple stuff will trip you
In a recent PR, I run into this code, which is used in query generation to decide if we need to quote a particular alias. The code itself is pretty straightforward and easy to follow:
It also have two distinct issues. First, there is the allocation because of the ToUpper call and second, we are doing O(N) search on the alias array every single time.
I asked for a change, to use HashSet and to use the OrdinalIgnoreCase comparer.
Here is the change I got back:
This is exactly what I asked for, and it is very subtly wrong. We are now saving an allocation, which is great, but the problem is with the Contains method.
This looks okay, but this is not HashSet.Contains, instead, this is an extension method from Enumerable.Contains, which iterates over the set and compare each value.
The fix is also simple:
And now we don’t have O(N) anymore.
Although I’ll admit that for such small size, it probably doesn’t matter.