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.

The code below is an approach of clean code separation, but I am confused if it is a right approach or wrong. Please suggest me.

public interface IRepository<T>
{
    [DataObjectMethod(DataObjectMethodType.Insert, true)]
    void Insert(T entity);
    [DataObjectMethod(DataObjectMethodType.Update, true)]
    void Update(T entity);
    [DataObjectMethod(DataObjectMethodType.Delete, true)]
    void Delete(T entity);
    IQueryable<T> SearchFor(Expression<Func<T, bool>> predicate);
    [DataObjectMethod(DataObjectMethodType.Select,true)]
    IQueryable<T> GetAll();
    T GetById(int id);
}

RepositoryBase

public abstract class RepositoryBase<T> : IRepository<T> where T: class
{
    protected DbSet<T> DbSet;

    public RepositoryBase(DbContext dbContext)
    {
        DbSet = dbContext.Set<T>();
    }

    #region IRepository<T> Members

    public void Insert(T entity)
    {
        DbSet.Add(entity);           
    }

    public void Update(T entity)
    {
        DbSet.Attach(entity);
    }

    public void Delete(T entity)
    {
        DbSet.Remove(entity);
    }

    public IQueryable<T> SearchFor(Expression<Func<T, bool>> predicate)
    {
        return DbSet.Where(predicate);
    }

    public IQueryable<T> GetAll()
    {
        return DbSet;
    }

    public T GetById(int id)
    {
        return DbSet.Find(id);
    }              

    #endregion
}

AplicationDbContext

public sealed class ApplicationDbContext : DbContext, IDisposable
{

    public DbSet<CountryModel> Country { get; set; }

    protected override void OnModelCreating(DbModelBuilder modelBuilder)
    {
        modelBuilder.Conventions.Remove<PluralizingTableNameConvention>();
    }

}

CountryRepository

public sealed class CountryRepository : RepositoryBase<CountryModel>
{
    CountryModel country;
    public CountryRepository(DbContext dbContext)
        : base(dbContext)
    {

    }

    public int Create()
    {
        using (EauthorityDbContext db = new EauthorityDbContext())
        {
            Insert(country);
            return db.SaveChanges();
        }
    }

    public int Delete()
    {
        using (EauthorityDbContext db = new EauthorityDbContext())
        {
            Delete(country);
            return db.SaveChanges();
        }
    }

}

Controller

public class CountryController : BaseController
{
    private readonly IRepository<CountryModel> repository;

    public CountryController(IRepository<CountryModel> repository)
    {
        this.repository = repository;
    }

    public ActionResult Index()
    {
        return View();
    }

    //
    // GET: /Country/Details/5
    public ActionResult Details(int id)
    {
        CountryModel country = this.repository.GetById(id);
        return View(country);
    }

    //
    // GET: /Country/Create
    public ActionResult Create()
    {
        return View();
    } 

    //
    // POST: /Country/Create

    [HttpPost]
    public ActionResult Create(CountryModel country)
    {
        try
        {
            if (ModelState.IsValid)
            {
                this.repository.Insert(country);
            }

            return RedirectToAction("Index");
        }
        catch
        {
            return View(country);
        }
    }
}

1. Is the code standard of CountryRepository & CountryController wrong ?

2. How to write a Test Method for CountryController

share|improve this question

closed as off-topic by Jamal Aug 1 at 15:19

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

2 Answers 2

up vote 0 down vote accepted

To provide some answers to your two questions.

  1. I don't think your implementation is wrong (in fact IMHO in general most things are not wrong just not necessarily the best method of implementation). However I think your CountryRepository as it stands is not correct as the variable country is never set. Perhaps this is still a work in progress?

Alternative

Rather than passing IRepository<CountryModel> to your controller I would consider creating a ICountryRepository interface that was something like

public interface ICountryRepository : IRepository<CountryModel>
{
   CountryModel Create();
   int Delete(CountryModel model);
}

Then I would pass that into your controller instead.

public CountryController(ICountryRepository repository)
{
    this.repository = repository;
}
  1. Controllers in MVC are not the easiest to unit test (although there are many articles out there for mocking the contexts required). However to me that is a good thing and it helps to ensure I keep my controllers as skinny (plenty of articles on skinny controllers out there...) as possible so that code within them does not require testing. Hence all my unit tests would go into the models, services that the controllers are using.

For example from your code I wouldn't unit test your controller actions themselves but would rather unit test model code within e.g.

repository.GetById() // do a unit test on GetById()
repository.Insert()  // do a unit test on Insert()

Alternatively as you have provided an interface into your controller already you could create a Mock database and using an DI framework such as Ninject you could do a form of testing without the real db anyway.

share|improve this answer

You should make the constructor in RepositoryBase protected (as public constructors in abstract classes are nonsensical), make DbSet private readonly (and pascalCase it) and then provide a protected get-only property DbSet to access it - principle of least privilege at work. Here's the snippet:

public abstract class RepositoryBase<T> : IRepository<T> where T: class
{
    private readonly DbSet<T> dbSet;

    protected RepositoryBase(DbContext dbContext)
    {
        dbSet = dbContext.Set<T>();
    }

    protected DbSet<T> DbSet
    {
        get
        {
            return dbSet;
        }
    }

    // ...
share|improve this answer
    
Yes Right that My Mistake, But I just want to know The Implemntation of CountryRepository And CountryController will work Or required some changes ?? If CountryControiller Imlementation is right then How write A Test Method against that ? –  user20915 Jan 8 '13 at 13:37

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