I'm Thinking of Putting View Logic into a Model

As I am clearly crazy, but hear me out. Say there's a Car object that uses single table inheritance and all the objects that descended from it are routed through the car method in the CarController. The CarController looks like this:




1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
class CarController < ApplicationController
def car
layout = nil
@scripts = []
@stylesheets = []
@car = Car.find(params[:id])
if @car.is_a?(Toyota)
layout = 'two_column'
@stylesheets << 'two_column_layout.css'
@stylesheets << 'two_column_theme.css'
elsif @car.is_a?(Hyundai)
layout = 'three_column'
@stylesheets << 'three_column_layout.css'
@stylesheets << 'three_column_theme.css'
@stylesheets << 'hyundai_theme.css'
@scripts << 'discount.js'
elsif @car.is_a?(Ford)
layout = 'ford'
@stylesheets << 'ford_layout.css'
@stylesheets << 'ford_theme.css'
@scripts << 'discount.js'
elsif @car.is_a?(Saturn)
layout = 'two_column'
@stylesheets << 'two_column_layout.css'
@stylesheets << 'two_column_theme.css'
@scripts << 'poll.js'
#
# 100 more lines like above
#
end
render :action => find_view, :layout => layout
end
end
So I'm not very happy about this and ultimately I'd like to refactor the app to use different controllers for different objects (crazy huh?), but that is a major undertaking as the whole app assumes this architecture. It occurred to me the other day that there's an in-between step. I could do something like this:




1
2
3
4
5
6
7
8
9
10
11
class Car < ActiveRecord::Base
def layout
'two_column'
end
def stylesheets
['two_column_layout.css', 'two_column_theme.css']
end
def scripts
[]
end
end
And then the Ford class would look like this:




1
2
3
4
5
6
7
8
9
10
11
class Ford < Car
def layout
'ford'
end
def stylesheets
['ford_layout.css', 'ford_theme.css']
end
def scripts
['discount.js']
end
end
So the CarController could then be shortened to this:




1
2
3
4
5
6
7
8
9
class CarController < ApplicationController
def car
@car = Car.find(params[:id])
@scripts = @car.scripts
@stylesheets = @car.stylesheets

render :action => find_view, :layout => @car.layout
end
end
Any model that needs to overwrite the default layout, scripts, or stylesheets can.

So should I take this step and put view information in the models, or should I just hold out for the big refactor into separate controllers? Keep in mind the big refactor is so big that it may never happen.

Comments

Mr. Neighborly said…
If you have classes for each car type like that, then absolutely.
Anonymous said…
What about using the Presenter Pattern?

http://blog.jayfields.com/2007/03/rails-presenter-pattern.html

Justin
David Chelimsky said…
Not sure of the best way to do this w/ javascript, but for stylesheets, why not just have stylesheets that use import statements. So you could have toyota.css which imports two_column_layout.css and two_column_theme.css.

Layouts could be named for each make as well, each including the appropriate partials.

Now the view logic is not only not in the model, it's not even in the controller :)
Jared Pace said…
This could easily be refactored to fit into the MVC pattern better.

I tried to type it here but couldn't figure out how to get Blogger to accept erb tags.

Here is the sample code.
Wes said…
I most of this belongs in a helper actually.
Ex: in yer view

= stylesheets_for @car
= scripts_for @car

And your code can be shortened by having an conditional for every scenario, rather than every type.

ex:

case @car
when Ford
return ''
when Toyota, Saturn
return 'two_column'
when Hyundai
return 'three_column'
else
raise "#{car.class} doesn't have a layout specified"
end
Ikai Lan said…
What about using the class name to determine the stylesheet to load? Rather than specify the stylesheets, how about creating a:

car.js
ford.js

car_layout.css
ford_layout.css

In your view you could use:

stylesheet_link_tag @car.class.to_s

If this is not possible or would create too much complication, it still doesn't make sense to define the stylesheets and JavaScrips as instance methods. I would probably opt for a module that lets you define class-level attributes. Each car instance does not need its own stylesheet, but I would assume that each type of car (car class) has its own set of view components. If I had to do this, I would opt for something like the following:

class Ford
stylesheet 'ford'
layout 'ford_two_column'
end
pragmatig said…
id use a helper method which reads data form layouts.yml
Anonymous said…
I think your original is much better than putting view code in the Model, which is lazy.

There are many better ways of doing it, use your imagination, for example you could create a hash with the required objects for each model.

foo[model.class][:javascript]

I don't mean to sound harsh, but if you really do need to put view code in your model it means your design is broken.
Maxim Kulkin said…
That looks digusting!

I would just create a partial (or view) for each car subclass with fallback to some default (e.g. two_columns). In controller you just check if custom view exists or fallback to default one.
stefano said…
Or you could use YAML files to hold the various configurations, something like this...

config/cars/toyota.yml
config/cars/ford.yml
etc.

