5
\$\begingroup\$

Please have a through look my custom implementation of generic repository pattern and suggest anything I may be lacking.

  1. I am using a generic interface with 6 methods
  2. I'll be using a repository class for each entity type (e.g UserRepository, ProductRepository, StoreRepository) which are not only going to implement the generic interface but also can add additional methods as per the need with respect to each repository class. for example:
    • UserRepository can add a new method named as CountUsers() which gets called from controller.
    • ProductRepository can add a new method named as GetExpiredProducts() which gets called from controller.
  3. I am declaring IGenericRepository<User> in UserController, similarly in other controllers and using repositoryPattern.

Generic Repository Interface

public interface IGenericRepository<T> : IDisposable where T : class
{
    IEnumerable<T> SelectAll();
    T SelectByID(object id);
    void Insert(T obj);
    void Update(T obj);
    void Delete(object id);
    void Save();
}

Implementation of Generic Repository Interface

public class UserRepository : IGenericRepository<User>
{
    private ModelDBEntities dataContext = null;
    public UserRepository()
    {
        this.dataContext = new ModelDBEntities();
    }
    public UserRepository(ModelDBEntities dataContext)
    {
        this.dataContext = dataContext;
    }

    public IEnumerable<User> SelectAll()
    {
        return dataContext.Users;
    }
    public User SelectByID(object argId)
    {
        long userId = long.Parse(argId.ToString());
        return dataContext.Users.Single(u => u.UserId == userId);
    }
    public void Insert(User obj)
    {
       dataContext.Users.AddObject(obj);
    }
    public void Update(User obj)
    {
        if (obj.EntityState == EntityState.Detached)
            dataContext.Users.Attach(obj);
        dataContext.ObjectStateManager.ChangeObjectState(obj, System.Data.EntityState.Modified);            
    }
    public void Delete(object argUserId)
    {
        long userId = long.Parse(argUserId.ToString());
        dataContext.Users.Single(u => u.UserId == userId).Status = "inactive";
    }
    public void Save()
    {
        dataContext.SaveChanges();
    }

    private bool disposed = false;
    protected virtual void Dispose(bool disposing)
    {
        if (!this.disposed)
        {
            if (disposing)
            {
                dataContext.Dispose();
            }
        }
        this.disposed = true;
    }
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
}

My User Controller

public class UserController : Controller
{
    private IGenericRepository<User> _repositoryUser = null;     
    public UserController()
    {
        _repositoryUser = new UserRepository();
    }
    public UserController(IGenericRepository<User> repository)
    {
        _repositoryUser = repository;
    }

    [HttpPost]
    public JsonResult SubmitHandler(User argUser)
    {
        if (ModelState.IsValid)
        {
            _repositoryUser.Insert(argUser);
            _repositoryUser.Save();
            return Json("success");
        }
        else
            return Json("failure");
    }
    public ActionResult Delete(object Id)
    {
            if (Id != null && Id.ToString().Trim().Length > 0)
            {
                int param = int.Parse(Id.ToString());
                _repositoryUser.Delete(Id as object);
                _repositoryUser.Save();
                return View("User", _repositoryUser.SelectAll());
            }
            else
                return View("404");
    }
}
\$\endgroup\$
  • \$\begingroup\$ @Steven: can you please look in to my coding technique. \$\endgroup\$ – tango Nov 16 '14 at 2:57
  • 1
    \$\begingroup\$ @Jamal: thanks, but i have not asked for reviewing grammatical mistake but code review :) \$\endgroup\$ – tango Nov 16 '14 at 4:49
  • \$\begingroup\$ Have you considered how you are going to test them? \$\endgroup\$ – craftworkgames Nov 17 '14 at 20:11
3
\$\begingroup\$

I have created generic repositories many times and what I found was that you're going to need to create interfaces for each entity typed repository.

So IUserRepository that implements IGenericRepository<User>, this way you'll be able to

  • Inject the repository easier with Inversion Of Control
  • Create extra methods for each entity type as and when
  • Test your controller easier

Update

I realise this update comes a long time after the question, but time has past and experience has been gained.

You don't necessarily need a new interface for entity type. IoC containers can inject a specific implementation based on the generic parameter of the IGenericRepositoy interface. You will need an interface however for each implementation that has specific methods for that type, such as GetExpiredProducts() as mentioned in the initial question.

You could also remove the generic parameter from the interface and move the where clause to each method:

