Recently, I’ve vowed to remove most of the model.find methods from my controller. I am not talking about simple find statements like:
model.find(1)
model.find_by_title("my title")
I am talking about find methods like so:
model.find(:all, :conditions => ["status = ?", my_status]) model.find(:all, :conditions => ["title = ?", my_title])
I would add methods on the model called, find_all_by_status(status) and find_all_by_title(title). Now, you may be thinking that these are simple find statements too. And I would agree with you. But in my opinion it makes the controller code look dirty. What would you rather see in your controller?
model.find(:all, :conditions => ["status = ?", my_status]) ~OR~ model.find_all_by_status(my_status)
Which one can you comprehend faster? For me, the first statement takes some time for my brain to process, where as the second statement doesn’t not. Ok, if this isn’t enough to convince you. What if you introduce a new boolean attribute on your model called ‘active’ and the logic is to only show active model instances? Now you have to change all the places where you have:
model.find(:all, :conditions => ["status = ?", my_status]) ~TO~ model.find(:all, :conditions => ["status = ? and active = ?", my_status, true])
You say you were thinking ahead and have a private method on your controller to handle this call, so you only have to make the change in one place. I would say congratulations, but I would point out that I think the controller knows too much about the model and thus breaks encapsulation.
I would like to hear what other think, and how they handle this situation.
In your final example, when you start using
model.find_all_by_status( my_status )
to:
model.find_all_by_status_and_active( my_status, true )
in any case? Either that or overload #find_all_by_status in your model (yuk). There are advantages to using _by_foo instead of :conditions, but you’re going to have to replace all those functions regardless.
I couldn’t agree with you more. I never put finders in my controllers. I don’t even put Model.find(:all) in my controllers. Never. And precisely for the reasons you’ve mentioned. With the way applications change, there’s no way to guarantee Model.find(:all) won’t soon have conditions, which would be a pain to change later.
The “M” in MVC should handle ALL business logic, even if “find all projects” is the logic. But that’s just my opinion.
You are correct that a controller method should never have underlying assumptions model.find_status (ie to only select active status). Better to have an explicit method such as model.find_activestatus (or however you want to name it), or a method that passes in both parameters.
Chad Fowler gave a great presentation at the first Rails Exchange in London where he spoke of the same idea. I try to follow it where I can but it is quite a discipline when you’re used to doing find(:all, conditions etc..) for the past 2 years.
My notes on Chad’s (and others) presentations are here
Plone has a good method of handling finds(), which could be summed up as “One model to find all models”. If you have a lot of models in your application, then you could end up with a lot of find_* methods. You might have Page.find_by_status() and then Image.find_by_status() and File.find_by_status(). An alternate strategy is to fire an event every time a model is modified, a listener would then update your metadata model. Then you could query your metadata model with something like:
metadatamodel.find(type=’MyModel’, state=’Active’)
This is overkill for a lot of applications, but it can be really nice in highly heterogenous systems such as a CMS to do a query like the one below without the expense of multiple queries or joins:
metadatamodel.find(type=[‘Image’,’Page’,’File’], state=’Active’, searchable_text=’my cat’)
Or to find all Page and Image object modified within the last week, sorted by modification date:
metadatamodel.find(type=[‘Page’,’Image’], modified=’2007-05-19’, modified_usage=’range:min’, sort_on=’modified’)
May be I am not getting your point, but we already have dynamic finders that can be used in this situation. Why you need to add methods like model.find_all_by_status in model???
@kevin – I’ve never heard of Plone, but I will take a look to see what its all about. It still seems like you know too much about underlying model structure. But again I might not know enough about Plone to make statements like that :)
@Akhil – I realize that there are dynamic finders for models, but I tend to stay clear of them just because they are dynamic. I like to look at a model and see what methods are available to me. Also, you still run into the problem of the underlying model structure changing, and your dynamic find method will no longer work. For example, if you had a model with a title and had find_all_by_title and then decided to change title to name, well you would have to change all the cases where you have find_all_by_title. If you added your own method, then you would only have to change it in one place – the model.
Graeme, I got your point, but in your case you will be using model.find_all_by_title which will be actually finding on other attribute named “name”. Then what about your own question “Which one can you comprehend faster?” . It will be confusing to other person also who’ll be reading that code. I mean when you read that code later say months after, you have to set you mind that find_all_by_title actually finding by “name”.
I’ll prefer to change all occurrence of find_all_by_title with find_all_by_name because that will be straight forward. And it will be only one time change.
@Akhil – good point! Probably shouldn’t reply to blog posts first thing in the morning :)