God Controllers - How to prevent them?

Question

In a few MVC projects I've been working on, it has become apparent that there are a few problematic controllers that have organically grown into God classes - demi-gods each in their own domain, if you will.

This question might be more of a matter 'what goes where,' but I think it's an important question with regards to SRP (Single Responsibility Principle), DRY (Don't Repeat Yourself), and keeping things concise, "agile" -- and I am not experienced enough (with this pattern and in general design) to be knowledgeable about this.

In one project, we have a NutritionController. Over time it's grown to include these actions (many with their respective, GET, POST, and DELETE methods):

Index (home controller)
ViewFoodItem
AddFoodItem
EditFoodItem
DeleteFoodItem
ViewNutritionSummary
SearchFoodItem
AddToFavorites
RemoveFromFavorites
ViewFavorites

Then we have an ExerciseController, which will include many similar actions, such as the searches, and favorites actions. Should these be refactored into their own controller so that it's something like so?

SearchController {
    SearchExercise
    SearchNutrition
    //... etc
}

FavoritesController {
    ViewNutritionFavorites
    AddToNutritionFavorites
    AddToExerciseFavorites
    EditNutritionFavorites
    EditExerciseFavorites
    //... etc
}

It just seems to me that if you break them out into separate controllers, you're going to grow an unbelievably large dependency at some level to deal with the information that you will need. OR you are going to have a completely generic handling application that will be very difficult to handle since you will have to jump through so many hoops to get your desired effect (either at the M, V, or C level).

I am thinking about this the wrong way? For example, should I have a generic Favorites object and then let the controller decide what view to throw it to?

*Sorry for spelling out the acronyms -- I'm doing so in case anyone else comes across this question and is clueless as to what those things are

EDIT: All the logic I perform is pretty much handled in the service layers. For example, the controller will send the 'new' FoodItem to the service. If it already exists, or there's an error with it, the service will bubble it back up to the controller.

Answer 1

I would break your first list up based on responsibility:

HomeController

• Index

FoodItemController

• ViewFoodItem
• AddFoodItem
• EditFoodItem
• DeleteFoodItem
• SearchFoodItem

NutritionController

• ViewNutritionSummary

FavoritesController

• AddToFavorites
• RemoveFromFavorites
• ViewFavorites
• SearchFavorites

Django's approach to MVC is to separate responsibilities into "applications", each with their own models, controllers, and even templates if necessary. You'd have a Food app, a Nutrition app, a Search app, and a Favorites app, most likely.

Edit: The OP mentioned that searching is more specific to each controller, so I've made those actions. However, searching may also be just a general global thing, so in those cases, a SearchController would be fine.

Answer 2

Do as Soviut says. You want to keep the controllers simple. It sounds like your ending up with too much coordination logic in your controllers. Remember they are responsible for hooking up a view and a model. This coordination logic should probably be split out into services.

I get this feeling because you mention the possibility of your controller growing huge dependencies. Well If FavoritesController needs to know about nutrition AND exercise favorites (to display in the same view) don't make your controller dependent on 2 repository like classes. Instead, encapsulate that coordination behavior. Maybe create a FavoritesService that knows how to return both nutrition and exercise favorites. That service might delegate to NutritionFavoritesService and ExerciseFavoritesService. This way you're controller only ends up with 1 dependency, you're keeping things DRY, enforcing the SRP, and concentrating your business logic in some place other than the controller.

Answer 3

I'm not too familiar with that framework, but I can offer a bit of general advice. A controller should probably only know how to complete a single action, or to invoke other, single action controllers to complete a sequence of related actions. Any information that must be passed around from action to action should probably be somehow passed through the model layer, since that information is most likely relevant to the underlying model.

Answer 4

I have also experienced these sorts of maintenance headaches and find sticking to a "Rails" like method to be very helpful in keeping my controllers focused and unbloated.

If I find myself adding actions with unusual names Eg. To use a blogging example, AddPostToBlog, that would be a flag to create a new Post controller with a Create action.

In other words, if the action is not either one of the Index, New, Create, Show, Edit, Update and Destroy actions, then I add a new controller specific to the action I require.

For your example.

SearchController {
    SearchExercise
    SearchNutrition
    //... etc
}

I would refactor this to...

   SearchExerciseController {
           Index   
   }

   SearchNutritionController {
           Index   
   }

This may mean having more controllers, but in my opinion thats easier to manage than ever expanding "god" controllers. It also means that the controllers are more self documenting.

Eg. Does the SearchExercise action, return the View to search the exercise or does it actually perform the search? You could probably ascertain this by looking at parameters and body, but its not as easy as for example a New and Create, or Edit and Update action pair.

SearchController {
    SearchExercise       
}