Tag Archives: code review

Code review #4 – in-memory caching

This is a post on a series about great code review feedback, that I either gave or received. You can go ahead and read the previous ones here: https://www.michalbialecki.com/2019/06/21/code-reviews/

The context

Caching is an inseparable part of ASP.net applications. It is the mechanism that makes our web pages loading blazing fast with a very little code required. However, this blessing comes with responsibility. I need to quote one of my favorite characters here:

Downsides come into play when you’re no longer sure if the data that you see are old, or new. Caches in different parts of your ecosystem can make your app inconsistent, incoherent. But let’s not get into details since it’s not the topic of this post.

Let’s say we have an API, that gets user by id and code looks like this:

[HttpGet("{id}")]
public async Task<JsonResult> Get(int id)
{
    var user = await _usersRepository.GetUserById(id);
    return Json(user);
}

Adding in-memory caching in .net core is super simple. You just need to add one line in the Startup.cs

And then pass IMemoryCache interface as a dependency. Code with in-memory caching would look like this:

[HttpGet("{id}")]
public async Task<JsonResult> Get(int id)
{
    var cacheKey = $"User_{id}";
    if(!_memoryCache.TryGetValue(cacheKey, out UserDto user))
    {
        user = await _usersRepository.GetUserById(id);
        _memoryCache.Set(cacheKey, user, TimeSpan.FromMinutes(5));
    }

    return Json(user);
}

Review feedback

Why don’t you use IDistributedCache? It has in-memory caching support.

Explanation

Distributed cache is a different type of caching, where data are stored in an external service or storage. When your application scales and have more than one instance, you need to have your cache consistent. Thus, you need to have one place to cache your data for all of your app instances. .Net net code supports distributed caching natively, by IDistributedCache interface.

All you need to do is to change caching registration in Startup.cs:

And make a few modifications in code using the cache. First of all, you need to inject IDistributedCache interface. Also remember that your entity, in this example UserDto, has to be annotated with Serializable attribute. Then, using this cache will look like this:

[HttpGet("{id}")]
public async Task<JsonResult> Get(int id)
{
    var cacheKey = $"User_{id}";
    UserDto user;
    var userBytes = await _distributedCache.GetAsync(cacheKey);
    if (userBytes == null)
    {
        user = await _usersRepository.GetUserById(id);
        userBytes = CacheHelper.Serialize(user);
        await _distributedCache.SetAsync(
            cacheKey,
            userBytes,
            new DistributedCacheEntryOptions { SlidingExpiration = TimeSpan.FromMinutes(5) });
    }

    user = CacheHelper.Deserialize<UserDto>(userBytes);
    return Json(user);
}

Using IDistributedCache is more complicated, cause it doesn’t support strongly types and you need to serialize and deserialize your objects. To not mess up my code, I created a CacheHelper class:

public static class CacheHelper
{
    public static T Deserialize<T>(byte[] param)
    {
        using (var ms = new MemoryStream(param))
        {
            IFormatter br = new BinaryFormatter();
            return (T)br.Deserialize(ms);
        }
    }

    public static byte[] Serialize(object obj)
    {
        if (obj == null)
        {
            return null;
        }

        var bf = new BinaryFormatter();
        using (var ms = new MemoryStream())
        {
            bf.Serialize(ms, obj);
            return ms.ToArray();
        }
    }
}

Why distributed cache?

Distributed cache has several advantages over other caching scenarios where cached data is stored on individual app servers:

  • Is consistent across requests to multiple servers
  • Survives server restarts and app deployments
  • Doesn’t use local memory

Microsoft’s implementation of .net core distributed cache supports not only memory cache, but also SQL Server, Redis and NCache distributed caching. It all differs by extension method you need to use in Startup.cs. This is really convenient to have caching in one place. Serialization and deserialization could be a downside, but it also makes it possible to make a one class, that would handle caching for the whole application. Having one cache class is always better then having multiple caches across an app.

When to use distributed memory cache?

  • In development and testing scenarios
  • When a single server is used in production and memory consumption isn’t an issue

If you would like to know more, I strongly recommend you to read more about:

  All code posted here you can find on my GitHub: https://github.com/mikuam/Blog

Code reviews

This is a series about great code reviews, that I either gave or received. Code reviews are crucial for code quality and I strongly recommend you to have it in your company. Two heads are always better than one, especially in an atmosphere of cooperation and mutual learning. I have pleasure and luck to work in such a team and I have to admit, that code review discussions are always appreciated.

