MVC : Controller responsabilities

Question

I have some doubts regarding the following controller action (in ASP.NET MVC, but it's a more generic question ) :

public ActionResult DoSomething( int id, IUser currentUser )
{
     var myDomainObject = businessService.GetDomainObjectById( id );

     if( !securityService.CurrentUserCanAcess( currentUser, myDomainObject ) )
     {
         throw new HttpException(403, "forbidden");
     }

     if( workflowService.IsWorkflowFinishedFor( myDomainObject ) )
     {
         return RedirectToAction( "OtherAction", identifier );
     }

     var myModel = entityToModelMapper.GetModel( myDomainObject );

     return View( myModel );
}

workflowService, securityService, businessService and entityToModelMapper are all injected into my controller with IoC.

I'm concerned about having security, business and workflow involved in the same controller action. Is it OK ?

Answer 1

IMO it is a clean design that should be "easy" to maintain. It may be a candidate for IOC to help testabililty, etc. so you are not creating concrete instances of some of the objects you are using but other than that it looks fine (again IMO).

I normally look at how portable a "web" action is to another technology (such as windows forms). So in this case, if you were to move the main code into another applciation your "user" may be resolved differently and the action if they are not authorised would definitely be different so I thnk that's fine to be a separate call. Then you have the main processing, again nicely separated. Finally, then and only then do you map the business object that's returned into a nice view model.

If nothing else, it is a fantastic clean starting point to go live with and modify as and when issues are exposed / identified.

Answer 2

If this is the processing you need then there's no alternative, they happen as part of the action.

The question might be whether some refactoring is appropriate. For example, whose responsibility should it it be to check the user's access rights? Here the action class makes the check and the myDomain object seems to allow anybody to read its contents.

Similarly the check for the workflow being finished: if the code of the action forgets to make that check, what happens?

My feeling is that in the current design, when extended to many action methods, this

Coders, please rember to check access permissions and workflow state

kind of logic could well be reproduced in many action methods - this is a bad thing, we have duplication of code.

Hence I think some refactoring to push that into the domain object, or a suitable wrapper is appropriate.