About the author

Artak Mkrtchyan is a software engineer from Redmond, WA with over 16 years of software development experience and has been a PullRequest reviewer since October 2019.

Follow Artak on Twitter @mkArtak

There are many things to look out for when reviewing C# code.

Anti-patterns and red flags are different depending on the context. For example, some concerns which are valid for client-side development are not applicable to server-side running code.

Because of this, we will cover these two situations separately and here we’ll focus on reviewing code that is run server-side.

Reviewing code which runs on the server

Most of the time these are ASP.NET Core, ASP.NET, WCF or ASP.NET Web API services. What is unique in these environments, is that the same application can serve thousands or more clients. As a result, issues which may seem to be tiny can result in dramatic and unexpected outcomes.

This article covers things to look for as red flags in server environments. Please note, that this list is not a comprehensive guide covering every possible situation, rather a list of issues which happen quite often in these environments.

Unnecessary allocations

While allocations are required in many situations, developers should try to minimize these as much as possible. Many people think about these as a memory concern, but that’s only part of it. Even the allocation of tiny objects is a concern. That is - these objects (we’re talking about reference types here) will fill up the heap and cause the Garbage Collector to run.

That’s where the main issue is, as the Garbage Collector run will impact the server performance dramatically.

So, if you happen to allocate too many objects too often, that will result in more GC runs and badly impact the server performance.

Here is an example showcasing a bad use case, which needs to be flagged:

  public class SomeClass
  {
    private readonly IService service;
    public IResult DoSomething (IDoSomethingArguments args)
    {
      var serviceParams = new char[] {'a', 'd', 's'};
      /// ...
      return new Result(serviceParams, args);
    }
  }

As you can see above, the the serviceParams variable is assigned to the same value on every call of the DoSomething API, no matter what arguments are passed in.

This results in a new array being created on each call. A good recommendation, in this case, would be to have a static variable which stores the array, and use that in the method for each call, as follows:

  public class SomeClass
  {
    private static readonly char[] serviceParams {'a', 'd', 's'};

    public IResult DoSomething (IDoSomethingArguments args)
    {
      /// ...
      return new Result(serviceParams, args);
    }
  }

Missing argument validations

Argument validation is key to a fail-fast strategy. This is important because in server environments the goal is to reduce the amount of work being done as much as possible to reduce resource consumption, be it memory, CPU, network or disk IO. Many situations, however, don’t fall under a red flag as these happen only once per application startup, and will block certain scenarios if not fixed. These issues are usually fixed once and won’t happen again.

The other, more important, side of this, is validation of user-provided arguments or subsequent API-call initiated functions.

Consider the following example:

  public class SomeApiController
  {
    private readonly IService service1;
    private readonly IService2 service2;

    public async Task SomeAPI(Param1 arg)
    {
      var input = await service1.GetSomeDataAsync(DateTimeOffset.Now);

      return await service2.Calculate(input, arg);
    } 
  }

Assuming service2 requires arg to be a non-null value, we know for sure that the second call will fail. However, the above code performed a resource-expensive operation, because SomeAPI didn’t check for the arg to be non-null.

A suggestion here will be to add a null check in the method, as follows:

  public async Task SomeAPI(Param1 arg)
  {
    if (arg == null)
      throw new ArgumentNullException(nameof(arg));
    ...

  }

Bad design

This is a major area for feedback. When reviewing code, it’s not always practical nor required to understand the high-level design. However, on the scale of methods or classes, it’s very reasonable to catch design flaws. One area I typically find issues is that of code cohesion.

Below is a list of things that should be raised as red flags:

• A method with multiple responsibilities.
• A method which checks whether it should be called or not. This happens quite often and almost always that check should be in the calling code, rather within the method itself.
• Classes that have disposable members should also be disposable.

  private class Handler
  {
    private IDisposableService service;

    public Handler(IDisposableService service)
    {
      if (service == null)
        throw new ArgumentNullException(nameof(service));

      this.service = service;
    }
  }

While the above code looks correct, the IDisposableService implements IDisposable and assuming the Handler instance is the sole owner of the passed-in instance of the IDisposableService, the code should be rewritten as follows:

  private class Handler : IDisposable
  {
    private IDisposableService service;

    public Handler(IDisposableService service)
    {
      if (service == null)
        throw new ArgumentNullException(nameof(service));

      this.service = service;
    }

    public void Dispose()
    {
      service.Dispose();
    }
  }

Multithreading

While task-based programming and the async/await keywords have simplified this a lot, there are still common mistakes developers make often that should be brought to attention.

ConfigureAwait(false)

In server-side running code, recommend users to call ConfigureAwait(false) when they are calling into an async method. The reason is that without this call the Task Scheduler will be trying to restore the original thread-context the call was made from, before returning. In UI applications (like WPF) this makes sense, as there is only a single UI thread, and all the UI interactions should happen on that thread, however in server applications this is not required and results in performance degradation.

Here is an example:

   private async Task SomeAPI()
   {
     var state = await service.GetRemoteState().ConfigureAwait(false);
   }

If you found this useful, we encourage you to check out Artak’s piece on Hidden Disposals featured in November 2016 issue of Microsoft’s MSDN Magazine.

Be sure to follow Artak on Twitter @mkArtak

#programming in real life: trying to find the optimal layout for the ceiling lights. pic.twitter.com/jzrb0ziUtO

— 🇦🇲 Artak (@mkArtak) October 20, 2019