6
\$\begingroup\$

Since I am new to MVC and the Entity Framework, I have been struggling to grasp the concept of creating useful Data and Service layers. I have come across a scenario where I believe my code has become very redundant:

Data Access Layer:

public static class DeploymentOrderDataLayer
    {

     public static usr_OrderFulfillment GetScannedItem(string orderNumber, string itemNumber)
        {
            using (TPGContext context = new TPGContext())
            {
                return context.usr_OrderFulfillment.FirstOrDefault(x => x.SOPNUMBE == orderNumber && x.ItemNumber == itemNumber);
            }
        }


    public static void UpdateScannedItem(string orderNumber, string itemNumber, string user, string serialNumber)
        {
            using (TPGContext context = new TPGContext())
            {
                var result = context.usr_OrderFulfillment.FirstOrDefault(x => x.SOPNUMBE == orderNumber && x.ItemNumber == itemNumber);

                result.IsFulfilled = true;
                result.FulfilledBy = user;
                result.DateFulfilled = DateTime.Now;

                if (!string.IsNullOrEmpty(serialNumber))
                {
                    result.SerialNumber = serialNumber;
                }

                context.SaveChanges();
            }
        }
}

I call all of my Data Layers through a BusinessLayer, which handles my business logic:

public class DeploymentOrderBusinessLayer
    {

    public static void UpdateScannedItem(string orderNumber, string itemNumber, string user, string serialNumber = "")
            {

                    var itemInfo = DeploymentOrderDataLayer.GetScannedItem(orderNumber, itemNumber);

                    if (itemInfo == null) throw new BusinessException("Item number not found.");

                    if (itemInfo.IsFulfilled) throw new BusinessException(string.Format("Item already fulfilled by {0}", itemInfo.FulfilledBy));


                DeploymentOrderDataLayer.UpdateScannedItem(orderNumber, itemNumber, user, serialNumber);
            }

   }

The BusinessLayer then gets called from my DeploymentOrder Controller:

 try
        {

          DeploymentOrderBusinessLayer.UpdateScannedItem(orderNumber, itemNumber, User.Identity.Name);

        }
        catch (BusinessException ex)
        {
            ViewBag.AlertMessage = ex.Message;
        }

A couple things that bug me:

  1. I'm calling GetScannedItem in my business layer so that I can do my validation first, before anything gets updated. When I'm ready to update, I have to re-query the database again, so that I can update my context. This extra hit to the database seems unnecessary to me, but I'm not sure how to fix it.

  2. The DeploymentOrderDataLayer is fine for my DeploymentOrderController, but useless for any other controllers that might need to access similar data. I would like my Data Layers to be more dynamic.

Any suggestions are appreciated.

\$\endgroup\$
5
\$\begingroup\$

