fat-code-refactoring-techniques icon indicating copy to clipboard operation
fat-code-refactoring-techniques copied to clipboard

Earlier Draft: Concerns

Open justin808 opened this issue 11 years ago • 5 comments

Rails Concerns

  • http://api.rubyonrails.org/classes/ActiveSupport/Concern.html
  • Simplest safest refactoring
    • Just cut and paste the code
  • Default part of Rails
  • Not just for code shared across models, but super to simply divide change a large model file into logical chunks with associated tests.

Why Concerns and Not Plain Ruby

  • Simple to consistently follow the pattern
  • Concerns can include other concerns
  • Single concern includes all of:
    • instance methods
    • class methods
    • code to run on model including the concern, such as =has_many=, =scope=, etc.
  • Yes, you can do the same with include, extend, etc., but why risk the pitfalls and why not the consistency of Concerns?

Where to put concerns

  • If just one concern for model, or if shared among models, then in app/models/concerns.
  • If multiple concerns for single model, group in subdirectory under models, such as app/models/user/.

Steps

  • Create file for Concern with skeleton structure:
  module ModelName::ConcernName
    extend ActiveSupport::Concern

    included do
      # your class level code
    end

    # move your instance methods here

    module ClassMethods
      # your class methods, removing self.
    end
  end
  • Move class level code to included block.
  • Move class methods to inside of module ClassMethods.
  • Move instance methods to module.
  • Place include statement in original model class:
  include ModelName::ConcernName

Another Concern Example

  • Break out methods concerning a user's feed.
  • Now the User is broken up into:
    • user/find_methods.rb
    • user/feed_methods.rb
  • Tests similarly matched up.
  • I like the smaller files!
  • Safe, easy way to slice up huge model files.
  • Could be first step to further refactoring.

Concerns: Didn't Show You But Useful!

  • Sharing a concern in multiple models
  • Nesting concerns (concerns loading concerns)
  • Applies to controllers as well as models

Concerns Summary

Applicable Situation

  • A giant model file with corresponding giant spec file.
  • Duplicated code in multiple models.

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.

Concerns References

Review on Reviewable

justin808 avatar Mar 31 '14 18:03 justin808

Under "Disadvantages", points 2 and 3 are actually alternatives -- not really disadvantages. You might want to mention that another disadvantage is, it could be argued, that Concerns hide the complexity of the Object. Also, it is often times harder to find how a certain object has access to a method, and where that method is defined.

"PORO" stands for "Plain Old Ruby Object", and comes from the Java acronym of "POJO" (perhaps another language coined it earlier). Saying Service Object is also known as PORO is not really correct. PORO is just any plain ruby object, that doesn't depend on any libraries in itself.

gylaz avatar Apr 08 '14 17:04 gylaz

Another thing worth mentioning is trickiness involved when testing Concerns that are included into several models. Do you test each model for those included methods? Do you create a dummy model/class and test the concern once? Do you use RSpec's shared examples?

Worth to think about Concern method that rely on some state of the model. Say you have a method like this in your Concern:

def by_email
  where(email: @email).first
end

That Concern now depends on the fact that the including model must have a @email ivar (or define the method email if we go that route). That's sort of implicit API, and things like that could be missed until they blow up in production.

gylaz avatar Apr 08 '14 17:04 gylaz

I dislike concerns hugely. Now that I have that off my chest...

@gylaz I used to do shared spec on these kinds of things, but that was when I was 'speccing' instead of 'testing' as I do now.

When I need to test any mix-in, I create a test class, mix in the module, then test the behavior. `At least this was how I used to do it.

These days I just extend self on modules and test them directly.

module SomeShitHere
  extend self

  def a_function
    #noop
  end
end

it "blah blah" do
  SomeShitHere.method.must_equal nil
end

dreamr avatar Apr 13 '14 15:04 dreamr

@dreamr Thanks for the response.

  1. What's the reason for disliking concerns but OK to include modules?
  2. Please correct me, but doesn't the extend self allow you to simply call methods on the module as statics? What if the function =a_function= depends on instance values?
  3. Does the =extend self= have any purpose other than testing?

Thanks.

justin808 avatar Apr 13 '14 18:04 justin808

@justin808 I write small focused, isolated classes. I also stay away from loading dependencies I don't need.

Concerns are inviting big-balls-of-shit, which I don't like. They keep you firmly in Rails-land, and they serve no purpose except to create highly coupled junk-drawers.

A much better idea would be to extract business log out into PRO (pure ruby objects). These can be modules, classes, whatever. But they should have well-defined boundaries.

Rails in general encourages highly coupled messes, which is why they always turn into a rescue at some point if they live long enough.

Off with the rant and onto one of my favorite topics, extend self.

Extend self is essentially a module-wide module_function call. It indeed turns all the 'instance' (which modules don't actually have) level stuff into class level stuff.

As I write as much stateless, immutable objects and functions as possible in my code, I end up using extend self a lot. I also create a lot more modules than classes and methods commonly delegate to a stand-alone module, ie:

module CarDiagnosticRecipeRunner
  def run(recipe)
    ...
  end
  alias_method :call, :run
end

class Car
  def run_diagnostic(recipe)
    CarDiagnosticRecipeRunner.(recipe, self)
  end
end

Whereas I think you tend to use modules like so:

module CarDiagnostics
  def run_diagnostic(recipe)
    ...
  end
end

class Car
  include CarDiagnostics
end

dreamr avatar Apr 13 '14 19:04 dreamr