Controller actions are not code behind, or even vaguely equivalent ... it is perfectly justifiable to have data access code in a controller, controller actions are, for example, unit testable (unlike code bhind)

While I would normally put data access code further back, for the vast majority of web applications that are CRUD applications, putting in injected repository in the controller is perfectly reasonable.

That said, in the app I have been working on recently, the code base has been degenerating due to this practice, but that is more to do with the complexity of the data operations

If you cannot see a requirement to separate your data access totally from your web application at some point (i.e. behind a  services facade or on another machine), then pushing your data access calls behind another facade just to avoid putting data access requests in a controller is probably a case of YAGNI

The whole point of the repository pattern is that a repository pretends to be a collection of objects - the fact that it may go to a database is transparent to the object (controller) using it.

Derik, I think you make an important point.  Thinking back to the first MVC apps my company wrote using Monorail, this was exactly the type of problem we had.  We ended up with an anemic domain model with too much stuff jammed into the controllers; and controllers required lots of dependencies.  Recently, we've solved this problem by not only using services, but using the Command pattern.  All our commands live within our domain model, so it keeps not only core business entities in the right place, but core business actions as well (much as a service would).  This makes controllers very light weight and easy to test.  Not to mention that factoring things this way reduces a number of unnecessary dependencies.

@Casey,

I'm not sure what all you mean by "it is perfectly justifiable to have data access code in a controller", but I think that is where people are going to abuse the MVC framework the most.

I think the MVC crowd will likely get across the message to pull business logic and data access out of the views, which is a great first step. But the next logical place to try to put it will be the Controller Actions.

The controller actions are there to handle the request, figure out what needs to be done and then delegate off to other places for data and presentation.

If people end up putting unnecessary data access in the controller actions this will put us in yet another mess similar to putting data access and business logic in the code behinds.

Chris

I don't know that repository access in the controller is bad.  Is persistence a business rule?  I think not.  I see repositories existing at different levels of the application.  In the domain you may have a repository for shipping rules, something that will never be exposed at the application level.

For example in our app, we have complex rules about where and what can get shipped in the world.  When an order is created those rules need to be pulled from somewhere (ie a repository).  However this is not at the app level, this is down in the domain.  The order then needs to be persisted, this IS at the controller level.  The order doesn't care about persistence, nor should it.  So the repository for an order is at a different level than the shipping rules.

...end ramble.

Its all about SOLID.  Separation of concerns and single responsibility scream loud here.

Remember that MVC is just a UI pattern and not some kind of system architecture.

The model does all the work whether its wrapped around services or something else in your domain.  The controller should only be used to orchestrate messaging your domain and signaling a view to render.

When I first started monorail I had several large controller actions and it became a mess real quick.  It was tough to test because my controllers were doing way too much.  Splitting that logic out into application and domain services allowed me to focus on smaller problems quicker and easier.

I like to keep my controller actions lean like any other method.  5 -10 lines tops, and no that does not mean refactoring to a protected helper method inside the controller.

Btw, the term "skinny controller" I just lifted from RailsConf '07 and they probably lifted it from another community. It certainly isn't a hard and fast rule, but I think it serves as a useful guideline.

Chris

If I have a service that is called by the controller and this service calls a repository OR I have the controller call the repository, what's the difference, you end up with a service making a single call.

The controller's action IS the service layer for many things.

I tend to think that if you have a real domain model, you should probably be using services; however, lots of applications don't need a domain model.  If you are building a forms over data application, then there is almost no business logic (other than some validation, which could be on active record objects).  I have seen applications where each method on a "service" just calls a single repository and returns the result.  In these cases, I am all for cutting out the middle man.

@Tim

"The controller's action IS the service layer for many things."

I quote Martin Fowler "The controller's job is to take the user's input and figure out what to do with it."

Notice the language "figure out what to do with it" not "do it".

The controllers are not the service layer nor should be used as the service layer like it is in Derik's example.  The controller just figures out what service to call and what output to render.

Its subtle but important because a controller is just a piece of the UI framework not the application model.

Now in the realm of keeping things simple a controller depending on a single repository and making a single call to that repository does not justify an additional service in my book. But I suspect the controller that takes 5 repositories and makes 5 calls to compose a DTO started out with a single repository.

So maybe like the Page_OnLoad and Button_OnClick methods its just knowing when and how to refactor to something better?

I would venture to bet that 90%+ of controller actions in 90% of applications require a very simple interaction with at most 1 repository.

So the choice is whether from your controller you do:

itemRepository.GetAllItemsUpdatedThisWeek()

or you do:

itemService.GetAllItemsUpdatedThisWeek() - which then proceeds to make the call above.

However, like all things software, this is a judegement call you need to make on a case by case basis - but separation for the sake of it is silly ... if the controller knows it must call "a thing" to get the latest items to display, whether that "thing" is a repository implementation or a service class that wraps a repository is pretty irrelevant, hallelujah for IoC and ISP - all it needs to do is implement IFetchItems ... problem resolved

@Adam >>So maybe like the Page_OnLoad and Button_OnClick methods its just knowing when and how to refactor to something better?<<<

With webforms it was always time to refactor the code out of the handler, you couldn't test anything in the handler - with MVC it isn't so important - testability is a primary reason for separation here - in fact testability is often one of the primary reasons for going MVC over other patterns anyway.

Adam Tybor  said:  

"Remember that MVC is just a UI pattern and not some kind of system architecture."

golden

I inject a service object into my controllers

controller -> service (transactional) -> dao layer

works well.

My own personal reason for not injecting repositories into a controller is because I often don't want to expose my domain objects directly to the UI. A lot of times, I want a screen-specific DTO. Cry overkill or YAGNI if you like, it's saved me in the past.

I think Derek is right, as MVCs are taken up by more of the community the controller will indeed become the new 'code behind'...

Most developers still don't know what 'business logic' is or how to seperate out responiblity. SOLID most of them they are not.

Pingback from  Arjan`s World    &raquo; LINKBLOG for October 23, 2008

@Chris,

The "skinny" term comes from an epic post by Jamis Buck. The complete expression was "skinny controller, fat model". It's a great read and a classic for MVC platforms now.

weblog.jamisbuck.org/.../skinny-controller-fat-model

Because you don't do that in your controller; that is done in your models.

Your business logic goes into it's own assembly...you can still have a Business Layer outside of just Controller classes

Your controllers manage calls to your business logic methods which intern call DL methods

Controllers are not your business layer.  So that makes sense to abstract out a controller to facilitate what the code-behind used to do...it's just another abstraction above your business layer and below your presentation layer.  It's not bad to have logic to control what is sent to the views....that's what they are for.

So..I don't see the argument that controllers are bad.

My latest project is built using the Castle Project stack ( Monorail , Windsor , and Active Record )

My latest project is built using the Castle Project stack ( Monorail , Windsor , and Active Record )

Pingback from  The Fat Controller - taccato! trend tracker, cool hunting, new business ideas

Pingback from  The Fat Controller - taccato! trend tracker, cool hunting, new business ideas

Pingback from  The Fat Controller - taccato! trend tracker, cool hunting, new business ideas