In MVC is it considered good practice to have private, non-action, functions in a controller class?

Question

Sometimes action functions in the controller class can become huge and nasty, with many-many lines of code to simply control the flow of data from the Model to the View. At some point these huge functions completely lose track of the basic principles of good code, i.e. only doing one thing, being small, readable and manageable etc.

Would it be considered good practice to break these huge action functions into smaller private functions in the controller class or should the need of such optimization mean we should rather add them in the model?

I would vote for having the smaller functions as private in the controller so that they are relative to the action, but I have heard arguments that the controller should preferably be simple while the model can get huge and clumpy; and was just wondering which one would be the most preferred method.

Answer 1

Might not be the best analogy but, think about the controller the same way as you would think about a spider's web. Its sole job is to catch flies (requests) for the spider (underlying layers) to digest. The web can catch and hold smaller or larger flies (models). A spider's web role is not to digest the prey, although it can be used in this purpose. The thinner and cleaner the web, the easier for the spider to make a living.

You could apply somewhat the same logic to your MVC application. The huge and nasty functions you describe are most likely behavior of the model and they should belong in the model (note that the model is not only the object that's being displayed in the view). If the behavior of the model changes it's the model that should be changed and not the controller that handles it.

Also, keeping them as private methods in the controller would only clutter it and make it hard to maintain. It also makes way for a bad habit, since other people that are involved in development would be tempted to do the same, since they've seen it done before in the project.

Answer 2

The best answer I can give is to quote from the great book by Robert Martin, "Clean Code" that I highly recommend to anyone interested in the subject:

The first rule of functions is that they should be small. The second rule is that they should be smaller than that.

Can't say it better. Another great quote from the same book applies:

Functions should do one thing. They should do it well. They should do it only.

When splitting your code into more functions, you are forced to give those functions meaningful names that can greatly improve readability of your code. Needless to say, all functions not intended for use outside the class, should be private, so you can easily re-use your code via inheritance.

If your controller now has too many functions, it is a sign that it probably does too much. Then you may split it into several independent pieces or try to move some functions to models as mentioned in the other answer. Also if you follow non-classical MVC flavour, where Views are allowed to have some logic, you can put some of your functions there whenever it fits.

Answer 3

In MVC I try and ensure that my controller is as "thin" as possible and also that my models are as dumb as possible.

The logic and helper functions that are needed get put into separate stand alone helper classes. It makes my testing a lot easier as well (you are testing.. right?? :D) Testing controllers is notoriously difficult, any time you try and create an instance of a controller to test you have to think about the HTTP Context and faking http this and that, and its a pain, but its a pain on purpose. You need all that stuff because a controller is so closely linked to HTTP and the web. Its the entry point to your web app.

Logic and helper functions have nothing to do with the web. They're entirely environment agnostic (or they should be). That alone should tell you they don't belong together in the same place. Plus, if you tie all of your applications logic so closely to the web, or a particular web implementation, you can never take it with you.

We developed our MVC site with all of our database entities (not our mvc models, our actual db entities), our storage, our helper classes and our logic in seperate stand alone dll's. We have only every had one web site, but we did it like this anyway.

A few months ago we were asked to create a few desktop apps that are related to a few of our fringe systems. This was easily done as all of our tested code could easily be re-used. If we'd shoved our code into our web project, or put in in our controllers, we would never have been able to do this.

Answer 4

Beside of Dmitri Zaitsev and spaceman great answers i don't know if the following is also valid for PHP: You should try to avoid private methods due the lack of automated test possibilities.

Yes, you can use metaprogramming or dependency injection to test private methods as well but you shouldn't do it as it has a huge impact at the readability of your code.

Always remember the KISS principle: Keep it simple, stupid.