Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
add comment

4 Answers

up vote 13 down vote accepted

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.

share|improve this answer
    
+1 for the creative analogy. :) You make an interesting point. Especially on the forming of bad habit. Thank you. –  ddtpoison777 May 21 '13 at 8:52
add comment

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.

share|improve this answer
    
I don't think that putting business logic into views is "non-classical MVC", it's just "bad MVC". Obviously you need basic control structures in views, but they should be aligned with the user/UI concerns, not domain/business concerns. An actual function in a view is pretty horrible. –  Aaronaught Sep 5 '13 at 12:28
    
@Aaronaught I was vague with "some logic", what I had in mind is e.g. Backbone.js library, where you put user events and functions to handle them in your view. In classical MVC this is the job of controller. However this can be impractical as you would need to adjust both View and Controller each time your UI changes. By putting your UI handler functions in the View, you only need to adjust View. That's just my subjective view - am I missing something? –  Dmitri Zaitsev Sep 6 '13 at 8:03
    
Just because something gets delivered on the client side doesn't mean that it's logically part of the view. Data bindings in views, sure, but Backbone is itself an MV* framework (kind of MVC, kind of MVP, not quite either) and your client-side scripts should be organized accordingly; otherwise, you're just hacking. –  Aaronaught Sep 6 '13 at 16:00
add comment

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.

share|improve this answer
2  
The model in MVC is the only layer that isn't supposed to be dumb. If the smarts aren't in the model, and they aren't in the controller, then where are they... in the view? Controllers should also not be difficult to test; the ability to use DI and fakes/mocks to facilitate unit testing is one of the draws of MVC over other frameworks. Most of my controller tests are under 5 lines. –  Aaronaught Sep 5 '13 at 12:35
    
i would use a "helper" class with logic in rather than permeating a model with logic. what kind of logic would you put inside a model? does it know how to load itself and save itself? i agree faking / stubbing is easy, but its not an excuse to start fattening up your controllers. –  spaceman Sep 5 '13 at 13:36
    
I have a feeling that this answer means well but it is worded incorrectly.. or, perhaps differing terminology. –  Simon Whitehead Sep 5 '13 at 13:39
3  
"Helper" classes are not an architectural element. They're either part of the M, the V, or the C. If you're not sure which, then those helpers lack cohesion. The word "helper" also ranks right up there with "handle", "do", "perform", and the dreaded Manager. –  Aaronaught Sep 5 '13 at 13:39
    
@SimonWhitehead: Most answers mean well but many aren't correct. This one is, unfortunately, either promoting a misunderstanding of the meaning of "Model" or recommending putting critical business logic outside of it. I've had the dubious pleasure of maintaining MVC sites with a zillion "helpers" - they're dreadful. –  Aaronaught Sep 5 '13 at 13:42
show 2 more comments

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.

share|improve this answer
5  
That is not a good reason for avoiding private methods, and also doesn't have anything to do with the MVC architecture. You don't try to test private methods, they should be covered by tests on the public methods. If you can't cover them, then that's a sign that your class is too complex and needs to be refactored; it doesn't mean you shouldn't have private methods or (I sincerely hope this isn't what you really meant) that they should be public instead. –  Aaronaught Sep 5 '13 at 12:39
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.