Refactoring fat models, fat controllers, fat views --> Options

Ruby on Rails
1

I’m going to be giving a talk at RailsConf 2014 on this topic, and I’ll happily give credit to those who help with this discussion.

I’m organizing my thoughts on the various options for improving Rails
code via:

  1. Concerns
  2. View Helpers
  3. Draper Decorators
  4. Page Objects, POROs linking a controller method to one or more views.
  5. Service Objects, POROS linking a controller method to a complicated model
    interaction.

I’m going to take crack at how I’d explain this, and I’d like to get some
constructive feedback.

Overall, my decision making of which approach is guided by the following
principles:

  1. DRY is always right. If the code is duplicated, it’s worth figuring out a way
    to eliminate the duplication. Code duplication usually counts as at least 2
    or lines. A single line of duplicated code does not always justify
    de-duplication. 2 lines probably. 3 lines definitely.
  2. Simplicity is better than complexity, and I’m not going to create an
    independent class or module unless it contains at least 3 methods.
  3. Easier to test is always a bonus, but it’s not the first reason I refactor.
  4. What’s going to be easiest place for me to find the relevant code later, so
    that I can reuse it? It’s usually easier to look in a well named concerns or
    a Draper decorator rather than some PORO.

Concerns

Applicable Situation

A giant model file with corresponding giant spec file.

Solution

The giant model can safely and easily be broken down into logical chunks (concerns), each with a
correspondingly smaller spec file per concern.

Advantages

  1. Ease and safety of refactoring. Concerns are a great first refactoring step
    because using concerns involves simply moving the methods and tests into
    separate files. And code accessing those methods does not need to change.
  2. A core part of Rails 4, so one can expect familiarity among Rails developers.
  3. Simplicity. It’s just a code organization that makes it easier to navigate to
    the right source and test file.
  4. Can DRY up code when a concern is shared among multiple models.

Disadvantages

  1. The model object does not become more cohesive. The code is just better
    organized. In other words, there’s no real changes to the API of the model.
  2. For a complex operation involving many different methods, and possibly
    multiple models, a Service Object, aka PORO, better groups the logic.
  3. If the code view-centric, and relies on helpers, then a Decorator is a better
    choice.

View Helpers

Applicable Situation

  1. Duplicated code among multiple views, such as used to create some html.
  2. Typically non-model-specific code, although Rails by default will generate
    a view helper per model. Consider using a decorator if the view code is model
    specific.
  3. If you’ve got too much code in a view used to create some complicated HTML,
    and maybe that code is duplicated among multiple views, then a view helper
    can be the perfect answer.

Solution

Create a view helper method, either in the ApplicationHelper if the method applies
for many models, or in a specific view helper matching up to a model. View
helpers are simply modules that get mixed into the context of the view, and they
have access to any of the methods available in the view, such as request,
params, url helpers, etc. These are good for very simple amounts of code.

Advantages

  1. Familiar Rails technique for code organization.
  2. Simple, effective.
  3. Effective for view code that is orthogonal to any given model (can’t use
    Draper for that).

Disadvantages

  1. If you find that you have to pass the model object into each of your view
    helper methods, then that suggests that a model decorator is a better place
    for the code.
  2. If you find that you have several methods closely linked together or a very
    long method, then that suggests using some sort of PORO.
  3. Awkward to use view helper methods outside of views. So if you want the
    methods available in controllers, models, etc., then a decorator typically
    works better.

Draper Decorator

Applicable Situations

  1. Code in model that really only applies to views and presentation.
  2. Calculations done in view on model object.
  3. Code in view helper that is more closely aligned with the model.
  4. Code is that seems relatively general as opposed to clearly useful in only
    one view.

Solution

Create a Draper decorator for the model. Move the code from the view or model
into the decorator. Very similar to a concern except:

  1. Decorator methods should be presentation/view specific.
  2. Decorators lack the “Concern” support for “included”, such as has_many, and
    scope, as well as for static methods.
  3. If method feels relatively generic for views, but specific for a given model,
    then the decorator is a good place to put the method.

