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?

There is nothing wrong with a static class so long as it makes sense. Will this class ever need an instance? Will it ever need to extend a class or be extended by another? In general though, there's no sense in handcuffing yourself as requirements change. — Drew Kennedy
– Drew Kennedy, Commented Feb 12, 2016 at 16:25

Chris Pratt · Accepted Answer · 2016-02-12 17:18:20Z

9

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.

edited Feb 12, 2016 at 17:18

answered Feb 12, 2016 at 16:32

Chris Pratt

240k37 gold badges414 silver badges468 bronze badges

Sign up to request clarification or add additional context in comments.

8 Comments

globetrotter Over a year ago

Thanks, can you show a dummy example of this? This is what's confusing me at times, I remember seeing examples on the MSDN blogs which I based my approach on.

Chris Pratt Over a year ago

Also, FWIW, you don't need async and await on your individual helper methods, unless they're doing more than awaiting a single call. Just return Task<ProductVM> and await that in your controller. It's a small optimization, but it removes one layer of overhead.

globetrotter Over a year ago

Thanks @Chris, looks like I have some refactoring to do :)

globetrotter Over a year ago

Hi @Chris, apologies for the late comment, but I need a clarification on your code snippet: I've started implementing your approach using Autofac.MVC5. In your code snippet above, you inject a DbContext whereas in my original question I use a myEntities class (inherits DbContext, autogenerated by EF from my database). This means that in the helper class, context needs to be myEntities instead so I can then register it with Autofac using builder.RegisterType<myEntities>().AsSelf().InstancePerRequest();, correct?

Chris Pratt Over a year ago

You need to access the DbSet generically, i.e. context.Set<MyEntity>().Where(...);

|

Community · Accepted Answer · 2017-05-23 11:53:13Z

2

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:

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

edited May 23, 2017 at 11:53

CommunityBot

11 silver badge

answered Feb 12, 2016 at 17:37

nikib3ro

20.7k25 gold badges124 silver badges185 bronze badges

5 Comments

Chris Pratt Over a year ago

One context per request is an oversimplification for safety's sake. You truly only need one context per discrete set of actions, but since usually a request involves a set of discrete actions, it's just easier to make the lifetime request-scoped. However, saying this is merely a "recommendation" goes too far. Create a method scoped context like the OP was doing will lead to exceptions in various scenarios. If you try to relate two objects retrieved this way or even try to lazy-load a relationship, BOOM - your app explodes.

nikib3ro Over a year ago

Well, put that way - that you should aim for context per set of actions and not mix them up - I agree. What worried me is scenario in which one DbContext is used for different sets of actions - code in which you end up with multiple db.SaveChanges() or one SaveChanges has unexpected results because DbContext was tangled with some other objects. Anyway, I'll edit my answer to point to your comment, thanks for leaving it.

Chris Pratt Over a year ago

There's no problem there, either. Doing multiple transactions with the same context has no downside, and while it's certainly not technically wrong to new up a context for a different transaction, it serves no purpose other than to add another object to the heap and the CPU time to create it. Perhaps, it would be better stated that, while you don't strictly need only one context per request, it makes little sense to do it any other way.

globetrotter Over a year ago

Interesting discussion, thanks guys. The unfortunate thing about all of this is that MVC documentation/tutorials online, more often than not, have totally different implementations with no justification - for a novice like me, stuff become confusing quite fast. If you guys have any good MVC resources/books to suggest I'm all ears.

nikib3ro Over a year ago

Yeah, Microsoft resource sites like asp.net/get-started are done by different authors and when you have different people paid as contractors you end up with mixed results. So, there is no silver bullet. Simply read everything and decide for yourself. Once you find author that "talks to you" see if he wrote an book and go with that. My personal favorite MS author is Mike Taulty - mtaulty.com but I dunno if he wrote that much on ASP.NET

Ali Borjian · Accepted Answer · 2016-05-10 16:46:02Z

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)
    {
        ...
    }
    ...

}

DukeDidntNukeEm · Accepted Answer · 2016-09-15 21:19:31Z

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");
 }

Collectives™ on Stack Overflow

ASP.NET MVC guidelines for static classes for database access

4 Answers 4

8 Comments

5 Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

8 Comments

5 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related