This is going to be just a partial review, I don't have much time but I can't help it, I have to say something.

  • usr_OrderFulfillment might be how the table is called in your database, and SOPNUMBE might be how a column is called in that table. It doesn't mean your entities have to be as painful. Entity Framework is an object/relational mapper; this implies a mapping - you can easily map table usr_OrderFulfillment to entity OrderFulfillment and column SOPNUMBE to property OrderNumber.

  • Your DeploymentOrderDataLayer class (I wouldn't call a class a layer - call it a service if you will, but a layer is more for an assembly than for a type), is tightly coupled to TPGContext, and each new method requires a new instance of your context - even if 20 calls are made in the same request. You should inject the instance through the class' constructor and use a single instance per request. There's really no need to new it up and dispose it in every method. The class being static isn't helping either (it does explain the non-existing constructor though). Always prefer instances over static helpers.

  • This architecture feels like stored procedures written in C# code. It's bloated, over-engineered and makes you almost as good as me in shredding KISS into tiny little pieces. Have you tried using the EF DbContext directly in your controller to get the job done, and then refactor into a more structured approach?

\$\endgroup\$
  • \$\begingroup\$ How do I "inject" the instance through the class' constructor? I don't think I have done anything like that before. I agree the architecture is over engineered, but I don't want my DBContext code to be in my controller. Maybe I could put that code in the BusinessLayer along with the validation? \$\endgroup\$ – broke Jan 8 '14 at 21:49
  • \$\begingroup\$ I'll edit with more details when I have a few minutes; sorry I'm speaking DI lingo - constructor injection is when you pass a class' dependencies as constructor arguments - the db context in this case. It moves the newing up to the caller, taking the concern of creating the context out of that class. You can then assign a private readonly field and use that instead of recreating it in every method :) \$\endgroup\$ – Mathieu Guindon Jan 9 '14 at 0:05
1
\$\begingroup\$

I have done this several different ways in several MVC projects for my work. I don't think there is a specific "right way" to do this and it really depends on the project. The size and scope of your project has a real bearing on how this is done. If it is small (one or two controllers tops, maybe 3 tables in the database), then the way shown in most MVC tutorials is probably just fine. However, as your project gets larger, maintainability becomes an issue and one must consider the repetitiveness of the code and the "ripple effect" small-ish changes have.

Your code has the right idea, but I think your creation and disposal of entity framework context objects causes you problems. It is unnecessary IMHO and could slow down your application. In addition, you are losing that most beautiful ability of entity framework to track changes to entities and such. As for your separation of duties (having a separate object working with the entity context, having the controller use this object only to get and manipulate data, etc), I agree with how you have done it.

I have found the using the following constraints has helped the code to be more maintainable for me. These change often as my code is critiqued at work and as I have to do maintenance on code I have long since forgotten how it worked:

  • Entity Framework entities are allowed to be passed around between the model/data layer, but they are never passed into views. Entities don't serialize well and "feels wrong" to have a razor page be allowed to inadvertently perform database operations such as loading a child entity.
  • There is only one Entity Framework context per request. This helps keep connection count down and allows you to pass entities around in your code. I often isolate this context from my controller code so that it is never touched by a controller (more on how I do this later).
  • I have a separate Model project. This is mainly by personal preference since I often have several components to an application (the web front end, a service that runs in the background, a bootstrap console application, etc) and I often find it useful to share the same logic. However, I have occasionally got myself into DLL and dependency issues using this, so at times I skip this completely.

Lately, I have implemented the following:

  • A central object which handles my queries and "hides" the database context from everything else. If the database and such are in a separate project, I make the context "internal" rather than public so that the database context can only be instantiated from this central object. I believe this is called the "repository pattern", but I am not sure since one could say that the EF context is a repository on its own.
  • The central object is IDisposable and when instantiated creates a single EF context and when disposed disposes the EF context.
  • The central object returns entities and has a SaveChanges method that is just a wrapper to the context's SaveChanges.
  • I often have several sub-objects that are only created by the central object which organize the various methods. For example, if I have a users table, an items table, and a purchases table, I would create separate objects for each set of operations that happens on each table. The Users object would have methods for finding and creating users, the Items object would have methods for finding and creating items, and the Purchases object would have methods for finding purchases. For operations that cross the boundaries (such as creating a purchase for a user with several items), I either just do it via the entity relationships or, if it is more complicated, I place the operation into the sub-object for the table that is most affected (in this example, the purchases object would get this particular operation since a purchase was created).
  • In the Application_BeginRequest method, an instance of the central object is instantiated and attached to the HttpContext.Current.Items dictionary. In the Application_EndRequest method, the central object is disposed. No other central object is created during the request so that all entities used during the request are attached to the same context. This makes it easy to do things like have both an IPrincipal-style user (accessed via the controller's User property) and a database user which could be stored somewhere like HttpContext.Current.Items.
  • In the event that an entity needs to leave the application, I use the DTO pattern (Data Transfer Object). These are serializable classes that I usually decorate with DataContract and DataMember attributes. As their constructor arguments, they take in one or more entities. In terms of logic, they are super super simple and have nothing more than a bunch of properties. The point of these is to mitigate entity serialization issues and also give a measure of control over what exactly is transmitted over the wire. These are what are placed into the ViewBag and set as the Model object for views. They are also the objects I pass out of WebAPI endpoints.

I often create a static helper class which extracts and casts the various objects stored in HttpContext.Current.Items to their appropriate types. I also have lately been having these methods actually instantiate the objects rather than having Application_BeginRequest do it. Application_EndRequest is then modified to only dispose the objects if they are actually found in HttpContext.Current.Items.

A small example of this is here (no central object separation of duties, no DTOs, and no static helper class): https://gist.github.com/kcuzner/8306451

Pretend that in the example the DbQueries class is your DeploymentOrderDataLayer class. I made that gist for another question and I would not have named that object in real life the way I have it in the gist.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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