Simple c# MemoryCache mechanism to reduce number of db calls 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?
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&lt;int, School&gt; 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.