Is it bad practice that a controller calls a repository instead of a service?

Question

Is it bad practice that a controller calls a repository instead of a service?

To explain more:

I figure out that in good design controllers call services and services use repositories.

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

And I can do it by just calling the repository - no need to call the service. Is this bad practice?

Answer 1

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.

Answer 2

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

Answer 3

It depends on your architecture. I use Spring, and transactionality is always managed by services.

If you call repositories directly for write operations (Or simple services without logic that simply delegate to the repository), probably you are using several database transactions for an operation that must be done in one. That will lead to incoherent data in your database. As a general rule, database operations should work, or should fail, but half-working operations are the cause of headaches.

For that reason I think that calling repositories directly from controllers, or using simple delegating services, is a bad practice. You begin doing it just for reading, and very soon you, or one or your mates will start doing it for write operations.

Answer 4

Yes, it is a bad practice. Controller should be used only to control the flow of your application, get input from client apps, call service and pass data to the view (JSON/HTML/XML/etc). Repository/DAO should be used to retrieve/persist the data without knowing any business logic.

Consider following scenarios:

• Somehow you need to add/modify role access for the data. You have to add the logic in any controllers that call the repository, or you have to put it in the repository (will broke logic of other services that call this repo). If you call via a service, you just need to modify the service, or just add another service, no need to worry will broke other services that call that repo.
• Let's say you have two clients of your application (Website and REST API), in the future, if you have to modify role access or business logic, you have to modify on both website and REST API controllers.