Remember your MVCs
Posted by Tammer Saleh
Oct 01
From novice to expert, I’m sure every Rails developer believes in MVC. But all to often I’ve seen programmers cut right through the Model/View/Controller boundaries when implementing new features – especially where precedent hasn’t yet been set.
I can understand where the urge comes from, as there are times when the boundaries are fuzzy. Rails has the validates_confirmation_of for entering a password and a password confirmation, while a good argument could be made that this is logic that should never be in the model. If the record was created by a 3rd party application, then it shouldn’t have to specify the password twice in the XML just because the model is usually used by humans. And the writers of the macro acknowledge this by not having it kicking in if the field is nil.
A friend of mine recently proposed the following solution for creating a Combobox (a dropdown with a customizable ‘other’ field). He didn’t like the complexity caused by using a before filter on the controller, couldn’t rely upon javascript in the form, and wanted to keep his controllers virtually untouched. Going on the “skinny controllers, fat models” school of thought, he added the following in his model:
1 2 3 4 5 6 7 8 9 10 11 12 13 |
attr_accessor :other_primary_source_of_funding attr_accessor :other_heard_through before_save :replace_with_others def replace_with_others unless other_primary_source_of_funding.blank? self.primary_source_of_funding = other_primary_source_of_funding end unless other_heard_through.blank? self.heard_through = other_heard_through end end |
This code wins points in being concise and understandable, but is also a big ‘ol MVC violation. The “Right” solution is to grab the value in the controller:
1 2 3 4 5 6 7 8 9 10 11 |
before_filter :convert_combobox_values, :only => [:create, :update] def convert_combobox_values if params[:object][:primary_source_of_funding] == "Other" params[:object][:primary_source_of_funding] = params[:object].delete(:other_primary_source_of_funding) end if params[:object][:heard_through] == "Other" params[:object][:heard_through] = params[:object].delete(:other_heard_through) end end |
Here’s the question you should be asking yourself when you think you’re treading the MVC line: “Would the model need this method if it wasn’t being accessed through HTML?” If you were using this model in a desktop application, that question for the example above would be No.
Comments on this post
Oct 01
Gabe da Silveira said,
Excellent example of a subtle MVC architectural decision that goes against the skinny C/fat M rule of thumb.
Compared to some MVC violations you see (global variables!?) I think this is a pretty minor distinction to make, nevertheless your second form is clearly superior.
Oct 01
Eric Torrey said,
With a little bit of mocha magic we can get that method tested without cluttering our post/put requests to create/update. Here is the ‘successful’ test:
Oct 01
Tammer Saleh said,
Torrey: Yeah, that would be a great test to add for that code. And a very good use of mocking.
Gabe: I agree that the violation is small, but I’ve seen code ruined by many small mistakes much more often than by a few large ones.
Oct 01
sandofsky said,
I think this falls into validation, but your model isn’t fleshed out.
It sounds like the project requirements are:
“If the user inputs an ‘other bank’, use that. Otherwise, use the dropdown.”
But are you storing it in one column? Why? Maybe I don’t get the requirements.
Oct 01
Chad Pytel said,
We want to store the value in one column because we know that the requirements are to be able to search the attribute and we don’t want to have to search the attribute column and the attribute_other column every time.
Oct 01
sandofsky said,
That’s a reasonable choice, though I’d rather write extra SQL hidden behind a finder than deal with stuff like this. Plus, if the user gave me the value from an enumeration, i’d rather keep it a definite enumerated value than assume it a plain old string.
But that’s all beside the point. I just feel dirty calling params:modelname
Oct 02
Piers Cawley said,
But the controller ‘solution’ is horrible. If sticking the behaviour directly in the model bothers you (and I remain to be convinced that it’s a problem), then introduce an adaptor which will translate into
paramsrepresentation into calls to the ‘real’ accessor methods. Then your controller method looks something like:OtherDecoratoris a horrible name, I know, but I think the pattern’s good. It wouldn’t be rocket science to start adding other sorts of decorators to handle other places where the form representation has impedance matching problems with the model’s interface, but that’s an exercise for the interested reader I think.Oct 02
Tammer Saleh said,
Piers: So the decorator would handle checking for the :other_foo fields and setting the :foo attributes instead? That would work, but it feels like it adds a lot of code to handle a pretty simple problem. You’d add a file, class, and before_filter in the controller just to handle comboboxes.
And the before_filter solution I posted could also easily be generalized and pluginized ($20 says it already is) to lessen the controller mess.
Taking care of those fields in the model is definitely a poor idea – both practically and philosophically. If your designer decided to convert another field into a combobox, you’d have to modify your model to accommodate that.
Oct 02
Piers Cawley said,
Where’s the before_filter?
A generalized and pluginized version of the controller based solution will end up needing more lines of code and it will certainly need more files than introducing a decorator.
Which isn’t to say that you couldn’t wrap the decorator up in such a way that you could declare your decorations in the controller without explicitly referencing the decorator class, but in doing that, you get to separate the code that’s responsible for munging the params hash into something that fits the model from the code that generates the before filters in the controller. Which is nice, because the two things are operating on entirely different levels of abstraction.
Oct 03
Jay Levitt said,
I think part of the problem is that – get ready – not everything fits into MVC, especially as Rails implements it today.
Where does display logic that’s model-specific but not view-specific go? (e.g. an Address that knows how to display itself, because an address has a standard format. Or a phone number that stores itself as digits-only, but wants to display everywhere with hyphens.) In general, anything that doesn’t look the same when displayed, serialized, and calculated feels like it straddles MVC.
Some folks are talking about “presenters” now. Others are talking about “Model/Controller/Conductor.” There’s talk on rails-core of making a new Column object that would let you create data types other than the primitives types, and let them determine how to store themselves. Whatever the solution, this definitely feels like a pain point.
Oct 04
Piers Cawley said,
There’s a Smalltalk pattern called ValueModel that tries to address some of these issues. Kent Beck wrote about it quite a bit in his Guide to Better Smalltalk that might be useful in this context. Unless I’ve completely misunderstood the point of the pattern, which is all too possible.
Oct 04
Tammer Saleh said,
Definitely, MVC is a great solution 90% of the time, but it isn’t the right way to go for every project. I think seaside is making a good run at proving that. But consistency trumps this – using a non-MVC approach in an MVC framework is just bad practice.
That being said, you haven’t shown an example that doesn’t work just fine with the Rails MVC pattern. Helpers for formatting strings are common, and are much better than asking the model to do it. Should the model be concerned with what format I want to display the date in? What if my designer decides that one page should be hip and display phone numbers as 123.456.7890? Should I have to change my model to include
human_phoneandhipster_phone?Putting this kind of logic in the model is a huge step backwards.
The seaside argument is that if you do this consistently, and separate each chunk of information by symantic tags, then your designer can modify the presentation via CSS. I personally believe that this is naive for the reasons above. The designer would have to be a JS and CSS wizard to make this work, and I’d hate to see the outcome.
Oct 04
Tammer Saleh said,
sandofsky:
I read this as “I’d rather have 30 pains in the ass than 1”. Why would you want to complicate something you’ll be doing often just to save complication on something you’ll be doing once?
Sorry, comments are closed for this article.
© 2000 - 2009 by thoughtbot, inc.
written by a bushel of tiny robots
Come “ride the toad” on Hoptoad, the app error app.
Thunder Thimble: Brand monitoring for social media.
Widgetfinger: Simple content management for simple websites.
Tee-Bot, funny shirts your friends won't understand!
Umbrella Today: “It’s like totally the simplest weather report ever, Julie.”
Thoughtbot
thoughtbot is a technology consulting firm that provides web application development and design services. We focus on building modern systems, embracing good ideas and delivering elegant solutions.
Interested in learning Rails?
Sign up for our beginning or advanced training.
Archives