My tips for .NET code-review

Posted on In Word count in article: 19k Reading time ≈ 17 mins.

Code-review is important in our daily development. Some developers may spend lots of time on learning new features of the languages, DDD, distributed system or some fancy stuff but the first thing we should keep in mind is we need to write the robust, maintainable code. Here are some tips from my recent code-review and I hope it would be helpful for you.

NullReferenceException series

The NullReferenceException is really annoying. The best way to avoid it is to check if the variable is null before you use it. Here are some potential issues. I also include other exceptions such as ArgumentNullException here.

Always initialize the collection when you declare it

One common error is sometimes we declared a collection but did not initialize it before using it. Like this:

1
2
3
List<Person> persons;
// Some logics
// If you use persons directly you may got a null object

So the better way is to always initialize the collection when we declare it:

1
2
List<Person> persons = new List<Persons>();
// Then you can do something as usual

Especially if we have a property which has its own private field:

1
2
3
4
5
6
7
// It is good to initialize the private field when we declare it.
private ObservableCollection<Person> _personList = new ObservableCollection<Person>();
public ObservableCollection<Person> PersonList
{
     get => _personList;
     set => SetProperty(ref _personList, value);
}

DO NOT return null values from methods returning collections

Another tip is if we have a method that returns a collection, do not return null. Instead, if there are no satisfied elements, we should return an empty collection.

1
2
3
4
5
public List<Person> GetPersons()
{
	// Some logic to retrieve the objects. If not found, then return an empty collection. DO NOT return null.
    return new List<Person>();
}

FirstOrDefault() or First()?

LINQ has some similar methods like First() and FirstOrDefault(). The key difference is the behavior for the empty collection. If the collection is empty or there is no element that satisfies the condition, then First() will throw InvalidOperationException. For this case, FirstOrDefault() is safe because it can return a default value for an empty collection or no element satisfies the condition. But we should be truly clear if the default value is null - that is another reason which would cause NullReferenceException - The default value for reference and nullable types is null. So it would be better if we do like this:

1
2
3
4
5
6
var firstPerson = persons.FirstOrDefault(x => x.Age > 18);
// Check if the variable is null
if (firstPerson != null)
{
    // Do something to the firstPerson
}

Similarly, Single() and SingleOrDefault() also have the different results. Single() throws InvalidOperationException for these scenarios:

  • The source collection is empty
  • No element satisfies the condition
  • More than one element satisfies the condition

For SingleOrDefault(), it also returns a default value if the collection is empty or no satisfied element is found. We should check if the return value is null before using it.

Another way to handle the empty collections is to use DefaultIfEmpty() extension method, as shown below:

1
2
3
4
List<int> numbers = new List<int> { };
// Setting the default value to 1 by using DefaultIfEmpty() in the query.
int firstNumber = numbers.DefaultIfEmpty(1).First();
// Now the firstNumber is 1, not 0.

FYI: Enumerable.First Enumerable.FirstOrDefault, Enumerable.Single, Enumerable.SingleOrDefault

Checking if the key exists in the dictionary

One probable reason of NullReferenceException is related to the dictionary. I have seen it quite a few times in code-review. Unless we are 100% sure we know the key exists in the dictionary, (eg, the dictionary is hard-coded.) we should always check it first. Because sometimes the dictionary comes from the APIs, or it has other dependencies. We cannot guarantee the key we want exists in the dictionary. So try to use this way:

1
2
3
4
5
6
7
var persons = new Dictionary<string, Person> = new Dictionary<string, Person>();
// Add some values
if (persons.ContainsKey("Jack"))
{
    var person = persons["Jack"];
    // Do something
}

If we do not check the key, we may get a KeyNotFoundException. FYI: Dictionary<TKey,TValue>.Item[TKey], Dictionary<TKey,TValue>.ContainsKey(TKey)

Also, when you add a key/value pair to a dictionary, you should check if the key already exists in the dictionary.

as or is?

When we cast the object type, we may use as, for example:

1
2
3
4
5
protected override void SetValueImpl(object target, SomeStatus value)
{
    var ctrl = target as ControlA;
    ctrl.Status = newValue;
}

The as operator explicitly converts the result of an expression to a given reference or nullable value type. It will not throw exceptions even if the conversion is not possible, but it will return null for this case. So usually, it is safer than the cast expression, but we still need to check if the result is null.

The is operator checks if the runtime type of an expression result is compatible with a given type. So it is safer to use the following code:

1
2
3
4
5
if (target is ControlA)
{
    var ctrl = target as ControlA;
    ctrl.Status = newValue;
}

From C# 7, we can use is to check and convert the type at the same time:

