0

In my View:

  <%= f.collection_select :product_id, Product.find(:all), :id, :name %>

I'm referring to Product.find(:all) in the above code.

Or in some cases we define a function in the model and we call it in the view: ModelName.my_function

If not ! whats the best way to do it without disobeying MVC principles?

I mean is it the best architecture we can follow or we have something else?

Sachin Prasad
  • 5,365
  • 12
  • 54
  • 101
  • I don't see anything wrong. You are creating a dropdown with name "product_id" and the options with name & id of all the savailable products. – Satya Kalluri May 22 '13 at 10:19
  • @satya I mean I'm doing a query in my view Product.find(:all) to get all the products for the dropdown. – Sachin Prasad May 22 '13 at 10:21
  • .. i would argue that Rails as whole is not implementing MVC, but that might be just [my opinion](http://stackoverflow.com/a/11984678/727208) – tereško May 22 '13 at 10:23
  • That's OK Sachin. The other way would be to initialize products in the controller. (products = Product.all) and use @products in the view. "Products" should be an instance variable in the controller. I could not apend at-the-rat-of in this comment :( – Satya Kalluri May 22 '13 at 10:25

3 Answers3

2

The View Shouldn't (Generally) Talk to a Model Directly

As a rule of thumb, the view should not be directly communicating with a model. There are probably times that violating this principle makes sense, but without more context I would assume that this isn't one of them.

In the Rails version of MVC:

  1. The model should define behavior.
  2. The controller should assign models and collections to instance variables which are then passed to the view.
  3. The view should format the attributes of the model as passed in from the controller as instance variables.
  4. In some cases, calling methods on objects passed in from the controller is warranted (not everything needs to be pre-computed into variables by the controller) but the view should not invoke the model directly.

As with all things OOP, your mileage may certainly vary.

Todd A. Jacobs
  • 81,402
  • 15
  • 141
  • 199
  • So you mean doing Product.find(:all) in view is incorrect. Then how to handle this? – Sachin Prasad May 22 '13 at 19:05
  • @SachinPrasad웃 You should perform your search in a controller, and assign it to a variable that can be passed to the view. For example: `@products = Product.all` generally belongs in a controller action. – Todd A. Jacobs May 22 '13 at 19:17
1

That is about as MVC as it gets.

The role of the controller in this simple case is to make the correct model available to the correct view (mediation). Thanks to Rails, you did not have to actually write any controller code for that to happen. That is kind of the point of the Rails MVC framework - eliminate the mundane, boilerplate pattern implementation from your application.

Semantically, using Products.find(:all) in the view and assigning @products = Products.find(:all) in the controller and then referencing @products in the view are the same thing, but why would you deliberately choose the technique that requires more code? (If you need to refactor later, easy enough, but don't refactor prematurely or you end up with cruft).

And, Rails will cache the query (for the life of the request), so it does nothing for performance, even if you use Products.find(:all) many times in the same view.

If you are going to scope your query, you would do that in the model, and again, you should not need any controller code.

When do you need controller code? Perhaps to perform authorization or some domain specific logic, or you need to respond with an alternate (not HTML) format, such as JSON.

pduey
  • 3,706
  • 2
  • 23
  • 31
1

The (V)iew should not directly talk to the (M)odel, which is what the Product.find(:all) is doing. Ideally what you would want to do is pass a local variable in to your view, from your controller.

Despite that being the dogmatically correct way to do it, some times it is not practical to pass in every last variable through locals. In those cases you should probably consider creating a helper to do it instead. That way it can have more semantic meaning:

 <%= f.collection_select :product_id, Product.find(:all), :id, :name %>

becomes

 <%= f.collection_select :product_id, product_options_for_select, :id, :name %>

and you have a helper associated that looks something like this:

def product_options_for_select
  Product.find(:all)
end

Which makes it at least a little more maintainable, particularly if you reuse it, in case you need to return a differing subset in the future.

Christopher WJ Rueber
  • 2,141
  • 15
  • 22
  • I'm really having hard time explaining this to a Java developer that this is a good practice and we are OK with this. Can you please help me? – Sachin Prasad May 22 '13 at 18:29
  • I think teaching basic MVC is probably beyond the scope of this basic question, but I'd recommend reading up on MVC ( here is one good reasource: http://programmers.stackexchange.com/questions/105352/why-should-i-use-an-mvc-pattern ), and then perhaps explaining why certain pieces fit where they do, and how it could ease maintenance, etc. Luck! – Christopher WJ Rueber May 22 '13 at 18:35
  • I mean this piece of code isn't following MVC right ? We are calling a helper and in return its calling the model. Isn't it that we have to call a controller and then it should call the model? – Sachin Prasad May 22 '13 at 18:58
  • Can we call a model directly from a view? – Sachin Prasad May 22 '13 at 19:02
  • That doesn't follow MVC, no. Because the "C" is supposed to gather the data, and the view is just supposed to show the data that has been gathered. Think of the view as pure presentation. By the time that's being rendered, you shouldn't need to get any more data. That's why even the helper isn't really ideal. Because you're still getting data while you're in the view layer. – Christopher WJ Rueber May 22 '13 at 20:25