Clean up fat rails helpers

Today, trying to DRY up some code, I extracted some duplicate File.exists? code used by several helper methods into a private method

def template_exists?(*template) and accidently monkey patched this already existing Rails helper method. This was a pretty clear indicator of a code smell, I didn't need need any the inherited methods, yet I inherit them. Besides, what was my refactored method doing in this helper at all?

So, this helper does way too much and hence violates SRP (Single Responsibility Principle). I feel that rails helpers are inherently difficult to keep within SRP. The helper I'm looking at is a superclass helper of other helpers, counting 300+ lines itself. It is part of a very complex form using javascript to master the interac开发者_运维技巧tion flow. The methods in the fat helper are short and neat, so it's not that awful, but without a doubt, it needs separation of concerns.

How should I go forth?

1. Separate the methods into many helpers?
2. Extracting the code inside the helpers methods into classes and delegate to them? Would you scope these classes (i.e. Mydomain::TemplateFinder)?
3. Separate the logic into modules and list them as includes at the top?
4. other approaches?

As I see, no 2 is safer wrt accidential monkeypatching. Maybe a combination?

Code examples and strong opinions appreciated!

Extracting helpers methods into classes (solution n°2)

ok to address this specific way to clean up helpers 1st I dug up this old railscast :

http://railscasts.com/episodes/101-refactoring-out-helper-object

At the time it inspired me to create a little tabbing system (working in one of my apps in conjunction with a state machine) :

module WorkflowHelper

  # takes the block  
  def workflow_for(model, opts={}, &block)
    yield Workflow.new(model, opts[:match], self)
    return false
  end

  class Workflow
    def initialize(model, current_url, view)
      @view = view
      @current_url = current_url
      @model = model
      @links = []
    end

    def link_to(text, url, opts = {})
      @links << url
      url = @model.new_record? ? "" : @view.url_for(url)
      @view.raw "<li class='#{active_class(url)}'>#{@view.link_to(text, url)}</li>"
    end

  private
    def active_class(url)
      'active' if @current_url.gsub(/(edit|new)/, "") == url.gsub(/(edit|new)/, "") ||
                 ( @model.new_record? && @links.size == 1 )
    end

  end #class Workflow
end

And my views go like this :

  -workflow_for @order, :match => request.path do |w|
    = w.link_to "✎ Create/Edit an Order", [:edit, :admin, @order]
    = w.link_to "√ Decide for Approval/Price", [:approve, :admin, @order]
    = w.link_to "✉ Notify User of Approval/Price", [:email, :admin, @order]
    = w.link_to "€ Create/Edit Order's Invoice", [:edit, :admin, @order, :invoice] 

As you see it's a nice way to encapsulate the logic in a class and have only one method in the helper/view space

1. You mean seperate large helpers into smaller helpers? Why not. I don't know your code too well, but you might want to consider outsourcing large code chunks into ./lib.
2. No. ;-) This sounds awfully complex.
3. Sounds complex too. Same suggestion as in 1.: ./lib. The modules in there are auto-loaded if you access them.
4. No

My suggestion is: Hesitate from using too many custom structures. If you have large helpers, ok, might be the case. Though I wonder if there is an explanation why that whole helper code is not in the Controller. I use helpers for small and simple methods that are used inside the template. Complex (Ruby-)logic should be put into the Controller. And if you really have such a complex Javascript app, why don't you write this complex stuff in Javascript? If it really has to be called from the template, this is the way to go. And probably makes you website a bit more dynamic and flexible.

Regarding monkey patching and namespace collisions: If you have class name, method names etc. that sound common, check out if they are defined. Google, grep or rails console for them.

Make sure you understand which code belongs to

• Controller: Fill variables with stuff, perform user actions (basically the computation behind your page)

• Helper: Help doing simple stuff like creating a fancy hyperlink

  def my_awesome_hyperlink url, text "Fancy Link to #{text}" end

• ./lib: More complex stuff that is used by more than one Controller and/or also used directly by other components like Cucumber step definitions

• inside the Template as Ruby Code: Super easy to read
• inside the Template (or ./public) as Javascript code: Matter of taste. But the more dynamic your app, the more likely code belongs in here.

Ok this is a really hard question to answer. Rails kind of leads you down the path of view helpers and really doesn't give you a decent baked-in alternative when you out-grow it.

The fact that helpers are just modules that are included in to the view object doesn't really help with the separation of concerns and coupling. You need to find a way to take that logic out of modules altogether, and find it a home in its own class.

I'd start by reading up on the Presenter pattern, and try to think of how you might be able to apply it as a middle layer between the view and the model. Simplify the view as much as possible, and move the logic to the presenter or the model. Move javascript right out of the view, and instead write unobtrusive javascript in .js files that enhance the functionality of the existing javascript. It definitely does not need to be in the view, and you'll find that helps clean up a lot if stuffing js in your views is what got you in this mess.

Here's some links for reading:

http://blog.jayfields.com/2007/03/rails-presenter-pattern.html

About presenter pattern in rails. is a better way to do it?

http://blog.jayfields.com/2007/01/another-rails-presenter-example.html

http://blog.jayfields.com/2007/09/railsconf-europe-07-presenter-links.html

Don't get too caught up in the specific code examples, rather try to understand what the pattern is trying to accomplish and think how you could apply it to your specific problem. (though I really like the example in the 2nd link above; the stack overflow one).

Does that help?

It is difficult to suggest a clear solution without the code. However since helper methods all live in the same global view instance, name collision is a common problem.

@Slawosz may be a solution but no really fit for the helpers philosophy.

Personally I would suggest to use the cells gem : cells are like component for rails, except lightweight, fast, cacheable and testable.

Also to answer your specific problem they're completely isolated. When your view helpers get to complex they're definitely the solution.

(disclosure I am not the creator of this gem, just happily using it....)

# some view
= render_cell :cart, :display, :user => @current_user

# cells/cart_cell.rb
# DO whatever you like in here
class CartCell < Cell::Rails

  include SomeGenericHelper

  def display(args)
    user    = args[:user]
    @items  = user.items_in_cart

    render  # renders display.html.haml
  end
end

Also you can use a generic helper for dryness here without fearing name clash.

I would create a small library which is responsible to manipulate yours forms.Some well named classes, some inheritance, and as input you pass ie parameters, and as output you can has ie. objects for partials used in this form. Everything will be encapsulated, and it will be easy to test.

Look at AbstractController code, and then Metal to see how smart are rails designed, maybe there You will find inspiration how to solve your problem.

Your approach is correct but I would prefer 3rd point i.e. separate the logic into modules.

Modules open up lots of possibilities, particularly for sharing code among more than one class, because any number of classes can mix in the same module.

The greatest strength of modules is that they help you with program design and flexibility.

Using such kind of approach you can achieve design patterns.