1
2
3
4
5
if (target is ControlA ctrl)
{
    //You can use ctrl variable directly now because the result is already assigned to it
    ctrl.Status = newValue;
}

FYI: Type-testing operators and cast expression

Check the instance which comes from the DI container for deep-sleep mode on mobile

This issue may happen on mobile devices. If we use Dependency Injection for the mobile app, usually we just get the service instance from the DI container. But on mobile devices there are too many tricky scenarios we need to deal with, eg. deep-sleep mode, background tasks and push notifications, etc. For example, if the user clicks the notification to execute some actions, the DI container might have not been initialized. In other words, the lifecycle of the app may not be the way we expect. I have met some issues related to it but I do not want to expand too much here because it is rarely happening for other applications.

Any better ways to replace if statement for null checking?

Yes. You can use the null-conditional operator to simplify it. Just use ? to short-circuit the expression. For example:

1
A?.B?.Do(C)

If A is null, the rest of the chain does not execute. If B is null, Do(C) does not execute.

Similarly, you can use ?[] for element access. eg. A?.B?[C]. It is safe even B is null.

It is also a good choice to invoke a delegate/Action/Func. For example:

1
2
3
4
5
6
7
8
9
class Counter
{
    public event EventHandler ThresholdReached;
    protected virtual void OnThresholdReached(EventArgs e)
    {
        // Use ? to avoid null reference exception
        ThresholdReached?.Invoke(this, e);
    }
}

By the way, you can use the null-coalescing operator ?? to return the value of its left-hand operand if it is not null; otherwise, it returns the value of right-hand operand. For example, if we need to assign a value to a variable:

1
2
3
4
if (variable is null)
{
    variable = expression;
}

The below code is available in C# 8.0:

1
variable ??= expression;

FYI: Member access operators and expressions

null-coalescing-operator

Using TryParse(), not Parse()

I think most of .NET developers already know that.

Task/Async/Await

There are a vast number of posts/questions/blogs you can find in StackOverflow, GitHub or someone’s blogs regarding Task, async and await. I do read a lot articles, but I will not add any new contents and I just would like to highlight some best practices in asynchronous programming.

Avoid async void method unless you use it for an event handler

Return Task or Task<T> if it is possible for the async method. If you return a void method, the complier will not complain but it is not good because any exceptions will be eaten so you cannot catch it properly.

One special scenario is to make an async event handler. Usually the event handler does not return any type, and it just returns void. But please make sure you already handle the possible exceptions in the event handler. For all the other cases, do not use async void method.

Avoid mixing async and sync methods together

We should keep in mind that async all the way, which means we should not mix async and sync code for most of scenarios. More particular, never use Task.Wait() and Task.Result to block the async code. Task.Wait() is synchronous, potentially blocking the call. If I see Task.Wait() in code-review, definitely it is a strong signal to indicate the code may cause deadlocks. Also the bug caused by the deadlock is hard to track so please do not use it anymore.

Return Task, not return await

If you already have an async Task and you need to call it in another Task, just return the Task, instead of making it as async then await. For example:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
public async Task<string> GetDataFromApiAsync()
{
    //Do some async jobs
}

// You can do it like this 
public async Task<string> GetDataAsync()
{
    return await GetDataFromApiAsync();
}

// But this one is better
public Task<string> GetData()
{
    // Just return the task
    return GetDataFromApiAsync();
}

Every time we declare a method as async, the compiler will create a state machine class to wrap the logic. So if you do not have other await in the method, please just return the task directly. It helps us not create additional resource to switch between different threads.

What if we have to block the async code?

For some reason we must convert an async code to synchronous, then you better use Task.GetAwaiter().GetResult(). It is the same with Task.Wait() if the task executes successfully, but the benefit is it can return a normal exception if the task gets any errors. But Task.Wait() wraps the exceptions in an AggregateException so it is extremely hard to diagnose the issue.

Use Task.WaitAll() to run multiple tasks

If you have multiple tasks to be executed and they do not have dependencies with each other, you can use await Task.WhenAll() to run them in parallel. Similarly, you can use await Task.WhenAny() to wait for any task to complete then continue the next code.

Use ConfigureAwait(false) to avoid deadlock

The SynchronizationContext class Provides the basic functionality for propagating a synchronization context in various synchronization models. Basically, using ConfigureAwait(false) can improve performance and avoid deadlocks. The general guidance is:

  • Do not use ConfigureAwait(false) in the application-level code.
  • Use ConfigureAwait(false) in the general-purpose library code.

