Most Common Mistakes in C# Interview Pull Requests
Tags: csharp, dotnet, codenewbie, coding
Published: 2023-09-21 17:01:10

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()

```csharp

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:

```csharp
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. 

```csharp
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.

```csharp
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?  

```csharp

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

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

```csharp
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:

```csharp

if (string.IsNullOrEmpty(phoneNumber))
{

}
```

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

```csharp

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:

```csharp
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. 

```csharp
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!"