Over a million developers have joined DZone.
{{announcement.body}}
{{announcement.title}}

PR Review: The Simple Stuff Will Trip You Up

DZone's Guide to

PR Review: The Simple Stuff Will Trip You Up

In PR, this dev ran into an issue that seemed small and easy to deal with but ended up causing a bit of a headache. Read on for more!

Free Resource

RavenDB vs MongoDB: Which is Better? This White Paper compares the two leading NoSQL Document Databases on 9 features to find out which is the best solution for your next project.  

In a recent PR, I ran 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:

image

image

It also has 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:

image

image

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.

image

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:

image

image

And now we don’t have O(N) anymore.

Although I’ll admit that for such small size, it probably doesn’t matter.

Get comfortable using NoSQL in a free, self-directed learning course provided by RavenDB. Learn to create fully-functional real-world programs on NoSQL Databases. Register today.

Topics:
performance ,database performance ,pr review

Published at DZone with permission of

Opinions expressed by DZone contributors are their own.

{{ parent.title || parent.header.title}}

{{ parent.tldr }}

{{ parent.urlSource.name }}