In other words, if you are developing a Windows Form/WPF/ASP.NET Core application, usually you do not need to use it. But if you develop a library for the general purposes, it would be better if you use it because the library does not care about the environment so no need to keep on the same SynchronizationContext.

I highly recommend the below articles about Task/async/await:

Exceptions

All the applications may have exceptions. But a well-designed app can handle the exception and errors to prevent crashing. Some common issues are:

Throw exceptions, not return an error code.

Exceptions are not scary. I have seen some codes that try to hide the exception then return error codes. That would not good because the caller may not check the return code. Exceptions ensure that failures can be handled properly.

Use standard exception types when you can

.NET framework already provides lots of exception types, eg. ArgumentException, ArgumentNullException, InvalidOperationException etc. Only introduce a new exception type when a predefined one does not apply.

Provide additional properties for custom exceptions

If you design a custom exception, please attach additional information to the exception, which is useful for diagnosis. For example, ArgumentNullException has the argument name property, and FileNotFoundException has a FileName property. The more information it has, the easier for debugging.

Do not eat the exception in try-catch block

I have seen the below code:

1
2
3
4
5
6
7
8
9
try
{
    // Do something
}
catch (Exception ex)
{
    // Log the exception
    throw ex;
}

This is not good because it will reset the stack trace. The correct way is:

1
2
3
4
5
6
7
8
9
try
{
    // Do something
}
catch (Exception ex)
{
    // Log the exception
    throw;
}

Just use throw to keep the original stack trace. So the caller would know what happens. Also, you may see the below code:

1
2
3
4
5
6
catch (Exception ex)
{
    throw new MyException("error occurs", ex); // This will wrap the original stack trace
    //or
    throw new MyException("error occurs"); // This will replace the original stack trace
}

So please make sure you throw exceptions correctly. DO NOT eat them!

I recommend you to read the below articles:

HttpClient

To be honest, the HttpClient class in .NET is not good enough because so many developers are looking for how to use it correctly every day. So here are some tips:

DO NOT use using to dispose the HttpClient instance explicitly

I think lots of .NET developers already know that. It is a common error for some junior developers because HttpClient implements the IDisposable interface. Also, you might find the below code in some code samples from Microsoft official documents:

1
2
3
4
using (var httpClient = new HttpClient())
{
    // Do something
}

It is understandable because it is an IDisposable object and we should use using statement. But for this case, the above usage is totally wrong. Instead of creating a new instance for each request, we should share a single instance for the entire application. Because even you dispose the HttpClient instance, the underlying socket connection cannot be disposed immediately (generally, it will be hold for 240 seconds on Windows) so if you create new instances again and again, the available socket ports will be exhausted soon.

The correct way is to keep a singleton instance for HttpClient. You can use a static instance or use singleton pattern to maintain the HttpClient instance. But we still have another issue.

HttpClient does not respect DNS changes

For most of scenarios, it is enough to maintain a singleton instance for all the requests. In .NET Framework 4.x , HttpClient does not respect the DNS changes. In other words, if the endpoint changes the DNS record(It could happen when we do the blue/green deployment), the current HttpClient instance cannot detect that change so it will be stuck until the socket connection is closed. In the latest .NET Core, it has been improved.

FYI: Singleton HttpClient? Beware of this serious behaviour and how to fix it

Singleton HttpClient doesn’t respect DNS changes

Specify HttpHeaders for HttpClient or HttpRequestMessage?

For some reason, we need to specify the HttpHeaders. We can do it through two ways:

  • Use HttpClient.DefaultRequestHeaders.Add() to update the header of the HttpClient instance.
  • Use HttpRequestMessage.Headers.Add() to update the header for each request.

The second way is better. Because usually we reuse the singleton HttpClient instance. Even HttpClient is thread-safe, if we frequently change the DefaultRequestHeaders collection, it may cause the below InvalidOperationException:

1
Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.

The reason is the DefaultRequestHeaders property of HttpClient is not thread-safe. It should not be modified while there are outstanding requests. FYI: HttpClient.DefaultRequestHeaders

Retry the request

The HTTP request may fail so usually we need to retry the failed requests by using Polly. So here is another exception you may see: 

1
InvalidOperation exception: The request message was already sent. Cannot send the same request message multiple times.

That is because Polly just retries the request but does not change it. However, if the HttpClient checks it sends the same request, it will throw the above exception. The solution is easy - just create a clone() method to create another copy of the request:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
public static HttpRequestMessage Clone(this HttpRequestMessage req)
{
    HttpRequestMessage clone = new HttpRequestMessage(req.Method, req.RequestUri);

    clone.Content = req.Content;
    clone.Version = req.Version;

    foreach (KeyValuePair<string, object> prop in req.Properties)
    {
        clone.Properties.Add(prop);
    }

    foreach (KeyValuePair<string, IEnumerable<string>> header in req.Headers)
    {
        clone.Headers.TryAddWithoutValidation(header.Key, header.Value);
    }

    return clone;
}