Without further due, let’s get to it. Have a good read! 🙂

Have you ever got a great review feedback? If you do, feel free to write me! I bet it’s worth to share it with the community and readers of this blog.

Code review #2 – remember about your awaits

This is another post about great code review feedback, that I either gave or received. It will always consist of 3 parts: context, review feedback and explanation. You can go ahead and read the previous ones here: https://www.michalbialecki.com/2019/06/21/code-reviews/. This time I’m going to show you a very simple bug I found.

The context

The problem occurred when I was investigating a code with multiple available calls. And after some refactoring, it came down to something like this.

public async Task InsertUser()
{
    try
    {
        // async stuff here

        client.PostAsync("http://localhost:49532/api/users/InsertMany", null).ConfigureAwait(false);

        // more async cally 
    }
    catch (Exception e)
    {
        Console.WriteLine(e);
        throw;
    }
}

The thing was, that my call to InsertMany endpoint was not working somehow, and I didn’t catch any exceptions in this code. Do you know what’s wrong with it?

Review feedback

Oh, you haven’t awaited this one!

Explanation

That’s correct. There is a try-catch block that should catch every exception, but when async call is not awaited, then a Task is returned. So as a result no call would be made. This is potentially dangerous, because code will compile. IDE will give you a hint, but it can be easily overlooked.

As a good practice you can check if a call was a success:

public async Task InsertUser()
{
    try
    {
        // async stuff here

        var response = await client.PostAsync("http://localhost:49532/api/users/InsertMany", null).ConfigureAwait(false);
        response.EnsureSuccessStatusCode();

        // more async cally 
    }
    catch (Exception e)
    {
        Console.WriteLine(e);
        throw;
    }
}

So take a good care about you async / await calls 🙂

Code review #1 – dapper and varchar parameters

This is a first post about great code review feedback, that I either gave or received. It will always consist of 3 parts: context, review feedback and explanation. You can go ahead and read previous ones here: https://www.michalbialecki.com/2019/06/21/code-reviews/. So lets not wait anymore and get to it.

The context

This is a simple ASP.Net application, that is requesting database to get count of elements filtered by one parameter. In this case we need a number of users providing a country code, that is always two character string.

This is how DB schema looks like:

Code in .net app is written with Dapper nuget package, that extends functionalities of IDbCommand and offers entities mapping with considerably good performance. It looks like this:

public async Task<IEnumerable<UserDto>> GetCountByCountryCode(string countryCode)
{
    using (var connection = new SqlConnection(ConnectionString))
    {
        return await connection.QueryAsync<UserDto>(
            "SELECT count(*) FROM [Users] WHERE CountryCode = @CountryCode",
            new { CountryCode = countryCode }).ConfigureAwait(false);
    }
}

Looks pretty standard, right? What is wrong here then?

Review feedback

Please convert countryCode parameter to ANSI string in GetCountByCountryCode method, cause if you use it like that, it’s not optimal.

Explanation

Notice, that CountryCode in database schema is a varchar(2) and this means that it stores two 1-byte characters. On the contrary nvarchar type is 2-byte per character type, that can store multilingual data. When using .net String type we are using unicode strings by default and therefore if we pass countryCode string to SQL it will have to be converted to ANSI string first.

The correct code should look like this:

public async Task<IEnumerable<UserDto>> GetCountByCountryCodeAsAnsi(string countryCode)
{
    using (var connection = new SqlConnection(ConnectionString))
    {
        return await connection.QueryAsync<UserDto>(
            "SELECT count(*) FROM [Users] WHERE CountryCode = @CountryCode",
            new { CountryCode = new DbString() { Value = countryCode, IsAnsi = true, Length = 2 } })
            .ConfigureAwait(false);
    }
}

If we run SQL Server Profiler and check what requests are we doing, this is what we will get:

As you can see first query needs to convert CountryCode parameter from nvarchar(4000) to varchar(2) in order to compare it.

In order to check how that would impact the performance, I created a SQL table with 1000000(one milion) records and compared results.

Before review it took 242 miliseconds and after review it took only 55 miliseconds. So as you see it is more that 4 times performance improvement in this specific case.

  All code posted here you can find on my GitHub: https://github.com/mikuam/console-app-net-core