Advantages

  1. Relatively simple way to add functionality to the model that only applies to the
    connection of the model to the view.
  2. Can separate view specific code from the model.
  3. Methods added to the model via the decorator have access to both view helper
    methods as well as the model.
  4. It’s super easy to automatically convert the retrieved model object into it’s
    decorated instance, either manually or automatically, at the controller
    layer.
  5. Simple to find the decorator methods, such as using for verification in
    feature specs.

Disadvantages

  1. Shares similar problems with concerns in that the model object is still
    getting fatter.
  2. If there’s multiple methods needed for some complex logic, that should be
    broken into it’s own PORO.
  3. The advantage of ease of adding more methods can also be a disadvantage in
    that the Decorator can turn into a junk-drawer of loosely coupled methods.

Page Object: Complicated controller setup for a view in a PORO

Applicable Situation

  1. Controller is initializing multiple instance variables for the view.
  2. Controller has several private methods supporting the public method
    associated with the action.
  3. View has lots of inline Ruby for calculating instance variables
  4. Logic is specific to one controller method and one view, or maybe a couple
    different response types for the same controller method.
  5. If you find same code in multiple Page Objects for same model, then see if
    you can move to a decorator. A module is another option for DRY’ing up
    duplication.

Solution

  1. Controller instantiates one PORO, called the PageObject, that is used by the
    view.

  2. Inline ruby code from view and the controller is moved to the PageObject.

  3. Possibly include this snippet of code so that view instance methods are
    available:

    include Draper::ViewHelpers

Advantages

  1. Clearly bundles the code around the work the controller is doing and what the
    view needs.
  2. Simplifies code at the controller and view layers.
  3. Provides the possibility of unit testing the Page Object.

Disadvantages

There might be simpler techniques than creating another class and object, such
as putting some logic in the Draper decorator. A few lines of inline ruby code
may work better than moving this code to a separate class. A PORO has to carry
it’s own weight to justify it’s existence as a class.

Service Objects

This is a variation of the Page Object technique, with more of an emphasis on
complex model interactions. A Service Object can return a Page Object for
consumption by the view, or the Service Object can serve as the Page Object for
the view.

Applicable Situation

  1. Some action involving the interaction between several models, such as a customer
    creating an order and charging the purchase.
  2. Long complicated methods, and groups of methods on the model or controller
    suggest the use of a Service Object.

Solution

  1. Create a class whose initializer takes instances of the relevant objects or
    controller parameters.
  2. Move all applicable methods into this class from the controller and models.
  3. Use an instance of ControllerResponse (see below) to handle responses from Ajax requests.
    Use a PageObject to handle the response for views, or simply have the views
    consume the Service Object.

Advantages

  1. Clearly bundles code around a specific, complex action.
  2. Promotes the creation of smaller, more clear methods when all methods in a
    small class are around a single purpose.
  3. Allows better unit testing of methods on this ServiceObject.

Disadvantages

  1. It can be overkill for a very simple action. In other words, having a
    separate class has to pull it’s own weight. If the action is reasonably
    simple, and only involves one model class, I’m much more likely to simply
    move the functionality out of the view/controller to a concern or decorator.

ControllerResponse

This class handles the communication from ServiceObjects back to the Controller.

 # Return value object from Service Objects and the Models back to the controller.
 # Rather than throwing an exception from the Service Objects, this class will encapsulate any
 # error status to send back to the client.