FYI: How Polly prevents InvalidOperation exception SendAsync

In .NET Core 2.1, Microsoft provides HttpClientFactory to try to solve some issues. It is better but one thing I do not like is it is tightly coupled with Microsoft DI. Here is an interesting thread on GitHub: Using HttpClientFactory without dependency injection.

Collections/List

There are too many topics regarding Collections/List in .NET. But I just want to emphasize the below tips here:

Always initialize the collection when you declare it

Oh I just realize that I have mentioned it at the beginning of this article. So please keep it in mind because I have struggled with this issue many times when I do the code-review.

ToList() first or just use foreach for the IEnumerable?

I am sure you have seen the below code:

1
2
3
4
5
// GetList() will return an IEnumerable object
foreach (var item in GetList())
{
    await HandleItemAsync(item);
}

We exactly know that when we use foreach to an IEnumerable object, it will iterate through the collection. For most scenario, it is the same with this one:

1
2
3
4
foreach (var item in GetList().ToList())
{
    await HandleItemAsync(item);
}

The difference is that when you use IEnumerable, the compiler gets a chance to defer the work until later, possible optimizing the way. If you use ToList(), the compiler will force to get the result immediately. An example is when we query data from database, we usually have LINQ conditions so it would be better to defer evaluation until we execute it. But in this case, we should be aware of the async method in the foreach body. Consider this scenario: Let us say we have an Azure blob. We need to get all the files in the blob storage then delete them. So the code might be like this:

1
2
3
4
5
var files = container.ListBlobs(null, true, BlobListingDetails.None);// List all the files in the blob. It will return an IEnumerable object.
foreach (var file in files)
{
    await file.DeleteIfExistsAsync();
}

It seems good. But the risk is this is a cloud application. ListBlobs() method might be timeout if it cannot complete in a specify duration. So if there are a bunch of files to be deleted, it would spend quite a few minutes for ListBlobs() method. Then you will get a timeout exception.

So the safe way is to use ToList() before making the iteration:

1
2
3
4
5
var files = container.ListBlobs(null, true, BlobListingDetails.None).ToList();// List all the files in the blob then convert to a List.
foreach (var file in files)
{
    await file.DeleteIfExistsAsync();
}

Now you query the file list right away. Then no need to worry about the timeout issue. So it is not easy to say which method is better. It depends.

Possible multiple enumeration of IEnumerable

You may see this warning if you use Resharper. The below code comes from Jetbrains:

1
2
3
4
5
6
IEnumerable<string> names = GetNames();
foreach (var name in names)
    Console.WriteLine("Found " + name);
var allNames = new StringBuilder();
foreach (var name in names)
    allNames.Append(name + " ");

Assuming that GetNames() returns an IEnumerable<string>, the code makes twice enumerating in two foreach statements. If GetNames() queries the database or calls an API, the two calls may get different results because the original data has been changed by other processes. So it can be fixed by converting the IEnumerable<string> to List:

1
List<string> names = GetNames().ToList();

So the two enumeration will get the same result.

Concurrent collections

The List and Dictionary are not thread-safe. Consider using concurrent collections for multiple-thread scenarios. You can find more information here: System.Collections.Concurrent Namespace

Miscellaneous

== or Equals?

They are both used to compare two value type data items or reference type data items. The difference is that == returns true if they refer to the same object. Equals() returns true if they have the same content. So they are not the same. FYI:

DO use protected constructor for abstract classes

Just do it.

Event pattern

I have met a bug regarding the event pattern. Basically the class maintains a global status field. If the app detects status changes, the class will publish an event to notify the subscriber. But it published the event first, then updated the global status field. So when the caller received the event, the global status might not be changed yet. It caused an intermittent bug. So if the caller needs to use some global values, please make sure the publisher updates the status first, then publishes the event.

Delete useless codes

If some codes are useless, you can add Obsolete attribute for the current version. Then totally delete them in the future versions. No need to keep the useless code in your solution. It is ugly. I have seen lots of codes that are commented but I don’t know why they are still there. Will they be restored in the future? No one knows. So if you do not need that, please delete it. Or you can leave a comment that indicates why it should be kept temporarily.

Do not use too many parameters for a method.

If there are too many parameters for a method, consider refactoring it as a class.

Summary

This is a cluttered note but indeed I met these issues many times. When I have more ideas I can add more topics to this series. Thanks.