Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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");
    }
}
share|improve this question
    
@Steven: can you please look in to my coding technique. –  tango Nov 16 '14 at 2:57
1  
@Jamal: thanks, but i have not asked for reviewing grammatical mistake but code review :) –  tango Nov 16 '14 at 4:49
    
Have you considered how you are going to test them? –  craftworkgames Nov 17 '14 at 20:11

3 Answers 3

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.

share|improve this answer

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

share|improve this answer

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
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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