Forum: Ruby on Rails MVC design good/bad

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
5eb7dc9121bd474726f9f5568c9573ed?d=identicon&s=25 Per Djurner (pernod)
on 2006-01-17 23:13
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?
6edd67c92a1dab5eb23fed79f3c18564?d=identicon&s=25 David Heinemeier Hansson (Guest)
on 2006-01-17 23:23
(Received via mailing list)
> 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 Hansson
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
9b7647d55df4584d7031130915556040?d=identicon&s=25 Will Briggs (Guest)
on 2006-01-17 23:26
(Received via mailing list)
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
5eb7dc9121bd474726f9f5568c9573ed?d=identicon&s=25 Per Djurner (pernod)
on 2006-01-17 23:43
> 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)?
6661ef9d747db3af8896cd94959d717d?d=identicon&s=25 Paul Barry (Guest)
on 2006-01-18 00:54
(Received via mailing list)
Which view in the Agile Rails book has a find method in the view?
5eb7dc9121bd474726f9f5568c9573ed?d=identicon&s=25 Per Djurner (pernod)
on 2006-01-18 01:27
> 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?
A2c85dc5ee81b12e3cc0a6522e8d079d?d=identicon&s=25 Chris Hall (Guest)
on 2006-01-18 02:21
(Received via mailing list)
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.
2dd904ec5981c31e7bb7a5743a53caf8?d=identicon&s=25 Bruce Balmer (brucebalmer)
on 2006-01-18 02:58
(Received via mailing list)
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
9b7647d55df4584d7031130915556040?d=identicon&s=25 Will Briggs (Guest)
on 2006-01-18 03:16
(Received via mailing list)
> 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
205bc8d44e9bc5c68d77dd412abcb3ce?d=identicon&s=25 Dan Shafer (Guest)
on 2006-01-18 03:22
(Received via mailing list)
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 Development 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 Balmer 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 Shafer
Technology Visionary - Technology Assessment - Documentation
"Looking at technology from every angle"
http://www.eclecticity.com
205bc8d44e9bc5c68d77dd412abcb3ce?d=identicon&s=25 Dan Shafer (Guest)
on 2006-01-18 03:34
(Received via mailing list)
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 Briggs wrote:

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



-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.
-.-.-.-.-.-.-.-.-.-.-.-.-.-.-
Dan Shafer
Technology Visionary - Technology Assessment - Documentation
"Looking at technology from every angle"
http://www.eclecticity.com
59de94a56fd2c198f33d9515d1c05961?d=identicon&s=25 Tom Mornini (Guest)
on 2006-01-18 03:37
(Received via mailing list)
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. :-)

-- -- Tom Mornini
Cb610750ee94ca103aef4b2dc7b1b768?d=identicon&s=25 Nick Stuart (Guest)
on 2006-01-18 04:01
(Received via mailing list)
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
38a8230ed3d5c685558b4f0aad3fc74b?d=identicon&s=25 Joe Van Dyk (Guest)
on 2006-01-18 04:55
(Received via mailing list)
On 1/17/06, David Heinemeier Hansson <david.heinemeier@gmail.com> 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)
82476266af9d460415d8f1fc16bb54ed?d=identicon&s=25 Jarkko Laine (jarkko)
on 2006-01-18 09:02
(Received via mailing list)
On 18.1.2006, at 5.17, Joe Van Dyk 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
5eb7dc9121bd474726f9f5568c9573ed?d=identicon&s=25 Per Djurner (pernod)
on 2006-01-18 11:25
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?
91eb330fb36d1e03c856574dfb77d2bc?d=identicon&s=25 Thibaut Barrère (Guest)
on 2006-01-18 12:09
(Received via mailing list)
>
> > 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
Ad7805c9fcc1f13efc6ed11251a6c4d2?d=identicon&s=25 Alex Young (Guest)
on 2006-01-18 12:45
(Received via mailing list)
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...
4005a47a8f2ceee49670b920593c1d52?d=identicon&s=25 Ben Munat (Guest)
on 2006-01-18 18:54
(Received via mailing list)
Alex Young 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. ;-)

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
84962fcce3fbde5aef15001d60e242ae?d=identicon&s=25 Michael Glaesemann (Guest)
on 2006-01-20 10:11
(Received via mailing list)
On Jan 19, 2006, at 2:19 , Ben Munat 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 Glaesemann
grzm myrealbox com
4005a47a8f2ceee49670b920593c1d52?d=identicon&s=25 Ben Munat (Guest)
on 2006-01-20 18:56
(Received via mailing list)
Michael Glaesemann wrote:
 > On Jan 19, 2006, at 2:19 , Ben Munat 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?
 >

No, sorry I can't... cuz I was just theorizing. I don't know if anyone's
actually done
this. It would probably be an uphill effort because rails does so much
for you as long as
you "stick to the script".

I would think that rather than the usual DAO approach, in which the DAOs
query the
database and return an assembled model object, the ActiveRecord might be
good as just a
member variable of the model. You could have a model base class that has
the ActiveRecord
var and the basic delegate methods. The actual instance of ActiveRecord
would be set in
the subclass model object.

Or, you could have a helper, basically a DAO, that creates an
ActiveRecord instance for a
specified table and handles db calls on behalf of the corresponding
model class. This
could be arranged using the "aggregates" approach Eric Evans advocates
in the book Domain
Driven Design. Well, that pattern is intended to control the chaos of
too many classes
talking to too many other classes in a large system. But the "root" of
the "aggregate"
could also be respsonsible in our system for maintaining ActiveRecord
objects for all of
its child objects.

