MVC design good/bad

I am curious what you find is good and bad MVC design.

Let’s use the find and find_by_sql methods as examples.
Currently I have these spread out all over the place…

View: here I have some find methods when I am for example populating a
select list with choices in a form.

Controller: here I have the simpler find methods.

Model: here I have the more complicated find methods (usually using
find_by_sql or find with lots of :conditions and :includes)

What do you think is good and bad about this?

View: here I have some find methods when I am for example populating a
select list with choices in a form.

I’d consider this a potential code smell. Doesn’t mean that it isn’t
the right choice in a given situation, just that I’d be on alert as to
whether my views are doing too much. I’d probably at least delegate
this to a helper that then makes the call.

David Heinemeier H.
http://www.loudthinking.com – Broadcasting Brain
http://www.basecamphq.com – Online project management
http://www.backpackit.com – Personal information manager
http://www.rubyonrails.com – Web-application framework

I’m new to Rails, but have done a bit of MVC-style work w/ Struts and
Websphere. In my (extremely humble and newbish) opinion, the controller
should tell the view what to display, and the view should display it.
Logic
other than some conditionals to decide the display should be abstracted
out
of the view.

I am not as familiar with the Rails concept of a helper function, I
would
certainly defer to David if that is the right place… but based on
past
MVC experience, I would say that this belongs in the controller.

-Will

I’d consider this a potential code smell. Doesn’t mean that it isn’t
the right choice in a given situation, just that I’d be on alert as to
whether my views are doing too much. I’d probably at least delegate
this to a helper that then makes the call.

This was my gut feeling too. I think I followed an example in the Agile
Rails book where a find method was in the view and then never moved away
from it.

Any thoughts on the model/controller and where to place queries (find
and find_by_sql)?

Which view in the Agile Rails book has a find method in the view?

Which view in the Agile Rails book has a find method in the view?
Page 347, but when reading it again I see that it does say: “Although we
show
the find() call adjacent to the select in this code fragment, in reality
the
find would probably be either in the controller or in a helper module.”

Ok, so now I know that I should never place any find methods in the
view. I guess that was the easy part of my question.

Now, what about find methods in Controllers and Models, where should
they go? Only in the Controller, only in the Model or both?

Thinking about this question in the light of dryness I would suggest
the following analogy may “shine some light” on the issue.

Imagine the earth is hollow and I shine a searchlight of 1 meter
diameter outwards towards the surface - I’d have a spot of light
about one meter wide on the surface. If I placed that searchlight at
the very centre of the earth, it would illuminate a far greater area
(albeit more weakly). Thus I would drive everything as far down as
make sense. Isn’t this the spirit of factoring and refactoring. You
just keep going till it makes no sense anymore.

Since views are the surface, the controller the mantle (if I may
stretch my analogy) and the models are the core - I’d push everything
layer by layer towards the model until it made no sense to continue.
As I understand MVC, Views are not meant to be “intelligent”, they
are like store windows that merely present what is put in them.

Bruce

As I understand MVC, Views are not meant to be “intelligent”, they are like store windows
that merely present what is put in them.

I agree with your entire statement, except I have a question about
this last comment: I agree that views should not be intelligent
regarding the model; I’ve never had a problem using conditions in
views to decide how or if to display a particular piece of information
(in particular, which partials to display in Rails)… am I wrong in
doing this? Should a view blindly display what it is given without
any logic at all? I’m a relative noob to Rails, so I’d like to hear
your thoughts on this… I’m always looking for ways to refactor my
code and make it cleaner.

Thanks for any insights you can provide!

-Will

depends…here’s how i tend to do it, YMMV…

if i find myself needing to get at a specific list of objects, for the
sake
of this conversation, say its a complicated find, i could do:

list = Model.find(:all, :joins => …, :conditions => …, :order =>
…)

now say i need this in several controllers/actions…i’d rather not have
to
type out that find everywhere i need it, so instead i might do

class Model < AR::Base
def self.complicated_find
find(:all, :joins => …, :conditions => …, :order => …)
end
end

then, everywhere i need it, i just do

list = Model.complicated_find

much easier and less prone to errors. hope this helps.

Again, strictly my opinion, but what you describe is an ideal for
which to strive. Ideally, the controller interacts with the model to
decide what the user needs to see and then passes that data to the
view, which simply displays it.

I can tell you from lots of MVC experience – good and bad – that
the closer you come to this, the cleaner your design will “feel” and
the more maintainable it will be.

On Jan 17, 2006, at 5:38 PM, Will B. wrote:

Should a view blindly display what it is given without
any logic at all?

-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.
-.-.-.-.-.-.-.-.-.-.-.-.-.-.-
Dan S.
Technology Visionary - Technology Assessment - Documentation
“Looking at technology from every angle”

I haven’t completed my first app yet, but I’ve
made a choice which I’m trying to stick to:

Any SQL goes in the model. Period.

I hope I can stick to it. :slight_smile:

– -- Tom M.

A simple rule that I find easy to follow is this. I never place
anything in view that takes more then one statement to do. This
includes looping structures and if statements for the most part. And
of course all the associated helper methods and the like.

I find, when using a bit of AJAX and the RJS templates for example, I
tend to have a rjs template that is pretty much a one or two clause if
statment to show the right stuff, but nothing besides that. I consider
views to be “smart” store windows. Windows that know what the nearest
passer by wants and shows them that.

For a more traditional, non-ajaxed app a lot of the if statments go
away, and the controller does indeed dictate a lot more on what gets
displayed, as it has to decide instead of the store window deciding.

