2

I have some EF code to retrieve some objects in a controller, but I want to split off my functions to improve code reuse.

My code currently looks something like this:

public ActionResult SentMessages(){
    MyModel model = new MyModel();
    int user_id = GetCurrentUserId();
    using(DataContext db = new DataContext()){
        model.Messages = 
               db.Messages
               .Where(x => x.sent == true)
               .Where(x => x.user_id == user_id)
               .Where(x => x.date_deleted == null)
               .OrderBy(x => x.date_sent)
               .Take(10)
               .ToList();
        model.Groups = db.Groups
               .Where(x => x.user_id == user_id)
               .Where(x => x.date_deleted == null)
               .OrderBy(x => x.date_created)
               .ToList();
    }
    return model;
}

I want to split it into reusable code chunks, (and make my controllers smaller) like so

public ActionResult SentMessages(){
    MyModel model = new MyModel();
    int user_id = GetCurrentUserId();
    model.Messages = GetLastTenMessages(user_id);
    model.Groups = GetGroups(user_id);
    return model;
}

public static List<Message> GetLastTenMessages(int user_id){
    using(DataContext db = new DataContext()){
        return db.Messages
               .Where(x => x.sent == true)
               .Where(x => x.user_id == user_id)
               .Where(x => x.date_deleted == null)
               .OrderBy(x => x.date_sent)
               .Take(10)
               .ToList();
    }
}

public static List<Group> GetGroups(int user_id){        
    using(DataContext db = new DataContext()){
        return db.Groups
               .Where(x => x.user_id == user_id)
               .Where(x => x.date_deleted == null)
               .OrderBy(x => x.date_created)
               .ToList();
    }
}

However, this results in two separate connections to the database (as far as I understand). One is opened and closed for each query.

Is there any way to pass the context to the method, something like this

public ActionResult SentMessages(){
    MyModel model = new MyModel();
    int user_id = GetCurrentUserId();
    using(DataContext db = new DataContext()){
        model.Messages = GetLastTenMessages(user_id, db);
        model.Groups = GetGroups(user_id, db);
    }
    return model;
}

public static List<Message> GetLastTenMessages(int user_id, DataContext db){
        return db.Messages
               .Where(x => x.sent == true)
               .Where(x => x.user_id == user_id)
               .Where(x => x.date_deleted == null)
               .OrderBy(x => x.date_sent)
               .Take(10)
               .ToList();
}

public static List<Group> GetGroups(int user_id, DataContext db){        
        return db.Groups
               .Where(x => x.user_id == user_id)
               .Where(x => x.date_deleted == null)
               .OrderBy(x => x.date_created)
               .ToList();
}

Is there something like this I can do so that I can both separate out my code and also use the minimum number of database connections possible?

roryok
  • 9,325
  • 17
  • 71
  • 138
  • 1st implementation also causes 2 queries to db – EgorBo Nov 14 '13 at 14:41
  • 4
    It seems you have written solution yourself, what is wrong with it? – SergeyS Nov 14 '13 at 14:43
  • Does this cause two separate database connections, or are both queries executed under the same connection in example 3? (@nagg, that's what I meant - two _connections_, not queries) – roryok Nov 14 '13 at 14:51
  • Your proposed solution will work as expected. A better solution might be to use the Unit-Of-Work pattern that combines the data from the two entities: http://www.asp.net/mvc/tutorials/getting-started-with-ef-5-using-mvc-4/implementing-the-repository-and-unit-of-work-patterns-in-an-asp-net-mvc-application – jessehouwing Nov 14 '13 at 14:54

1 Answers1

2

I would move in the direction of a Service class first. So you could have a new class like this:

public class UserService
{

private DataContext _db;
//private int _user_id

public UserService(DataContext db)
{
   _db = db
   //perhaps it would be better to get the user id here
   //rather than pass it in to the methods as a parameter
   //_user_id = GetCurrentUserId();
   //or maybe put HttpContext into DataContext and do this:
   //_user_id = db.GetCurrentUserId();
}

private List<Message> GetLastTenMessages(int user_id){
        return _db.Messages
               .Where(x => x.sent == true)
               .Where(x => x.user_id == user_id)
               .Where(x => x.date_deleted == null)
               .OrderBy(x => x.date_sent)
               .Take(10)
               .ToList();
}

private List<Group> GetGroups(int user_id){        
        return _db.Groups
               .Where(x => x.user_id == user_id)
               .Where(x => x.date_deleted == null)
               .OrderBy(x => x.date_created)
               .ToList();
}

public MyModel GetSentMessages(int user_id)
{
        MyModel model = new MyModel();
        model.Messages = GetLastTenMessages(user_id, db);
        model.Groups = GetGroups(user_id, db);
        return model
    }
}
}

Then your controller will look like this

public ActionResult SentMessages(){
    using(DataContext db = new DataContext()){
        var us = new UserService(db);
        return View(us.GetSentMessages(GetCurrentUserId()));
    }
}

Then I'd look to introduce repositories for the data access. The UserService would then migrate to something like this:

public class UserService
{
    public UserService(DataContext db)
    {
       _db = db;
       _msgRepo = new MessageRepository(_db.Messages);
       _groupsRepo = new GroupsRepository(_db.Groups);
    }

    public MyModel GetSentMessages()
    {
            MyModel model = new MyModel();
            model.Messages = _msgRepo.GetLastTenMessages(db.user_id);
            model.Groups = _groupsRepo.GetGroups(db.user_id);
            return model
        }
    }
}

Then I'd think about wrapping the DbContext in my own UnitOfWork class. The UserService would then migrate to something like this:

public class UserService
{
    private UnitOfWork _uow;

    public UserService(UnitOfWork uow)
    {
        _uow = uow;
    }

    public MyModel GetSentMessages()
    {
            MyModel model = new MyModel();
            model.Messages = _uow.MessageRepo.GetLastTenMessages();
            model.Groups = _uow.GroupRepo.GetGroups();
            return model
        }
    }

And the Controller would migrate to this:

private UnitOfWork _uow;

public UserController(UnitOfWork uow)
{
    _uow = uow;
    _us = new UserService(_uow);
}

public ActionResult SentMessages()
{
   return View(us.GetSentMessages());     
}

protected override void Dispose(bool disposing)
{
    if (disposing)
        _uow.Dispose();
    base.Dispose(disposing);
}
Colin
  • 22,328
  • 17
  • 103
  • 197
  • I think there's a mistake in that first controller code shouldn't `return us.GetSentMessages(GetCurrentUserId());` be `return View(us.GetSentMessages(GetCurrentUserId()));`? I hadn't thought of generating the entire model in the service layer, that would make my controllers a lot smaller – roryok Nov 15 '13 at 09:27
  • Yes it probably should. I just copied the code from the question and didn't spot that. I'm actually not sure whether mapping to ViewModels is best in the Service Layer or the Controller. It's up to you really... – Colin Nov 15 '13 at 09:33