1

I have been looking around for an answer to my question but couldn't find one, mostly because I don't really know how to phrase it!

I am playing around with EF code first and trying to implement some sort of repository pattern whilst using dependency injection (thanks to Unity). I am also trying to keep to SOLID (well, the SRP part at least) in that I have IStaffRepository and IDepartmentRepository interfaces which in turn implement IRepository<TEntity> which provides basic CRUD methods.

My question is, how can I keep to the SRP when I need to update a staff members' department?

I have read that it is bad practice to use a single DbContext instance for all repositories due to possible memory leaks and so I cannot simply call the IDepartmentRepository to get the new department as this would use a separate instance of my DbContext.

The obvious solution to me is to include something like...

Department GetDepartment(int id);

Within IStaffRepository - but would this not break the SRP?

The code I have in my Controller is...

private IStaffRepository repository;
private IDepartmentRepository departmentRepository;

public StaffController(IStaffRepository repository, IDepartmentRepository departmentRepository)
{
   this.repository = repository;
   this.departmentRepository = departmentRepository;
}

public ActionResult Edit(int id)
{
   return View(new StaffViewModel(repository.GetItem(id)));
}

[HttpPost]
public ActionResult Edit(int id, StaffViewModel model)
{
   if (!ModelState.IsValid)
      return View(model);

   var item = repository.GetItem(id);

   // throws error due to different context
   item.Department = departmentRepository.GetItem(int.Parse(model.SelectedDepartment));

   UpdateModel(item, "Item");

   repository.Save();

   return RedirectToAction("Member", new {id});
}

And the StaffMember model contains...

public class StaffMember
{
   public virtual Department Department { get; set; }
}

And the StaffViewModel looks like this...

public class StaffViewModel : ViewModelBase<StaffMember>
{
   public SelectList DepartmentList {get;set;}
   public string SelectedDepartment {get;set;}

   public StaffViewModel()
   {
      var departmentRepository = new DepartmentRepository();
      DepartmentList = new SelectList(departmentRepository.GetCollection(), "ID", "Title", SelectedDepartment);   
   }

   public StaffViewModel(StaffMember item) : this()
   {
      Item = item;
      SelectedDepartment = Item.Department.ID.ToString();
   }

   public StaffViewModel(List<StaffMember> collection) : this()
   {
      Collection = collection;
   }
}  

In the database there is a int Department_ID field which hooks up to the Department table.

I have a drop down in my view which looks like...

@Html.DropDownListFor(m => m.SelectedDepartment, Model.DepartmentList, "--Please Select--")

Apologies for the length of this question!

simonlchilds
  • 2,730
  • 1
  • 16
  • 23
  • "*I have read that it is bad practice to use a single DbContext instance for all repositories due to possible memory leaks*": Do you have a reference for this statement? I believe it is terribly wrong. – Slauma Nov 04 '11 at 14:33
  • I read this from http://stackoverflow.com/questions/5187562/mvc-ef-datacontext-singleton-instance-per-web-request-in-unity#5191797 – simonlchilds Nov 04 '11 at 15:33
  • 1
    I think you misunderstood it. Ladislav meant there that sharing context *among multiple web requests* is a bad practice. Creating one single context (=unit of work) per request and injecting this same context into all repositories you use is fine. If you create your own context in every repo you run into the problems you have now. You can only possibly do this if your repositories represent aggregates of entites which belong together (one single repo for `Department` and `Staff`). But with one repo per entity and relationships between entities you must inject a single context into all repos. – Slauma Nov 04 '11 at 15:45
  • Oh ok, so to understand, would that mean using a single context for more than one database connection per request? – simonlchilds Nov 04 '11 at 15:52
  • No, the single context holds the database connection until the context is disposed. So, with one context you only have one connection. What I mean with injecting a single context into multiple repos is here: http://stackoverflow.com/questions/5823115/adding-user-to-a-role-inserts-a-duplicate-user-into-users-table/5823230#5823230 (the "Instead do this" part). – Slauma Nov 04 '11 at 16:01
  • please reconsider your design here see post below. You have data repository access in your ViewModel and your controller. – Adam Tuliper - MSFT Nov 04 '11 at 16:19
  • Thanks for your comments Slauma, you were very helpful. I've accepted Maess answer as it basically says the same as you did. – simonlchilds Nov 04 '11 at 16:20

3 Answers3

1

If all of your models are based on entities in the same database, there is not reason not to use a single context instance to manage them within the same controller. I would go with the simple solution and use a single context instance.

Maess
  • 4,032
  • 18
  • 29
  • based on the answer here http://stackoverflow.com/questions/5187562/mvc-ef-datacontext-singleton-instance-per-web-request-in-unity#5191797 I cannot use a singleton context – simonlchilds Nov 04 '11 at 15:37
  • Based on the comments in the OP, my comment above is invalid. – simonlchilds Nov 04 '11 at 15:55
0

If you're using the Repository pattern then you have at least a class which implements one of more repository interfaces. Since your code is coupled to an interface and not to an implementation you don't have to worry. However, you should treat the repository implementation as a singleton i.e every method should be independent and the class should not store state (you can store the conenction string since is basically a constant). So a method look like this

public void Save(Entity entity)
{
   using(var db=new MyDbContext())
   {
      //do stuff
   }
}

This way you're always disposing the data context and thus there should be no memory leaks As for SRP, the role of an repository is to get/save objects. If it does that it's good.

MikeSW
  • 15,292
  • 3
  • 33
  • 50
  • With this approach, in the edit method i would use one instance of the context to get the item to update and a separate instance to save it, which would not work. – simonlchilds Nov 04 '11 at 15:36
0

The problem potentially with a single instance is not releasing the context when it is done. If you use dependency injection, this will handle the disposing of this object when it is done. I do not recommend the design you have above either - you are giving your ViewModel data access behavior, which generally they aren't purposed for. A ViewModel has properties for the model and MAYBE some functionality specific to prepare that data. Inject a service layer into your controller whose responsibility it is to populate that viewmodel OR return a model. You can then just use automapper to map that model to your ViewModel. There's a neat attribute available as well to do this mapping.

Check out: http://lostechies.com/jimmybogard/2009/06/30/how-we-do-mvc-view-models/

Adam Tuliper - MSFT
  • 29,569
  • 4
  • 49
  • 68