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
http://blog.jayfields.com/2007/03/rails-presenter-pattern.html
Justin
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 :)
I tried to type it here but couldn't figure out how to get Blogger to accept erb tags.
Here is the sample code.
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
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
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.
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.
config/cars/toyota.yml
config/cars/ford.yml
etc.
@car_config = YAML.load(File.read(Rails.root + "/config/cars/" + @car.class.name.underscore + ".yml"))
# 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.
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.
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
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.
http://rhnh.net/2008/08/27/no-view-logic-in-models
summary: you can use a better data structure to solve your problem
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.