3
\$\begingroup\$

I am designing a new asp.net MVC4 application. I know that context class generated by EF is Unit of work and Dbset is the repository class. I am just avoiding the extra abstraction. I want to know whether I am following the correct pattern or not.

Entity

  public partial class ProductMaster
  {
    public long ID { get; set; }
    public string ProductCode { get; set; }
    public string ProductName { get; set; }

  }
  public partial class CompanyMaster
  {
    public long ID{ get; set; }
    public string CompanyName { get; set; }
    public string CompanyCode { get; set; }
  }

Context class(Generated By EF)

public partial class SampleEntities : DbContext,IUnitOfWork
{
    public SampleEntities()
        : base("name=SampleEntities")
    {
    }
    public void Commit()
    {
       SaveChanges();
    }

    protected override void OnModelCreating(DbModelBuilder modelBuilder)
    {
        throw new UnintentionalCodeFirstException();
    }

    public DbSet<ProductMaster> ProductMaster { get; set; }
    public DbSet<CompanyMaster> CompanyMaster { get; set; }
    public DbSet<UserMaster> UserMaster{ get; set; }


    public virtual ObjectResult<SP_UserMaster_Result> SP_UserMaster(Nullable<long> tM_UM_ID, string userName, string userPassword, Nullable<byte> userType)
    {
        var tM_UM_IDParameter = tM_UM_ID.HasValue ?
            new ObjectParameter("TM_UM_ID", tM_UM_ID) :
            new ObjectParameter("TM_UM_ID", typeof(long));

        var userNameParameter = userName != null ?
            new ObjectParameter("UserName", userName) :
            new ObjectParameter("UserName", typeof(string));

        var userPasswordParameter = userPassword != null ?
            new ObjectParameter("UserPassword", userPassword) :
            new ObjectParameter("UserPassword", typeof(string));

        var userTypeParameter = userType.HasValue ?
            new ObjectParameter("UserType", userType) :
            new ObjectParameter("UserType", typeof(byte));



        return ((IObjectContextAdapter)this).ObjectContext.ExecuteFunction<SP_UserMaster_Result>("SP_UserMaster", tM_UM_IDParameter, userNameParameter, userPasswordParameter, userTypeParameter);
    }
}

Unit of Work Interface

public Interface IUnitOfWork
{
    public DbSet<ProductMaster> ProductMaster;
    public DbSet<CompanyMaster> CompanyMaster;
    public Dbset<UserMaster> UserMaster;
    public virtual ObjectResult SP_UserMaster(Nullable<long> tM_UM_ID, string userName, string userPassword, Nullable<byte> userType){};
    public void Commit();
}

Business Logic(Implementation)

 public class ProductMaster
 {
    IUnitOfWork _unitOfWork;
    public ProductMaster(IunitOfWork unitOfWork)//Injected using Ninject
    {
        this._unitOfWork = unitOfWork;
    }
    public void AddProduct(Product product)
    {
        _unitofWork.ProductMaster.Add(product);
        _unitOfWork.Commit();
    }
 }

Interface of Product Logic

 public class IProductMaster
  {
    public ProductMaster(IunitOfWork unitOfWork);
  }

Product Controller

  public class ProductController : Controller
  {
     IProductMaster _productMaster;
     public ProductController(IproductMaster productMaster)
     {
       this._ProductMaster=productMaster;//Injecting using Ninject
     }
     public void SaveProduct(ProductMaster product)
     {
        this._productMaster.AddProduct(product.ToEntity());
     }
  }
\$\endgroup\$
1
\$\begingroup\$

I think the problem is you’re starting with an implementation (Entity Framework) and then trying to bind this to two common design patterns – Unit Of Work and Repository. The Entity Framework handles these two patterns and combines them together in Microsoft’s own implementation – they couldn’t actually work independently, for example you couldn’t just take a DbSet (Repository) and get a Product (entity) without instantiating the context (Unit Of Work).

These design patterns exist so they can be easily replaced within your application without changing much code, but if your IUnitOfWork is returning a DbSet<T> repository and you decide to read your Product entities from a local file instead of a database then you’re not really going to be able to implement this with your current abstraction.

  1. IUnitOfWork should be strongly-named to the domain context – this is actually containing names of repositories which are specific to the domain context i.e. ProductMaster. You could have IShopUnitOfWork derive from IUnitOfWork (which only has Commit as this is reusable in different domain contexts)
  2. Your IUnitOfWork will get progressively bigger as you add new repositories and stored procedure definitions and I fear you’ll break the Interface Segregation Principle because of this.
  3. I believe your repositories should actually contain the “query” calls specific for an aggregate root entity rather than your Unit Of Work – by putting it in the Unit Of Work you’re giving it two responsibilities:

i. Maintaining the register of objects to be persisted

ii. Knowing how to authorise a user (in your example) along with all of the other stored procedure calls you’ll have

Therefore this will violate the Single Responsibility Principle

Furthermore SP_UserMaster should probably exist in the User repository as its specific to that domain entity. Again this is strongly-named to imply a database call with a stored procedure – what if this was to be changed to a web service call?

With these caveats in mind it’s going to be hard to change your implementation to work this way with Entity Framework as Microsoft has taken the design patterns and implemented it in their own way with Unit Of Work and Repository mingled together - I'm not saying this is wrong its just the way they've decided to do it. The main benefit of having these abstractions in this context is the benefit of being able to create stubs in unit testing so you’ll be mocking stored procedure and query calls without hitting the database.

\$\endgroup\$
0
\$\begingroup\$

Some quick style remarks.

Nullable<long>: convention now is long?


There is a lot of repeated code like this:

 var userNameParameter = userName != null ?
    new ObjectParameter("UserName", userName) :
    new ObjectParameter("UserName", typeof(string));

Why not do this:

 var userNameParameter = new ObjectParameter("UserName", 
    (userName == null) ? typeof(string) : userName);

Can't you rename tM_UM_ID, SP_UserMaster_Result, tM_UM_IDParameter? These are ugly names that violate C# naming guidelines.


I get why you're naming things userNameParameter, userPasswordParameter,... but I feel that's the wrong approach. Instead I'd propose a single class that contains all of the necessary parameters, e.g. like this:

internal class StoredProcParameter
{
   public string UserName {get;set;}

   // other properties

   public List<ObjectParameter> ToObjectParameters()
   {
      return new List<ObjectParameter>
      {
         new ObjectParameter("UserName", 
                   (UserName == null) ? typeof(string) : UserName)
      };
   }
}

Then you could use that class, i.e; fill it with the necessary data, call SP_UserMaster() and pass that class as the param, and use the ToObjectParameters() method when you call ExecuteFunction().


Your code also seems to be broken: in the last excerpt you have this._ProductMaster and there is no _ProductMaster. Moreover, since the _ prefix indicates a field there should be no need for the this..


Your comment style is inconsistent. I'd put the //Injected using Ninject on the line above the method, not next to it on the same line or below in the method's body. Also, is it even necessary?

\$\endgroup\$
  • \$\begingroup\$ Thank you for your suggestions.The variable names i wrote here is generated automatically by the entity framework(database first approach). The context class itself is auto generated. Actually i was asking about the design pattern i followed. Thank you for mentioning about the coding style problems \$\endgroup\$ – VIVEK P S Apr 18 '15 at 4:44

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.