Should you use Model::query() in the controller?
The query() function of the model is sometimes very practical as it allows you to execute any SQL statement you want. But to me it seems it is often used in a “wrong” way (I have to include myself *g*), in a way which works but is not that clean. I am talking about calling query() from the controller:
$this->MyModel->query('Here comes the SQL statement');
Sure, following the Cake conventions this is a valid usage of query(), as the function is public. But it introduces database-related code to the controller, which is a violation of the MVC pattern.
I think a cleaner solution is to move the SQL statement to the model. So the snippet from above can be refactored to:
// in the model
function doSomething() {
$this->query('Here comes the SQL statement');
}
// in the controller
$this->MyModel->doSomething();
Ok, this solution needs more code, but it comes with some additional advantages:
- it is easier to test
- it is more readable (at least if you give the function a better name than I did)
- the query can be re-used without copy & paste in other actions/controllers
What do you think about this approach?
“What do you think about this approach?”
I think you are right :).
AD
Booo! I’m disappointed that my comment was dated at the same times as your article instead of before it - it’s 11:27 :)
AD
I think you’re right.
BUT, most of the time, when i’m using the query() i only use that query once within the whole application, so DRY is no argument here, and if it’s against the MVC pattern - thats ok for me, every file opened in zend studio is a file too much :)
The problem is you are going to create a new method which does nothing else but being a proxy for another method :/
Whenever it’s needed by the model itself, or is to be reused, I think you’re right. Otherwise you’re simply wrong. If you merely want to push each SQL query out of the controller, for the cleanliness of the code… I think it’s pushing DRY too far, just as Tom just said.
Shouldn’t this be logical, if we follow the MVC pattern?
I think you are absolutely right, all data related codes should be in the model. It is always a better idea in the long run, when it comes to refactoring or testing. You know exactly where to find the code.
i think you are right (me three)
i’m modeling this type of tree http://dev.mysql.com/tech-resources/articles/hierarchical-data.html
and tonight i will move all queries in model
you’re (obviously) right
Ever since I read http://weblog.jamisbuck.org/2006/10/18/skinny-controller-fat-model I have always tried to put as much in the model as I can, especially if it is data related.
Yeah, perfect, that’s what I do
Just curious.. when would you feel the urge to put a query in the controller anyways. Like, give us a situation when that might actually make a lot of sense?
Benjamin - I’m currently refactoring my application and found one example of this. The query() was not actually in a controller, but in a component, which I feel is just as bad.
The reason it started here is because the query needed to do a complex join across four different models, and it didn’t seem particularly appropriate to be in a single one. If you are using a direct query() at all, chances are it is a complex query that cannot be handled by the existing model methods (find/findAll).
However, in my refactoring I have moved this to one of the models involved, just because it does make the component execution more readable, and make the query itself more easily unit tested.
This is, in my opinion the most obviously correct approach. After all, a query of a model’s data is quite obviously a function of the model itself, as you say - any use of it in the controller is an invalid use of the MVC pattern.
The link Joel Moss posted above was a very interesting read, and I’d suggest anyone new to perhaps MVC or cakePHP should read it. It opened my eyes a bit.
Now I’m going to go hang my head in shame as i do a find all for ‘query(’ in controllers/*
Let us not forget that CakePHP was designed for Rapid Application Development. Using Model::query() in the Controller is a fast and practical way of prototyping (from concept to working code).
Creating a specific method in the Model for the specific query is generally done when code re-usability is being consider, which occurs when a working code and prototype has been done.
The problem is, when time is a constrain, developer will forget about code re-usability and let the prototype work as is.
Copongcopong , imho you’re confusing two methods to achieve “faster development”.
the first is by using libaries who provide an api that lets you create applications fast, but decently, offering an advantage on both the long and short term.
the second method is by writing little hacks that give you a (modest) advantage in the short term, but becomes deadly on the middle and long term. I know you are talking about short-term, but even then
this method should imho _never_ be used. Why? Because also during short term development it’s better to have a good separation of your logic. Think about it, even on the short term, which would cost you the most time ?
The “overhead” of wrapping the logic in a place where it belongs versus the time you would otherwise need (by writing the same code multiple times, by needing to fix the same bugs in several places, etc etc) Even when you just need to fix 1 little bug in 2 places or without bugs, the need to copy-paste code between 2 places already is worse then the small amount of time it needs to put the code where it belongs.
for me, this is a no-brainer
@All: Thanks for your comments.
@Tom, JMG: Yes, you are right, if I have a query which is used once, it doesn’t matter whether it is in the model or the controller from a DRY point of view. But for me it is also important that the code is readable, and a method call is for me more readable than a non-trivial SQL statement.
@Ahsan: Yes, it should be logical, but that doesn’t mean it is also done ;-) In earlier applications I used query() in the controller, too, but I had to learn that it isn’t the best idea…
@Joel Moss: Thanks for your link, it is an interesting article.
@Copongcopong: Well, I think it is ok to use query() in the controller if you write a prototype you throw away later. But as Dieter@be said, it is recommended to have a good separation of the logic. My experience is that you usually have to pay later much more than you saved with some shortcuts.
I think that this is a good idea. Just recently I had the very same issue with query not working in the controller and then realizing I had to move it to the model. We have similar db functions that we use in the controller, so why not query().
This is how I currently handle all of my custom SQL. I am new to Cake, but not new to the MVC pattern. I don’t think this is pushing DRY too far. The principle behind it is to decouple from business logic and templating. If I were to switch database layers, then I am not having to edit a bunch of business logic with embedded SQL - versus editing my model layer.
It might be an extra step, but well worth it to keep things neat and tidy. I think it would also help for debugging.
To those who think it pushes DRY to far, what are your thoughts about putting business logic inside of the views?
I agree totally. I try to stick to the MVC pattern as much as possible. That doesn’t mean I have everytime, but you understand.
from a semantic POV, $this->Model->getProductsBySomeCriteria()
is far more meaningful than $this->model->query($bloatedYAY);
specially when your prototyping, workflow etc..
in a collaborative environment it becomes more crucial, someone will read your code, of course you can put comments in there (and you should)..but separating that piece of code and putting it where it should be the best, is helpful. because that person who’s reading your code can understand what you’re doing and continue reading the flow of your code, later he will decide to discover further by reading that function. and he already knows what it’s doing, he now wants to know how it was done.
Is there any way to ‘pull’ data from the models directly from the views? How would you go about calling model methods directly from the view instead of the controller pushing data to the view? Is it as simple as $this->Model->myMethod() within the views? I’ve yet to see any example of this in Cake.
In the article that Joel posted, RoR can simple make calls to its model completely by passing the controller for reading data.
If you the views are not aware of the models, then I’m questioning if Cake is a true MVC framework, but perhaps more of the PAC framework.
@Jonathan: No, you cannot access a model from the view. The controller decides, which data go to the view.
The same approach is also used in the RoR article, the find method is called in the controller, which then makes the data available for the view.
@Cakebaker: Later in the article the author demos that you simple pull read only data straight from the model and no need to pull data from the controller and push it to the view. That’s MVC and not MVC-like. That’s why I question if Cake’s view are aware of the models, which at first glance it is more like a PAC framework (MVC-like).
In the author’s RoR example:
“>
@Cakebaker: That’s too bad, then I would call Cake a PAC framework and not a MVC framework.
Later in the RoR article, the author illustrates that you call the model’s methods directly from the view. When its read only data, there’s no need to pull data into the controller and push it into the view.
@Jonathan: Yes, in the RoR article they call the model attributes directly to get the data. That’s not possible with Cake, as Cake works in the current versions with arrays and not with objects.
I think it depends on the point of view whether you call cake a MVC framework or a PAC framework. But in the end it’s not that important…
When I got started with CakePHP I was also wondering where to put custom queries. You should indeed put them inside the model at all times. Even though it may seem you will only use the query once at the time, it`s still possible you will need it elsewhere as you make progress in developing your application.
More or less related: For a similar reason I don`t like the way the recursive option works in Cake: When you add tables in your database as you are developing, existing code might pull in data from tables you don`t care about.
@Tim: Thanks for your comment. To avoid the problem you described you could use the approach explained in http://bakery.cakephp.org/articles/view/185