-Nick

On 1/17/06, David Heinemeier H. [email protected] wrote:

View: here I have some find methods when I am for example populating a
select list with choices in a form.

I’d consider this a potential code smell. Doesn’t mean that it isn’t
the right choice in a given situation, just that I’d be on alert as to
whether my views are doing too much. I’d probably at least delegate
this to a helper that then makes the call.

I sometimes write code like:

<%= select ‘article’, ‘category’, Category.find(:all).map { |c|
[c.name, c.id ] } %>
Which display a select box for all categories.

Where should stuff like that go?

Would I want
<%= select ‘article’, ‘categories’, all_categories_for_select %>
?

(where all_categories_for_select is in application_helper.rb)

I have a somewhat different take on this, though a similar
philosophical objective.

(FWIW, lots of background in OOD and OOP in Smalltalk and JavaScript,
but fortunately no Java or C++ taint.)

Models, Views and Controllers do not exist in a hierarchy in which it
makes sense to me (YMMV) to think about “pushing everything layer by
layer towards the model.” Models, Views and Controllers all have
their individual and vital roles to play in an application. But
moving things from a model to a controller, e.g., only makes sense if
you realize that you’ve done something functional or operational in a
model because functionality and operationality belong in controllers,
not in models.

That said, code reusability comes from pushing things “up” the class
hierarchy to the highest level of abstraction that still makes sense.
Virtually all of the refactoring I’ve done in my OO experience has
involved that process, not shifting code from a model to a view or
from a view to a controller unless, as I say, I find I’ve just made a
bad decision at some point in which case that’s not refactoring, it’s
error correction!

Chapter 2 of Agile Web D. explains MVC pretty well. Models
are for holding the state of the app. Views are for generating UIs.
Controllers manage the app and the interaction between the model and
the view. The trichotomy is pretty clean most of the time. I find
that if I’m spending time trying to figure out where to put a
particular class or method, I need to take a step back and ask myself
what the purpose of the new code is. Very often, what results from
that process is a discovery that i need to factor into two pieces of
code, one in the controller and one in the view (at least that’s
where most of my confusion sorts out).

HTH.

On Jan 17, 2006, at 5:19 PM, Bruce B. wrote:

Since views are the surface, the controller the mantle (if I may
stretch my analogy) and the models are the core - I’d push
everything layer by layer towards the model until it made no sense
to continue. As I understand MVC, Views are not meant to be
“intelligent”, they are like store windows that merely present what
is put in them.

Bruce

-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.
-.-.-.-.-.-.-.-.-.-.-.-.-.-.-
Dan S.
Technology Visionary - Technology Assessment - Documentation
“Looking at technology from every angle”

Lots of interesting thoughts here.

Any SQL goes in the model. Period.

I like this approach (and by SQL I assume you mean both the normal find
and find_by_sql), does anyone else do this or do you think it’s taking
the MVC a little too far?

On 18.1.2006, at 5.17, Joe Van D. wrote:

this to a helper that then makes the call.
<%= select ‘article’, ‘categories’, all_categories_for_select %>
?

(where all_categories_for_select is in application_helper.rb)

I would put this in the controller:

@categories = Category.find :all

And then in the view

<%= select ‘article’, ‘categories’, @categories.map{|c| [c.name,
c.id]} %>

If you don’t need the categories for anything else, you can put even
the map part to the controller. If you use it in more than one
action, create e.g. a class method like Category.all_names_and_ids
that will return what you want.

//jarkko

Per Dj wrote:

Lots of interesting thoughts here.

Any SQL goes in the model. Period.

I like this approach (and by SQL I assume you mean both the normal find
and find_by_sql), does anyone else do this or do you think it’s taking
the MVC a little too far?

Not far enough. ActiveRecord wasn’t written with this in mind, but my
belief (and that’s all it is) is that the model code should be a
semantic representation of the business logic that the model encodes.
SQL is the job of the ORM layer, not model code.

Of course, we’re all used to thinking in terms of a combined ORM/model
layer, but it should be recognized for the bodge that it is…

Any SQL goes in the model. Period.

I like this approach (and by SQL I assume you mean both the normal find
and find_by_sql), does anyone else do this or do you think it’s taking
the MVC a little too far?

Except for very small applications, I tend to put everything in the
model
also, or at least in a helper.

I write tests quite often (TDD or not) and I feel I have a better grasp
on
the code I’m testing when I test the model or a helper, compared to
testing
a view.

Thibaut

Alex Y. wrote:

Not far enough. ActiveRecord wasn’t written with this in mind, but my
belief (and that’s all it is) is that the model code should be a
semantic representation of the business logic that the model encodes.
SQL is the job of the ORM layer, not model code.

Of course, we’re all used to thinking in terms of a combined ORM/model
layer, but it should be recognized for the bodge that it is…

+1

You make database calls all over your views and you might as well go
back to php. :wink:

I see a lot of discussion of the limitations of ActiveRecord on this
list. Well, there’s
nothing saying that you can’t create your model classes as the domain
dictates and have
them use the ActiveRecord objects as (extremely) convenient DAOs.

b

On Jan 19, 2006, at 2:19 , Ben M. wrote:

I see a lot of discussion of the limitations of ActiveRecord on
this list. Well, there’s nothing saying that you can’t create your
model classes as the domain dictates and have them use the
ActiveRecord objects as (extremely) convenient DAOs.

That sounds interesting. Could you expand on that, or perhaps provide
references to where I might find more information on this?

Michael G.
grzm myrealbox com