public interface IGenericRepository : IDisposable
{
    IEnumerable<T> SelectAll() where T : class;
    T SelectByID(object id) where T : class;
    void Insert(T obj) where T : class;
    void Update(T obj) where T : class;
    void Delete(object id);
    void Save();
}

then implement a GenericRepository class:

public class GenericRepository : IGenericRepository
{
    public GenericRepository()
    {
        blah...  
    }
    public GenericRepository(ModelDBEntities dataContext)
    {
        blah...  
    }

    public IEnumerable<T> SelectAll() where T : class
    {
        blah...  
    }
    public T SelectByID(object argId) where T : class
    {
        blah...  
    }
    public void Insert(T obj) where T : class
    {
       blah...  
    }
    public void Update(T obj) where T : class
    {
        blah...            
    }
    public void Delete(object argId)
    {
        blah...  
    }
    public void Save()
    {
        blah...  
    }
}

this would mean that any implementation of a type specific repository could just inherit this saving you some work

public interface IUserRepository : IGenericRepository
{
    int CountUsers();
}

public class UserRepository : GenericRepository, IUserRepository
{
    public UserRepository()
    {
        blah...  
    }
    public UserRepository(ModelDBEntities dataContext)
        : base(dataContext)
    {
        blah...  
    }

    public int CountUsers()
    {
        blah...
    }
}

You can then decide on what you want to inject into your controller, if you just want the base CRUD methods then you will only need to pass in the implementation of IGenericRepository and use for several types.

\$\endgroup\$
2
\$\begingroup\$

Var

Prefer the var keyword when declaring local variables if the right hand side of the declaration makes the variable's type obvious.

int param = int.Parse(Id.ToString());

Should be:

var param = int.Parse(Id.ToString());

Doing this makes it easier to change variable type later, and makes it more concise and easy to read for others.

Braces

Prefer to use curly braces for if, else, etc statements, even if they are a single line. It lets the code breathe a bit and improves readability. Additionally, add a separate such clauses from the rest of the code body with an empty line.

if (obj.EntityState == EntityState.Detached)
    dataContext.Users.Attach(obj);
dataContext.ObjectStateManager.ChangeObjectState(obj, System.Data.EntityState.Modified);

Becomes:

if (obj.EntityState == EntityState.Detached)
{
    dataContext.Users.Attach(obj);
}

dataContext.ObjectStateManager.ChangeObjectState(obj, System.Data.EntityState.Modified);

Validation

You should validate the parameters of your public methods using guard clauses.

For example:

public void Update(User obj)
{
    if (obj.EntityState == EntityState.Detached)
        dataContext.Users.Attach(obj);
    dataContext.ObjectStateManager.ChangeObjectState(obj, System.Data.EntityState.Modified);            
}

Does not test for a null user before using it. The result would be a NullReferenceException when it would be more useful and descriptive to throw an ArgumentNullException

\$\endgroup\$
1
\$\begingroup\$

I presume that each of your entities are completely independent from each other, that none of them refer to each other, nor do you perform compound queries. Also, the entities are updated or created at completely different times. Also, you only ever update or query one entity instance at a time.

Since those are unreasonable restrictions on entity design, you have to rethink what you are doing. If you have Stores that hold Products, then either the Store repository has to know about Products or vice versa.

Similarly, proper repository design should support the use of statement of work pattern. Transaction boundaries, validation and similar design issues also come into play.

\$\endgroup\$
  • \$\begingroup\$ The independence of the entities depends on how you're implementing each method inside your repository and the over all design of your system. I've used this approach in a CRUD system with an ORM behind it, passing in an entity with a child entity works fine as the ORM has always taken care of it. I disagree with the notion that I'd have to rethink this as I've used it in the manner you are suggesting regarding Stores and Products. I've also used this with a Unit of work so that everything is wrapped up inside one transaction. \$\endgroup\$ – matt_lethargic Mar 14 '18 at 13:48
  • \$\begingroup\$ As for Transaction boundaries and validation they are still taken care of by the entities themselves and are only a concern of the repo when it comes to making sure you do not save an invalid entity. Again all of this comes into how you not only implement the methods inside the repo, but also the design of your system as a whole. If you're using DDD rather than basic CRUD then most people will recommend that you don't use a generic repo anyway \$\endgroup\$ – matt_lethargic Mar 14 '18 at 13:55

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.