Most Common Mistakes in C# Interview Pull Requests

I often hire C# developers on different positions. One of the part of the interview is a technical code challenge in the form of a pull request. I ask the candidate to write a simple feature that usually involves fetching some data using Entity Framework.

Here is some recompilation of the most common mistakes and best practices I recommend when reviewing them.

LINQ First() vs Single()

var order = await DbContext.Orders
    .Where(x => x.Id == orderId)
    .FirstOrDefault(ct);

This one is very common mistake one should avoid. The main issue is the misleading intent.

Is more than one order expected with the particular orderId? In most case it is straight no.

What you want to do always is:

var order = await DbContext.Orders
    .Where(x => x.Id == orderId)
    .SingleOrDefault(ct);

Can't this be static?

Let's say you want to filter out some orders based on certain filters.

var visibleOrders = new List<OrderStatus>()
{
    OrderStatus.InProgress,
    OrderStatus.Shipped,
    OrderStatus.Delivered,
    OrderStatus.Confirmed
};

var orders = await DbContext.Orders
    .Where(x => visibleOrders .Contains(x.Status))
    .Where(x => x.UserId == userId)
    .ToListAync(ct);

Even experienced developers miss this out, visibleOrders should be static which allows all class instances share the exact copy of it.

private static visibleOrders = new List<OrderStatus>()
{
    OrderStatus.InProgress,
    OrderStatus.Shipped,
    OrderStatus.Delivered,
    OrderStatus.Confirmed
};

Are you going to update that entity?

So here we are getting an order. Are you going to update that order here?

var order = await DbContext.Orders
    .Where(x => x.Id == orderId)
    .SingleOrDefault(ct);

If the answer is "No". You should always add:

var order = await DbContext.Orders
    .AsNoTracking()
    .Where(x => x.Id == orderId)
    .SingleOrDefault(ct);

This makes sure that entities returned will not be cached in the DbContext or ObjectContext.

This is a good practice as it improves query performance.

Is that thing empty or null or does it have anything?

Let's check if the string is not empty:

if (string.IsNullOrEmpty(phoneNumber))
{

}

It's a good practice to never use this and instead use:

if (string.IsNullOrWhiteSpace(phoneNumber))
{

}

The method IsNullOrWhiteSpace covers IsNullOrEmpty, but it also returns true if the string contains only white space characters making it the true empty string checker!

Just updating a few records here and there?

Let's say we want to update several records at the same time:

var orders = await DbContext.Orders
    .Where(x => x.UserId == userId)
    .Take(50)
    .ToListAync(ct);

foreach (var order in orders)
{
    order.Status = OrderStatus.Confirmed;
    order.ModifiedDateUtc = DateTime.UtcNow;

    await DbContext.SaveChangesAsync(ct);
}

This is one of the biggest performance sins! You would never save changes inside the loop. Saving inside a loop in our example creates 50 requests to the database.

var orders = await DbContext.Orders
    .Where(x => x.UserId == userId)
    .Take(50)
    .ToListAync(ct);

foreach (var order in orders)
{
    order.Status = OrderStatus.Confirmed;
    order.ModifiedDate = DateTime.Now;
}

await DbContext.SaveChangesAsync(ct);

Here is much faster approach as the Entity Framework will send all 50 updates once.

Good luck!

and..

"May your coffee be strong and your bugs be weak!"

Comments

[deexter]

Actually you are wrong, in entity framework firstOrDefault is faster then SingleOrDefault and it makes sence, but dependends on your index and usecase

[Artur Kedzior]

Well actually I have never mentioned the performance. My focus was on a misleading intent.

However, if we are talking about performance technically are right First can potentially be faster than Single but again as you say it depends if you query by indexed property.

In case of this article we query by order Id which in 99% of the cases would be a primary key which means it would be indexed by default.

So in my case the performance gain is so insignificant that it makes more sense to have a better clarity in the code.

In any case, it's great you have pointed that out and I hope I was able to clarify it from my point of view.