class ControllerResponse
  attr_reader :flash_message
  attr_reader :flash_type
  attr_reader :http_status_code
  attr_reader :data
  attr_reader :redirect_path
  attr_reader :session_data

  # http_status can be either symbol or number
  # Pass in flash_message as a html_safe string if you do not want it escaped.
  def initialize(flash_type: :notice,
    flash_now: false,
    flash_message: "",
    http_status: :ok,
    data: {},
    redirect_path: nil,
    session_data: {})
    @flash_type       = flash_type
    @flash_now        = flash_now
    @flash_message    = flash_message
    @http_status_code = Rack::Utils::status_code(http_status)
    @data             = data
    @redirect_path    = redirect_path
    @session_data     = session_data
  end

  def self.symbol_to_status_code(symbol)
    Rack::Utils::SYMBOL_TO_STATUS_CODE[symbol]
  end

  # Currently supports applying the:
  # 1. session_data
  # 2. flash, flash_type, and flash_now
  # 3. redirect_path
  # Does not apply the http_status_code, nor the data
  def apply(controller)
    @session_data.each { |k, v| controller.session[k] = v }
    if @flash_message.present?
      if @flash_now
        # NOTE: There is no need to escape, as Rails will do that unless the flash_message is html_safe
        controller.flash.now[@flash_type] = @flash_message
      else
        # NOTE: There is no need to escape, as Rails will do that unless the flash_message is html_safe
        controller.flash[@flash_type] = @flash_message
      end
    end
    controller.redirect_to @redirect_path if @redirect_path.present?
  end

  def set_flash(flash)
    # NOTE: There is no need to escape, as Rails will do that unless the flash_message is html_safe
    flash[flash_type] = flash_message if flash_message.present?
  end

  # Returns the html escaped version of the flash.
  # This method should be used when setting value in json.
  # In the case of setting the flash on the controller, there is no need to escape, as Rails will do
  # that unless the flash_message is html_safe
  def flash_message_escaped
    if @flash_message.html_safe?
      @flash_message
    else
      CGI::escapeHTML(@flash_message)
    end
  end
end

Refactorings

Refactoring Steps to Concern or Decorator

Large controller method

Large method on controller does not allow simple stubbing, nor simple unit
testing.

  1. Break out method lines into separate private method.

  2. Move method to a concern or decorator for the given model.

  3. Remove parameter of the model instance.

  4. Assign the model instance var name to equal self.

    my_model = self

  5. Inline variable my_model.

  6. Remove self. where it improves readability.

  7. Break up long method into several smaller private methods.

3 Likes
2

I have been trying very hard to avoid situations that lead to the need to create concerns (when those concerns are not shared functionality related to CRUD operations). Whenever I do something with a model that isn’t fairly tightly tied to pulling data out of just that DB row or saving data to just that DB row, I try to build a PORO that implements that behavior rather than cramming more behavior into the model.

I (perhaps erroneously) refer to most of these POROs as “service objects” or “query objects”. Usually the constructor takes a model as one of (sometimes its only) argument. The overhead from having lots of small objects that are easy to test and have one simple, very understandable responsibility is nothing compared to the maintenance hell generated by having junk-drawer model classes.

Over the past year I’ve come to understand that if the tests are hard to write, there’s (almost always) something wrong with your design.

1 Like
3

I would suggest adding something like Corey Haines’ quote below to the disadvantages of Concerns:

Its implementation is actual hiding a dependency management system. This hides design issues that are highlighted when you are more explicit.

Source

4

I plan to spend some more time looking this over and providing some additional thoughts, but one that stuck out immediately to me was your very first point: “DRY is always right.”. While I mostly (nearly always) agree with that sentiment, this tweet got me thinking about it recently https://twitter.com/BonzoESC/status/442003113910603776/photo/1

5

Good talk idea. I’m interested to hear it and will probably be there!

But isn’t a class with a single, well-factored method simpler than a class with 3 well-factored methods? In all seriousness, I’m perfectly happy to have a class that has 1 method. I’m also perfectly happy to have a class that has 50, provided those 50 are actually part of a the class’ single responsibility and don’t represent drift. I think a high method count is a smell (not necessarily a problem) but don’t think a low method count is a smell. I’m curious to hear why you decided on 3 methods being the magic number.

If needed, I could provide some examples where I feel an object extracted with a single method lead to cleaner code.

But if you put this another way: difficulties in testing are indicative of a problem I want to cosnider refactoring for. The tests are just the first users of my code - if it’s difficult for the tests to use my code then it’s likely going to be difficult for the application (or other applications if this is a library) to use it as well.

There may well be a point where something is already easy to test and further improvements for testing sake are possible but not a good return on your investment of time, but that’s not actually what I often hear people discuss. In general we talk about the smell of pain in your tests. Not the possibility of something being simpler to test. Be careful that you’re not setting up a strawman argument here…