Now, I'm just blundering around here, theorizing off the cuff... so
please take this as
such. Like I said above, the problem with all this is that rails is
based on a merged
domain and data access layer; its one fatal flaw if you ask me... but it
does make small
apps very easy to kick out.

I suspect that someone will want separation of concerns bad enough that
they'll write a
plugin or module to facilitate using "POROs" (plain old ruby objects)
for the model with
mixins to use whatever data access library you want.

A good first step towards this would be to separate Validations out of
ActiveRecord,
allowing it to be mixed in to model classes. And ActiveRecord should be
a mixin in too. My
model classes don't have an "is-a" relationship with ActiveRecord.
That's because
persistence is an orthogonal concern and it should be treated as such!
I'm surprised that
someone like Martin Fowler hasn't taken DHH over his knee and spanked
him for this yet.

Ahem. Anyway, once again... these are the ravings of someone who's read
about three
quarters of the agile book and played with a few examples. Flames
welcome. :-)

b
69b2ef4bce76b5b27c94e898976dc6d8?d=identicon&s=25 matthew clark (Guest)
on 2006-01-20 19:27
(Received via mailing list)
No flames from my corner.  I really like where this discussion is
leading
to.  I think that as Rails grows up from being a simple CRUD front end
flexibility in the data structure is inevitable.

There was a thread less than a week ago about pulling the validations
stuff
out of ActiveRecord, and into a mixin.

http://www.ruby-forum.com/topic/51771#23335

matt
37b7b848b1e3d667174d28d32d04bcee?d=identicon&s=25 Charles M. Gerungan (Guest)
on 2006-01-21 11:51
(Received via mailing list)
On 20-jan-2006, at 18:49, matthew clark wrote:

> I think that as Rails grows up from being a simple CRUD front end
> flexibility in the data structure is inevitable.

Has Rails ever been "a simple CRUD front end"? Seems people will
always associate Rails with scaffolding...

Don't let David hear you! :p

--
Regards, Charles.
4005a47a8f2ceee49670b920593c1d52?d=identicon&s=25 Ben Munat (Guest)
on 2006-01-21 19:39
(Received via mailing list)
Well I don't think of rails as a simple CRUD front end, but I do think
having to make all
you domain classes subclass a framework class is short-sighted.

Ironically, this is one of the things complain about struts for: it
depends heavily on
subclassing. And, when I described ActiveRecord to my co-worked, his
eyes lit up and he
said, "a-hah... they've come up with their own EJB CMP!" (although
obviously without all
the deployment descriptors).

And I think this "simple CRUD front end" urban lengend is just the first
of a host of
annoying associations rails will be pegged with...

b
Df040ca3576504b24a73744179903277?d=identicon&s=25 Tobias Luetke (Guest)
on 2006-01-21 20:53
(Received via mailing list)
What a ludicrous comment.

On 1/20/06, matthew clark <winescout@gmail.com> wrote:
> No flames from my corner.  I really like where this discussion is leading
> to.  I think that as Rails grows up from being a simple CRUD front end
> flexibility in the data structure is inevitable.
--
Tobi
http://jadedpixel.com    - modern e-commerce software
http://typo.leetsoft.com - Open source weblog engine
http://blog.leetsoft.com - Technical weblog
6076c22b65b36f5d75c30bdcfb2fda85?d=identicon&s=25 Ezra Zygmuntowicz (Guest)
on 2006-01-21 21:49
(Received via mailing list)
On Jan 21, 2006, at 10:38 AM, Ben Munat wrote:

> And I think this "simple CRUD front end" urban lengend is just the
> first of a host of annoying associations rails will be pegged with...
>
> b


	There is nothing about the way rails works that forces you to use a
subclass of ActiveRecord as your domain models. Most of the models I
use for the newspaper website I wrote are in fact just custom ruby
interfaces to proprietary Baseview databases that a lot of newspapers
use.

	In fact you could just as easily use dbi or whatever you want for
your model files. ActiveRecord is of course the nicest way to do
models in rails right now since the framework makes it so easy to
work with but you can use anything you want as your model

Cheers-
-Ezra Zygmuntowicz
WebMaster
http://yakimaherald.com
Yakima Herald-Republic Newspaper
ezra@yakima-herald.com
509-577-7732
16be06b16d37545f788c64f48955ce47?d=identicon&s=25 Andrey Tarantsov (Guest)
on 2006-01-21 22:18
> ActiveRecord is of course the nicest way to do
> models in rails right now since the framework makes it so easy to
> work with but you can use anything you want as your model

Of course you can, and you can even throw away all the rails code and
rewrite it from scratch. The thing people are talking about that it
should be easy to use non-AR models, while still enjoying standard
validations, type casting etc.

BTW, I think besides validations, there are two other important areas:
type casting and relationships (belongs_to etc).

The lack of type casting for custom attributes hurts most when you e.g.
want a non-persisted date field in your AR model. While the fields
coming from DB can automatically assemble a date from year-month-day
parts and have other nice things, when using custom attributes you're on
your own.

Whether and how belongs_to, has_many etc can be used for custom models
is much less clear. I think there should be a way of customizing how
related rows are fetched even for AR models, and probably this will work
for non-AR models too.
This topic is locked and can not be replied to.