Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I want to make sure I'm following industry standards and best practices with my first real crack at MVC. In this case, it's ASP.NET MVC, using C#.

I will be using Entity Framework 4.1 for my model, with code-first objects (the database already exists), so there will be a DBContext object for retrieving data from the database.

In the demos I've gone through on the asp.net website, controllers have data access code in them. This doesn't seem right to me, especially when following the DRY (don't repeat yourself) practices.

For example, let's say I am writing a web application to be used in a public library, and I have a controller for creating, updating, and deleting books in a catalog.

Several of the actions may take an ISBN and need want to return a "Book" object (note this is probably not 100% valid code):

public class BookController : Controller
{
    LibraryDBContext _db = new LibraryDBContext();

    public ActionResult Details(String ISBNtoGet)
    {
        Book currentBook = _db.Books.Single(b => b.ISBN == ISBNtoGet);
        return View(currentBook);
    }

    public ActionResult Edit(String ISBNtoGet)
    {
        Book currentBook = _db.Books.Single(b => b.ISBN == ISBNtoGet);
        return View(currentBook);
    }
}

Instead, should I actually have a method in my db context object to return one Book? That seems like it is a better separation to me, and helps promote DRY, because I might need to get a Book object by ISBN somewhere else in my web application.

public partial class LibraryDBContext: DBContext
{
    public Book GetBookByISBN(String ISBNtoGet)
    {
        return Books.Single(b => b.ISBN == ISBNtoGet);
    }
}

public class BookController : Controller
{
    LibraryDBContext _db = new LibraryDBContext();

    public ActionResult Details(String ISBNtoGet)
    {
        return View(_db.GetBookByISBN(ISBNtoGet));
    }

    public ActionResult Edit(ByVal ISBNtoGet as String)
    {
        return View(_db.GetBookByISBN(ISBNtoGet));
    }
}

Is this a valid set of rules to follow in the coding of my application?

Or, I guess a more subjective question would be: "is this the right way to do it?"

share|improve this question

2 Answers 2

up vote 28 down vote accepted

Generally, you want your Controllers to do only a few things:

  1. Handle the incoming request
  2. Delegate the processing to some business object
  3. Pass the result of the business processing to the appropriate view for rendering

There shouldn't be any data access or complex business logic in the controller.

[In the simplest of apps, you can probably get away with basic data CRUD actions in your controller, but once you start adding in more than simple Get and Update calls, you are going to want to break out your processing into a separate class.]

Your controllers will usually depend on a 'Service' to do the actual processing work. In your service class you may work directly with your data source (in your case, the DbContext), but once again, if you find yourself writing a lot of business rules in addition to the data access, you will probably want to separate your business logic from your data access.

At that point, you will probably have a class that does nothing but the data access. Sometimes this is called a Repository, but it doesn't really matter what the name is. The point is that all of the code for getting data into and out of the database is in one place.

For every MVC project I've worked on, I've always ended up with a structure like:

Controller

public class BookController : Controller
{
    ILibraryService _libraryService;

    public BookController(ILibraryService libraryService)
    {
        _libraryService = libraryService;
    }

    public ActionResult Details(String isbn)
    {
        Book currentBook = _libraryService.RetrieveBookByISBN(isbn);
        return View(ConvertToBookViewModel(currentBook));
    }

    public ActionResult DoSomethingComplexWithBook(ComplexBookActionRequest request)
    {
        var responseViewModel = _libraryService.ProcessTheComplexStuff(request);
        return View(responseViewModel);
    }
}

Business Service

public class LibraryService : ILibraryService
{
     IBookRepository _bookRepository;
     ICustomerRepository _customerRepository;

     public LibraryService(IBookRepository bookRepository, 
                           ICustomerRepository _customerRepository )
     {
          _bookRepository = bookRepository;
          _customerRepository = customerRepository;
     }

     public Book RetrieveBookByISBN(string isbn)
     {
          return _bookRepository.GetBookByISBN(isbn);
     }

     public ComplexBookActionResult ProcessTheComplexStuff(ComplexBookActionRequest request)
     {
          // Possibly some business logic here

          Book book = _bookRepository.GetBookByISBN(request.Isbn);
          Customer customer = _customerRepository.GetCustomerById(request.CustomerId);

          // Probably more business logic here

          _libraryRepository.Save(book);

          return complexBusinessActionResult;

     } 
}

Repository

public class BookRepository : IBookRepository
{
     LibraryDBContext _db = new LibraryDBContext();

     public Book GetBookByIsbn(string isbn)
     {
         return _db.Books.Single(b => b.ISBN == isbn);
     }

     // And the rest of the data access
}
share|improve this answer
    
+1 Overall great advice, although I'd question whether the repository abstraction was providing any value. –  MattDavey Mar 2 '12 at 9:51
2  
@MattDavey Yeah, at the very beginning (or for the simplest of apps) it's hard to see the need for a repository layer, but as soon as you have even a moderate level of complexity in your business logic it becomes a no-brainer to separate the data access. It's not easy to convey that in a simple way, though. –  Eric King Mar 2 '12 at 13:13
    
yeah I wouldn't advocate using the DBContext directly in the application layer - but I think the contexts/sessions etc generated by ORMS are usually enough of a 'repository' in their own right and don't warrant an extra abstraction. In this case I'd think about having LibraryDBContext implement IBookRepository.. In any case I'd never say that a repository layer was ever a bad architecture :) –  MattDavey Mar 2 '12 at 14:28
1  
@Billy The IoC kernel doesn't have to be in the MVC project. You could have it in a project of its own, which the MVC project depends on, but which in turn depends on the repository project. I don't generally do that because I don't feel the need to. Even so, if you don't want your MVC project to call your repository classes... then don't. I'm not a big fan of hamstringing myself so that I can protect myself from the possibility of programming practices I'm not likely to engage in. –  Eric King Jan 28 at 14:54
1  
We use exactly this pattern: Controller-Service-Repository. I'd like to add that it's very useful for us to have the service/repository tier take parameters objects (e.g. GetBooksParameters) and then use extension methods on ILibraryService to do parameter wrangling. That way ILibraryService has a simple entry point that takes an object, and the extension method can go as parameter crazy as possible without having to rewrite interfaces and classes every time (e.g. GetBooksByISBN/Customer/Date/Whatever just forms the GetBooksParameters object and calls the service). The combo is been great. –  BlackjacketMack Apr 16 at 11:45

This is the way I've been doing it, although I'm injecting the data provider as a generic data service interface so that I can swap out implementations.

As far as I know, the controller is intended to be where you get data, perform any actions, and pass data to the view.

share|improve this answer
    
Yes, I've read about using a "service interface" for the data provider, because it helps with unit testing. –  scott.korin Feb 18 '12 at 19:06

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.