ASP.NET MVC guidelines for static classes for database access

Question

The way I am utilising the MVC pattern at the moment in my ASP.NET application (using Entity Framework) is as follows:

1) My Models folder contains all EF entities, as well as my ViewModels

2) I have a Helpers folders where I store classes created for the purposes of the particular application.

3) In my Helpers folder, I have a static class named MyHelper which contains methods that access the DB using EF.

namespace myApp.Helpers
{
    public static class MyHelper
    {
        public static async Task<ProductVM> GetProductAsync(int productId)
        {
            using (var context = new myEntities())
            {
                return await context.vwxProducts.Where(x => x.ProductId == productId).Select(x => new ProductVM { A = x.A, B = x.B }).FirstOrDefaultAsync();
            }
        }
    }
}

4) My controllers then call these functions where necessary:

namespace myApp.Controllers
{
    public class ProductController : Controller
    {

        [HttpGet]
        public async Task<ActionResult> Index(int productId)
        {
            var productVM = await MyHelper.GetProductAsync(productId);
            return View(productVM);
        }
    }
}

I usually encounter comments in SO of the type "don't use a static class, static classes are evil, etc". Would this apply in such a scenario? If yes, why? Is there a better 'structure' my app should follow for best practices and for avoiding such pitfalls?

Answer 1

I use a static class that has the context injected into a static constructor for the purposes of loading a cache of data that rarely changes. And it (should) be thread safe. I hope this helps you, it's very handy in my experience:

 public static class StaticCache<T>  where T: class 
 {
    private static List<T> dbSet;
    public static Dictionary<string, List<T>> cache = new Dictionary<string, List<T>>();
    private static readonly object Lock = new object();
    public static void Load(DbContext db, string connStr, string tableName)
    {
        lock (Lock)
        {
            try
            {
                if (connStr != null)
                {
                    using (db)
                    {
                        dbSet = db.Set<T>().ToList();                            
                        cache.Add(tableName, dbSet);
                    }
                }

            }
            catch { }
        }
    }
 }
 void Testit() 
 {
    var context = new YourContextSubClass(connStr);       
    StaticCache<TableEntity>.Load(context, connstr, "tableEntityNameString");
 }

Answer 2

Your idea is correct and I use it always. But the style is like this: 1) For each entity (i.e User) we have a static class inside Providers folder. In this class we can do general methods (i.e create, Get, GetAll , ..)

    public static class Users
{
    public static IEnumerable<kernel_Users> GetAll()
    {
        Kernel_Context db = new Kernel_Context();
        return db.kernel_Users;
    }

public static kernel_Users Get(int userId)
    {
        Kernel_Context db = new Kernel_Context();
        return db.kernel_Users.Where(c => c.UserId == userId).FirstOrDefault();
    }
    ...
}

2) We have another class that is not static.It is inside Models folder. This is the place that we can access to an instance of the entity :

    public partial class kernel_Users
{
    [Key]
    public int UserId { get; set; }
    public string Username { get; set; }
    public string Password { get; set; }

    [NotMapped]
    public string FullName
    {
        get
        {
            return FirstName + " " + LastName;
        }
    }
    public bool Delete(out string msg)
    {
        ...
    }
    ...

}

Answer 3

I was thinking to add comment to ChrisPratt's answer, but it ended being too long, so let me add separate answer.

Basically, this is not a life/death choice. Sure, static methods are not as flexible as classes for db access. But they are not bad per-se. One DbContext per request is a something to aim for. It is not an absolute must. It is kinda like dependency injection - you get more flexibility and in turn increase code complexity.

Look at these three questions and their answers, by taking into account everything they say, I'm sure you'll be able to answer your question yourself:

• Why would I use static methods for database access
• When to use static classes in C#
• One DbContext per web request... why?

EDIT: Chris left good comment on my answer and I've changed answer a bit to take into account what he said.

Answer 4

You can't really use a static class for this. Your Entity Framework context should have one and only one instance per request. Your methods here instantiate a new context for each method, which is going to cause a ton of problems with Entity Framework.

The general concept is fine, but your MyHelper class should be a normal class. Add a constructor that takes an instance of your context, and then use a DI container to inject the context into the helper class and the helper class into your controller.

UPDATE

Helper

namespace myApp.Helpers
{
    public class MyHelper
    {
        private readonly DbContext context;

        public MyHelper(DbContext context)
        {
            this.context = context;
        }

        public async Task<ProductVM> GetProductAsync(int productId)
        {
            return await context.vwxProducts.Where(x => x.ProductId == productId).Select(x => new ProductVM { A = x.A, B = x.B }).FirstOrDefaultAsync();
        }
    }
}

Controller

namespace myApp.Controllers
{
    public class ProductController : Controller
    {
        private readonly MyHelper myHelper;

        public ProductController(MyHelper myHelper)
        {
            this.myHelper = myHelper;
        }

        [HttpGet]
        public async Task<ActionResult> Index(int productId)
        {
            var productVM = await myHelper.GetProductAsync(productId);
            return View(productVM);
        }
    }
}

Then, you just need to set up a DI container to inject everything. The code for that is entirely dependent on which container you end up going with, so I can't really help you further. It's usually pretty straight-forward, though. Just read the docs for the container. You'll want to set the life-time scope of your objects to the request. Again, it's different for different containers, but they'll all have some sort of request-scope.