If the model can be broken into logical chunks, then I wonder if it can be broken into logical classes instead. I think concerns – particularly if not shared – are more a means of addressing a perceived problem (the size of a file) rather than the actual problem (the number of jobs the class in that file has).

I think the exercise of deciding what concerns to extract may be a good first step towards turning those into classes though, as you elude to, but don’t outright suggest as a possible step.

Just a small note here that Rails creates a single helper per controller not per-model.

Is it important or interesting that these use Draper? I think you could discuss decorators and presenters in general here. Much of what you’re discsussing doesn’t have much to do with Draper. The simplest decorator is often the standard library’s own SimpleDelegator. I think it’s fine to show examples of extracted objects that use draper, but this is an implementation detail, really.

Is this a common term for this type of object? I’ve only ever heard the term PageObject in the context of testing. See Martin Fowler’s reference.

I’ve heard these referred to as view models or facades, though so many of these terms are overloaded it’s hard to choose the most applicable sometimes. I think you’ve got the important distinction (at least in my mind) though - it’s an object that exists solely to glue together others for the purposes of a view. It’s different from a decorator in that it does not decorate an instance but rather provides the API you want in your view.

Agree with most of this, but I’m again intrigued by the notion that class must justify it’s existence (pull its own weight as you say here). Service Objects in particular often have a single method, in my experience. They exist to do a single job.

I’d love to see some examples of this. I’m not familiar with this type of object.

Good luck with the talk. You certainly have a good deal of content you’re trying to cover here and I think it’s well-timed with what the rails community has been talking about.

1 Like
6

@derekprior, many thanks for the insightful comments.

In terms of classes with single, short methods (try for 5 lines or less), I’d say that it’s just too much ceremony to have a separate class. 2 is on the margin. I’d say 3 definitely warrant a separate class. I’ve been through way too much ceremony with Java!

Yes, I’d love to see this!

I’ve chosen the name Presenter over PageObject. It looks and sounds better. I’d rather say and see the UserCreatePresenter for the User Controller’s create method, rather than the UserCreateFascade.

My reason for personally using Draper and for presenting it in my talk is practicality. I like Draper because it facilitates the addition of a new Decorator for a given model. Yes, I could do some of the same with SimpleDelegator, but like much of Rails, this is a personal, somewhat aesthetic preference. For a 30 minute talk, I want to present my concrete techniques, rather than philosophical abstractions.

Thanks again, and thanks in advance to anybody that participates in this discussion.

7

Hi, this is an awesome idea for a talk. As someone who’s in the junior-intermediate level, I’m enamoured by the different extractions for the AR models. BTW, if you can put tests for each of the extractions, that would be really be awesome, since the CodeClimate blog post doesn’t have tests, so we know what we should be doing

8

@daryllxd I’m actually giving a talk about this at my next local Rails’ group, particularly directed at junior-intermediates (which I where I place myself too).

My plan is to talk about my journey towards understanding where you might apply design patterns in your code, which invariably comes from figuring out code smells (such as bloated classes with multiple responsibilities).

I think from a beginner’s perspective, you’re lead to believe that code_with_tests == good_code, however I’ve realized that’s not always the case and there’s a reason your code is hard to maintain or tests are hard to write.

The talk is going to be something along the lines of

  1. My experience trying to figure out how to add stubs and mocks in my tests and creating these awful false positive tests, to;

  2. Learning about the Single Responsibility Principle (SRP) and the SOLID principles in general (but not quite understanding them), and then;

  3. Reading/listening to Sandi Metz talk about PORO’s and her ‘rules’, which helped me understand my testing problems (point 1) and how Dependency Injection is the answer, leading me to finally;

  4. Consider applying design patterns to help fit into those rules and using design patterns as a tool to achieve SRP and to help communicate with others about how/what each class does.

3 & 4 clicked for me after reading the Extract class discussion on the forum and seeing @derekprior’s /app directory in a project.

Should really do a blog post as well, which I can share on the forum if people are interested.

9

@ralphwintle sounds good!