Saturday, September 30, 2006

Rails Model View Controller + Presenter?

Ruby on Rails directs you to use Model View Controller by convention. This often results in a one to many relationship between Controllers and Views. A common controller could contain the following code.
class StandardsController < ApplicationController

def list
@standards = Standard.find(:all)
end

def results
standards = Standard.find(:all)
@total_standards = standards.size
@standards_for_hr = standards.select { |standard| standard.department == 'hr }
@standard_categories = standards.collect { |standard| standard.category }.uniq
...
end

end
This controller implementation works fine; however, the results method can become a bear to test. To solve this issue my team began inserting another layer: a Presenter. The main responsibility of a Presenter is to expose data for the view to consume. After introducing the presenter the controller becomes much slimmer.
class StandardsController < ApplicationController

def list
@standards = Standard.find(:all)
end

def results
@presenter = Standard::ResultsPresenter.new()
end

end
The Standard::ResultsPresenter class handles aggregating all the required data.
class Standard::ResultsPresenter

def total_standards
standards.size
end

def standards_for_hr
standards.select { |standard| standard.department == 'hr }
end

def standard_categories
standards.collect { |standard| standard.category }.uniq
end

def standards
Standard.find(:all)
end

end
After introducing this abstraction the Presenter can be tested in isolation.
class Standard::ResultsPresenterTest < Test::Unit::TestCase

def test_total_standards
Standard.expects(:find).with(:all).returns([1,2,3])
assert_equal 3, Standard::ResultsPresenter.total_standards
end

def test_standards_for_hr
standard_stubs = [stub(:department=>'hr'), stub(:department=>'other')]
Standard.expects(:find).with(:all).returns(standard_stubs)
assert_equal 1, Standard::ResultsPresenter.standards_for_hr.size
end

def test_standard_categories
standard_stubs = [stub(:catagory=>'Ruby'), stub(:catagor=>'Ruby')]
Standard.expects(:find).with(:all).returns(standard_stubs)
assert_equal ['Ruby'], Standard::ResultsPresenter.standard_categories
end

end
We like this style because it's much cleaner than the previously required code that lived in the controller tests. However, there is overhead involved in generating this additional layer. Because of the overhead we generally don't add a presenter until we notice that testing has become painful in the controller test file.

19 comments:

  1. I think I'm a little confused.

    Couldn't you just add some singleton methods to the standards object returned by the find all call?

    Or does that have a performance overhead or violation of some testing constraint that I'm not aware of?

    Definitley good to reduce the complexity of the tests though.

    ReplyDelete
  2. Dan,

    Sorry, I'm just not quite sure what you mean. Can you post an example?

    ReplyDelete
  3. Alright, starting from StandardsController#results


    def results
    standards = Standards.find(:all)
    class << standards
    def total
    self.size
    end
    def for_department(department)
    self.select{|standard| standard.department == department}
    end
    def categories
    self.collect{|standard| standard.category.uniq
    end
    @total_standards = standards.total
    @standards_for_hr = standards.for_department("hr")
    @standards_categories = standards.categories
    return standards
    end
    end



    def test_total_standards
    Standard.expects(:find).with(:all).returns([1,2,3])
    assert_equal 3, StandardsController.results.total end

    Sorry about the formatting.

    Again, I'm not sure if this is particularly dirty in some way or I've forgotten something important.

    Also, maybe this is too clever for its own good.

    ReplyDelete
  4. It looks like a cool idea; however, I think it would suffer from the same issues that simply using instance variables does: it is more complicated to test behavior when it lives in controllers.

    Of course, I could be over-complicating the issue to over-simplify the test. =)

    ReplyDelete
  5. Ehh, I guess you're right. Looking more closely, all I was doing was shifting it around so the Presenter was a singleton class instead of explicitly defined.

    I think the only benefit is that you wouldn't be calling any methods on objects sent to the view.

    ReplyDelete
  6. I've handled similar issues by making my model add singleton methods to the array returned by model#find.

    ReplyDelete
  7. Why don't things like standards_for_hr and standard_catagories belong in the standard model? Be weary of taking code out of the model, as you may find yourself in another controller wanting to do the same thing. Besides, since the code clearly manipulates or extracts info from the model, to me there is no reason why it shouldn't be a model method.

    ReplyDelete
  8. @yan, For this example you could simply move the behavior to the model. However, a better example would have been if the necessary data is spread across various models. You bring up a good point though, I always try to push behavior on to the model when possible.

    I like my controllers thin. Using a presenter gives you this option when moving things to models doesn't make sense.

    ReplyDelete
  9. If you're familiar with the Struts framework, this looks a lot like its ActionForms. I like the idea.

    ReplyDelete
  10. I took a little different approach with formatting stuff in and out of the models... This was extracted out of a large webapp where we had to deal with many many fields that needed to strip formatting for data getting put into the model and then format using the same code for the data coming out of the model and into the views (and other stuff)...


    Anyway, just my 2c ;)

    ReplyDelete
  11. Sorry to be a pain, but it's "category", not "catagory."

    ReplyDelete
  12. I'm with yan, in the example you gave I would have moved the code to the model.

    "However, a better example would have been if the necessary data is spread across various models."

    I still don't see why it wouldn't work in the model. I do this often for the code that runs soulcast.com (a blogging/community site)
    Something like this.

    class Tag < ActiveRecord::Base
    def some_method
    # aggregate post data related to this tag
    end
    end

    My controllers are clean like the one in your example and I can easily test the data aggregation/manipulation in my unit tests. Why would this be a bad thing?

    ReplyDelete
  13. The classic example for using a Presenter is a summary page. For example, if you buy books from Amazon, the final page has information about each book that you bought, tax information, shipping information, billing information, user account information, etc. In that scenario, I'd prefer to aggregate all that data in a Presenter.

    Presenters offer other opportunities for separation of concerns that models do not. For example, if you want to validate that the user entered two matching passwords or email addresses you must add 'virtual' attributes to your models. The same is true if you would like to create an acceptance attribute (to use validates_acceptance_of). You could argue that since these attributes are not persisted to the database that they should not live in the model.

    Additionally, Presenters that include the Validatable module can contain child objects who's validations can participate in the presenters 'valid?' method.

    Building software isn't near as simple as right or wrong. The presenter pattern offers an alternative for those who prefer to include less behavior in their models.

    ReplyDelete
  14. Why not use the handy shortcut

    @standard_categories = standards.collect(&:category).uniq

    instead of

    @standard_categories = standards.collect { |standard| standard.category }.uniq ?

    ReplyDelete
  15. Jay F, Thanks for the follow up. The summary page example made it clear.

    Jay D

    ReplyDelete
  16. Jay, you said: "You could argue that since these attributes are not persisted to the database that they should not live in the model."

    Ummmm. You could, but then you'd be wrong ;-)

    I'm very interested in reading more about this blend of MVP and MVC. In the mean time, though...

    An attribute of a person is age. Age is a "virtual attribute". It is derived from the difference between now and the (persisted) date of birth value. Who would argue that age does not belong in the model?

    Your matching password and email example could be considered part of the "application" layer (where controller and presenter types might live) - especially if such requirements are only useful to this one process - but it still rightly could still be considered part of the domain. It's certainly reflecting a business rule, it's just that it's a specialised part of the business process through registration time that helps satisfy trustworthiness concerns like Non-repudiation, Data Integrity, and Data Quality, and possibly also customer relationship requirements like having electronic correspondence details. Just because it's orthogonal to the core activities of an investment bank, insurance company, newspaper site, or whatever, doesn't mean it's not part of the business domain.

    Fight the anaemic model crisis, give blood now.

    ReplyDelete
  17. Anonymous9:34 AM

    Hey ya,
    I might sound out of topic here but the comments are overlapping. Can you please add more space between comments so to distinguish which is which?
    Cheers,

    ReplyDelete
  18. Oh my god - you just reinvented the "form bean" / DTO anti-pattern from J2EE :)

    Dave

    ReplyDelete
  19. I think this is especially useful when you are trying to pass data from many models to a view in one clean object.

    I do want to point out that you take a large performance hit by doing a find for each method. You could easily add an instance variable in the object to cache the standards so you only have to make one db call.

    ReplyDelete

Note: Only a member of this blog may post a comment.