Remember your MVCs

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.

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:

1
2
3
4
5
6
7
8
9
10
11
12

  def test_should_convert_combobox_values_for_the_controller
    @controller.stubs(:params).returns({:team =>
                                         {:other_primary_source_of_funding => 'value',
                                           :primary_source_of_funding => "Other",
                                           :heard_through => "Other",
                                           :other_heard_through => 'value'}})
    @controller.convert_combobox_values
    assert_equal 'value', @controller.params[:team][:primary_source_of_funding]
    assert_equal 'value', @controller.params[:team][:heard_through]
    assert_nil @controller.params[:team][:other_primary_source_of_funding]
    assert_nil @controller.params[:team][:other_heard_through]
  end

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.

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.”

1
2
3
4
5

def before_save
    if self.other_bank
        self.other_bank = nil unless self.bank_selection == OTHER
    end
end

But are you storing it in one column? Why? Maybe I don’t get the requirements.

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.

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

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 params representation into calls to the ‘real’ accessor methods. Then your controller method looks something like:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16

def update
  @model = find_resource
  if @model.update_attributes(params[:model]) 
    # success
  else
    # failure
  end
end

def find_resource
  raw_model = Model.find(params[:id])
  OtherDecorator.new \
    raw_model, 
    :decorated_fields => [:source_of_funding, 
                         :heard_through]
end

OtherDecorator is 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.

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.

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.

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.

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.

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_phone and hipster_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.

sandofsky:

That’s a reasonable choice, though I’d rather write extra SQL hidden behind a finder than deal with stuff like this

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?