@car_config = YAML.load(File.read(Rails.root + "/config/cars/" + @car.class.name.underscore + ".yml"))
alex said…
how about a helper that keeps a hash of model_class => class_specific_configuration - the class_specific_configuration could either be a hash like {:stylesheets => 'toyota.css' ....}, an openstruct or maybe a small CarViewConfig class. you could even create a hierarchy of CarViewConfig classes if you want - without putting any view logic into your actual models.
nesquena said…
Why not remove it from your controller, and put it in the view where it belongs using a helper which is perfect for this situation.

# some_helper.rb
def place_stylesheets(car)
if car.is_a?(Toyota)
...
elsif car.is_a?(Ford)
...
end

end

That can concatenate the scripts and stylesheets properly right in the view where they belong. Now for the layout. That is a bit trickier. Myself, I would perhaps move that to a simple method in a helper or as a private method in the controller.
Dave Hoover said…
Seems like you could just use a big data structure and extract it to a different class. Here is a try on gist.
Jake Scruggs said…
It's interesting to me that many commenter suggest moving the big switch/if/else into a helper or presenter. Which seems to me a bit like sweeping the dirt under the rug.

While moving from switch to OO is such a classic bit of Refactoring it's hard to dismiss.

It is a violation of the MVC layers, but what bad thing happens if I put stylesheet, layout, and javascript file information in the model? Well if the name of any of those files changes, I guess I'll have to change the model. Which doesn't seem like it will happen very often.

However, it does set a bad precedent -- future coders may start jamming more and more view stuff into the model and that would be bad.

When is it OK to break the rules? Or, to put it another way, if I have to choose between switching on class or breaking MVC which is more egregious? I guess it depends on the team who will be maintaining the code.
Sudhindra Rao said…
Hi Jake,
What you are saying strikes a chord. If we look at your code it looks like the presentation very much depends on the model or the type of the model. So it totally makes sense to use polymorphism to figure this out and replace those ugly if/else/switch statements. Once we have crossed that hurdle it looks like a view or a presenter class becomes your first refactoring as you and some commenters suggest. I have a weird feeling about putting all this in helpers, since helper itself does not sound very OO.
Once we have this first refactoring in place we have done some damage control. But what you are referring to(as pushing under the rug) needs a deeper thought and look at the code. At this point we should really start asking the question - how much is the view dependent on the type of the model? Well if we take a closer look - we find that the model defines the way it is going to look. So the behavior certainly belongs into the model.
But does this relationship really warrant extending the ActiveRecord class? and does it justify putting this code into the model(Statically)? Maybe not. What could make it better is a mixin? Based on the model type we should include the mixin for the appropriate layout like so
---- Code begins ----

module Ford
def layout
"ford"
end

def stylesheets
['ford_layout.css', 'ford_theme.css']
end

def scripts
"discount.js"
end
end

class Car < ActiveRecord::Base
.
.
.

def present
extend self.type.to_s.Constantize
end
.
.
.
end

class CarController < ApplicationController
@car = Car.find(params[:id])
@car.present
render :action => find_view, :layout => @car.layout
end
---- Code ends ------
Using mixins really makes this code look like its meant to be in ruby.

-Sudhindra
Jake Scruggs said…
Which is more Terrible, the violation of MVC or the Switch on Object, ActsAsFlinn?
Matth said…
Thing is, there's a difference between "class which persists its data in a database" and "class which forms part of the model layer of an application".

The use of ActiveRecord tends to make you think of these two as one and the same, but they're not. Sometimes the UI layer needs to persist UI settings in a database. Shock f---ing horror. MVC cries in pain.

If there's a genuine 'model level' concept which you model and persist and use to infer the relevant UI state, then great.

But sometimes, you'll just need to persist something blatantly UI-level, like 'x and y coordinates of UI widget foo' in the database. And that's fine. It would be nicer if you were using a component-based web framework though, which doesn't force you to put anything database-persisted in a 'model' layer.
Sergio said…
I get scared anytime I see a million elsif statements. Moving it to a helper or a view doesn't make it go away. I don't think its that big of a deal to stick this kind of info in the model. You should never subscribe to rules so hard that it makes you write nasty code.
Xavier Shay said…
My response:
http://rhnh.net/2008/08/27/no-view-logic-in-models

summary: you can use a better data structure to solve your problem
paytonrules said…
What if your models get used in multiple applications - which happens to us all the time?

I've thought of doing this, and have run into the problem before. You end up with methods that only apply to one rails app. Ick.

What about a decorator for that object? You isolate the view logic into its own class, and remove the switch. I don't have the sample code - but you can probably picture a CarViews object (or something with a better name) that uses method missing to delegate to the real car when it needs it.

Popular posts from this blog

Point Inside a Polygon in Ruby

What's a Good Flog Score?

SICP Wasn’t Written for You