2
\$\begingroup\$

I have some legacy code which gets a list of Students and their corresponding School. It's LinqToSql and there is no lazy loading:

public class Student
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public School AttendedSchool { get; set; }

    public Student(int studentId) {
        // code to get the student from the database goes here, this
        // gets FirstName, LastName and SchoolId from the db.
        FirstName = dbResult.FirstName;
        LastName = dbResult.LastName;
        AttendedSchool = new School(dbResult.SchoolId);
    }
}   

public class School
{
    public string SchooolName { get; set; } 
    public School(int schoolId) {
        // go to database and call 'GetDbSchool' using schoolId
        SchoolName = resultFromDatabase.SchoolName;
    }
}

There are thousands of students but only a handful of schools. Because of the way the code is built we see a GetDbSchool call to the database every time a Student is created but this clearly isn't required, it's getting the same school details again and again. I want a simple cache of schools to prevent so many db calls. The cache doesn't need a mechanism to empty or update and if there is a race condition and the school is fetched from the database twice that's not a problem. This is the caching code I've written:

public static class SchoolCache
{
    public static School GetSchool(int schoolId)
    {
        if (MemoryCache.Default["school_" + schoolId] != null)
        {
            return (School)MemoryCache.Default["school_" + schoolId.ToString()];
        }

        var cachePolicy = new CacheItemPolicy { SlidingExpiration = TimeSpan.FromSeconds(100) };
        var schoolFromDb = // go to database to get the school details
        MemoryCache.Default.Add("school_" + schoolId, schoolFromDb, cachePolicy);
        return schoolFromDb;
    }
}

Then in the School class I can use the cache and we get one db call per school:

public School(int schoolId) {
    SchoolName = SchoolCache.GetSchool(schoolId).SchoolName;
}

It works as expected but I don't see many caches this simple, most examples and implementations are more complicated. Are there any obvious problems with this simple implementation?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to Code Review! Please have a look at our question guidelines and change your title to describe what the code is supposed to do, not your concerns about it. \$\endgroup\$
    – AlexV
    Feb 25, 2020 at 14:56
  • \$\begingroup\$ You seem to seek an advice rather then code review. If this is the case, you might want to post your question on Software Engineering SE instead. \$\endgroup\$
    – slepic
    Feb 25, 2020 at 17:04

2 Answers 2

2
\$\begingroup\$

Seems you know that getting them individually is a bad idea but you are stuck with some legacy code. For your new code the biggest issue you will have is this line.

if (MemoryCache.Default["school_" + schoolId] != null)
{
    return (School)MemoryCache.Default["school_" + schoolId.ToString()];
}

you should retrieve the value and store in a variable. Then check if that variable is null or not and return the variable if not null. The odds are low but there is a chance the value is there for the If statement but ejected from the cache before it runs the statement to return the value.

If you want it to be multi-threaded then you would also need to change some code. For now I assume you don't care about that part.

Update for multi threaded

While memory cache is thread safe your code has the issue of potentially allowing calls that happen at the same time to hit the database. In pseudocode you code does this

  1. Check if it's in the cache
  2. retrieve the value from cache
  3. do the database call
  4. save the value in the cache

I already said in a non-multi threaded environment how 1 & 2 can cause issue. In a multi threaded a thread 1 could be doing step 3 and thread 2 could check step 1 and it will return it's not in the cache. Then thread 2 skips to step 3, because it's not in the cache yet because thread 1 hasn't saved it there yet. In the end it will not throw, as memory cache is threadsafe but will create extra database calls that are then ignored.

If you are using .net core then you can just use GetOrCreate method. In .net framework we can create that method ourselves.

-- .net framework from here on out

The easiest way is to use Lazy to hold the call to the database and store the Lazy object in the cache. MemoryCache has a method for AddOrGetExisting_that will handle if it should insert the value or return it back. The downside is now the call is in the cache if the call threw an exception we cached that exception, typically not what is desired. We will need to check for that and evict the cache if it's an exception.

public static TItem GetOrCreate<TItem>(this MemoryCache cache, string key, Func<string, TItem> factory, CacheItemPolicy policy)
{
    // wrap the factory in a lazy so we don't execute the factory every time
    var lazy = new Lazy<TItem>(() => factory(key));
    // cache will return the value from cache or null if it inserted it
    var result = cache.AddOrGetExisting(key, lazy, policy) as Lazy<TItem> ?? lazy;
    // now the factory is wrapped in the cache. if it threw an Exception then all callers would get the Exception
    //  trap for that an evict the cache if its an exception
    try
    {
        return result.Value;
    }
    catch (Exception e)
    {
        // evict factory that threw exceptions
        cache.Remove(key);
        throw;
    }
}

You could also make this handle async as well with some minor tweaks.

\$\endgroup\$
2
  • \$\begingroup\$ I thought MemoryCache was thread safe, can you outline changes I'd need to make to make my cache safe for multi-threaded use? \$\endgroup\$
    – jonnarosey
    Feb 26, 2020 at 11:13
  • \$\begingroup\$ @jonaglon I updated my answer \$\endgroup\$ Feb 26, 2020 at 14:45
3
\$\begingroup\$

This is a really bad pattern: neither your Student nor your School class should be responsible for retrieving themselves from the DB. Imagine retrieving a list of 100 Students: that would mean 100+ queries to the DB. You can't even apply filters (e.g. get all the Students who go to one School).

Your caching mechanism is solving the wrong problem. What you should do is:

  • Get all Schools and store them in a Dictionary<int, School> where the key is the Id of the School.
  • Get all Students, and then loop through them and attach the School based on the SchoolId.

Two queries instead of hundreds or thousands.

\$\endgroup\$
1
  • \$\begingroup\$ You are 100% correct, the business objects shouldn't be responsible for retrieving themselves from the DB. This is a small part of a large code base which I need to be careful making changes to. I've over-simplified my example, the students are retrieved in a single query. The problem is that a 'Populate' function in Student repeatedly ends up calling 'GetDbSchool' by using new School in the way described above. The code above reduces ~9000 db calls (the number of students) to ~30 (the number of schools). I want to know specifically if there are problems with the caching strategy. \$\endgroup\$
    – jonnarosey
    Feb 25, 2020 at 16:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.