is it bad practice that controller call repository instead of service?

to explain more:

I figure out that in good design controllers call service and service use repository.

but sometimes in controller I don't have/need any logic and just need to fetch from db and pass it to view.

and I can do it by just calling repository - no need to call service - is it bad practice?

  • How are you calling the service? Through a REST interface? – Robert Harvey Jan 8 '16 at 17:25
  • 2
    I use that design approach myself rather commonly. My controller (or an underlying composer class) will request data from or send data to the repo, and then pass it to any service classes that need to do processing. No reason to combine data processing classes with data retrieval/management classes, they're different concerns though I know the typical approach is to do it that way. – Jimmy Hoffa Jan 8 '16 at 17:27
  • 3
    Meh. If it's a small application and you're just trying to get data from a database, a service layer is a waste of time unless that service layer is part of some public API such as a REST interface. "Is milk good for you or bad for you?" Depends on whether you're lactose intolerant. – Robert Harvey Jan 8 '16 at 17:29
  • 4
    There isn't a hard and fast rule that you should have a Controller -> Service -> Repository structure over Controller -> Repository. Pick the right pattern for the right application. What I would say is that you should make your application consistent. – NikolaiDante Jan 8 '16 at 17:30
  • Maybe you could come up with a generic service that only forwards your request to the repository and then returns. This could be useful to keep a uniform interface and would be simple if in the future you need to add a real service to do something before call the repository. – Henrique Barcelos Jan 8 '16 at 19:13
up vote 15 down vote accepted

No, think of it this way: a repository is a service (also).

If the entities you retrieve through the repository handle most of the business logic there is no need for other services. Just having the repository is enough.

Even if you have some services that you must pass through to manipulate your entities. Grab the entity from the repository first and then pass it to said service. Being able to toss up a HTTP 404 before even trying is very convenient.

Also for read scenario's it's common you just need the entity to project it to a DTO/ViewModel. Having a service layer in between then often results in a lot of pass through methods which is rather ugly.

  • 2
    Well said! My preference is to call repositories, and only in cases, then a repository is not enough (i.e. two entities must be modified by using different repositories), I create a service which is responsible of this operation and call it from the controller. – Zygimantas May 11 at 19:19

It is not bad practice for a controller to call a repository directly. A "service" is just another tool, so use it where it makes sense.

NikolaiDante commented:

... Pick the right pattern for the right application. What I would say is that you should make your application consistent.

I don't think consistency is the most important aspect. A "service" class is meant to encapsulate some higher level logic so the controller doesn't need to implement it. If there is no "higher level logic" required for a given operation, just go directly to the repository.

To promote good Separate of Concerns and testability, the repository should be a dependency you inject into the service via a constructor:

IFooRepository repository = new FooRepository();
FooService service = new FooService(repository);

service.DoSomething(...);

If searching for records in the database needs some sort of parameterized query, a service class might be a good place to take in your view model and build a query that is then executed by the repository.

Similarly, if you have a complex view model for a form, a service class can encapsulate the logic of creating, updating and deleting records by calling methods on your Domain Models/Entities, then persisting them using a repository.

Going the opposite direction, if your controller needs to get a record by its Id, then delegating to a service object for this is like hitting a thumbtack with a sledgehammer -- it's way more than you need.

I have found that the controller is in the best position to handle the transaction, or a Unit Of Work object. The controller or Unit Of Work object would then delegate to service objects for complex operations, or go directly to the repository for simple operations (like finding a record by Id).

public class ShoppingCartsController : Controller
{
    [HttpPost]
    public ActionResult Edit(int id, ShoppingCartForm model)
    {
        // Controller initiates a database session and transaction
        using (IStoreContext store = new StoreContext())
        {
            // Controller goes directly to a repository to find a record by Id
            ShoppingCart cart = store.ShoppingCarts.Find(id);

            // Controller creates the service, and passes the repository and/or
            // the current transaction
            ShoppingCartService service = new ShoppingCartService(store.ShoppingCarts);

            if (cart == null)
                return HttpNotFound();

            if (ModelState.IsValid)
            {
                // Controller delegates to a service object to manipulate the
                // Domain Model (ShoppingCart)
                service.UpdateShoppingCart(model, cart);

                // Controller decides to commit changes
                store.SaveChanges();

                return RedirectToAction("Index", "Home");
            }
            else
            {
                return View(model);
            }
        }
    }
}

I think a mix of services and working with repositories directly is perfectly acceptable. You could further encapsulate the transaction in a Unit Of Work object if you felt the need.

The breakdown of responsibilities goes like this:

  • The controller controls the flow of the application
    • Returns "404 Not Found" if the shopping cart isn't in the database
    • Re-renders the form with validation messages if validation fails
    • Saves the shopping cart if everything checks out
  • The controller delegates to a service class to execute business logic on your Domain Models (or Entities). Service objects should not implement business logic! They execute business logic.
  • Controllers may delegate directly to repositories for simple operations
  • Service objects take data in the view model, and delegate to Domain Models to execute business logic (e.g. the service object calls methods on the Domain Models before calling methods on the repository)
  • Service objects delegate to repositories for data persistence
  • Controllers should either:
    1. Manage the lifetime of a transaction, or
    2. Create a Unit Of Work object to manage the lifetime of a transaction
  • 1
    -1 for putting DbContext into a controller rather than a repo. The repo is meant to manage data providers so nobody else has to in the event a data provider changes (from MySQL to flat JSON files for instance, change in one place) – Jimmy Hoffa Jan 8 '16 at 19:30
  • @JimmyHoffa: I'm actually looking back at the code I've written, and to be honest, I create a "context" object for my repositories, not necessary the database. I think DbContext is a bad name in this case. I'll change that. I use NHibernate, and the repositories (or context if it's handy) manage the database end of things, so changing persistence mechanisms doesn't require code changes outside the context. – Greg Burghardt Jan 8 '16 at 19:36
  • you seem to be confusing controller with repository by the looks of your code... I mean, your "context" is all wrong and should absolutely not exist in the controller. – Jimmy Hoffa Jan 8 '16 at 19:58
  • 5
    I don't have to answer and I'm not sure this is a good question to begin with, but I down vote because I think your approach is bad design. No hard feelings, I just discourage contexts being owned by controllers. IMO a controller shouldn't be starting and committing transactions like this. That's the job of any number of other places, I prefer controllers to delegate everything that's not simply fulfilling the HTTP request to somewhere else. – Jimmy Hoffa Jan 8 '16 at 20:29
  • 1
    A repository, it's typically responsible for all data context information to ensure that nothing else needs to know anything about data concerns other than what the domain itself needs to know of – Jimmy Hoffa Jan 8 '16 at 23:39

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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