Topic: Refactoring on Rails: Multiple Scopes in Controller

If you hang around the refactoring crowd long enough, you may hear the term bad smells in code. Smelly code works, but the code itself is poorly implemented. If you keep coding without removing the bad smell, it will only get worse and become harder to fix in the future.

When we find this stinky code, how do we fix it? Because the code is already satisfying the requirements, refactoring is the perfect solution. Refactoring, as you may recall from the previous article, is improving the design of existing code without changing its functionality. Therefore, each smell has its own set of recommended refactorings that take care of the smelly situations.

Martin Fowler lists several bad smells in his book. By far the most popular smell is Duplicate Code. The other smells/refactorings in the book are equally generic, but we can take this concept and create Rails-specific smells and refactorings. This article will do just that by defining one smell and one refactoring to solve it.



Smell: Multiple Scopes in Controller

If a controller has a large number of actions, it may be suffering from multiple scope-osis (sorry, I couldn't resist). This problem can usually be spotted by simply scanning the method names. If you see a group of actions that seem to go together, you should consider doing the Extract Controller refactoring (see below) to move the group of actions into its own controller.

Let's take a look at an example. Here we have a controller with a list of actions:

Account Controller
index
register
update
login
logout
addresses
create_address
update_address
destroy_address

This controller obviously has more than one scope. How many do you see? I see three. One scope handles the displaying, creating, and editing of the account model. Another scope handles user login/logout. The third scope manages the account's addresses.

This makes the controller unnecessarily large which will only get worse as we add features. The four address actions should be moved into its own controller. The login/logout actions probably should too, but that's not as critical as it is only two actions.



Refactoring: Extract Controller

This refactoring involves creating a new controller from a portion of an existing controller. To summarize: a new controller is created, the desired actions are moved to this new controller along with their views, partials, helpers, etc., and the old actions are set to redirect to the new actions. If we apply this refactoring to the "addresses" scope in our Account Controller, we get this:

Account Controller
index
register
update
login
logout

Addresses Controller
index
create
update
destroy

Much cleaner. If we try to do this all at once, we are bound to break something. This is why refactorings are broken down into little steps. Here are the steps for this refactoring:

1. Generate a new, empty controller

2. Copy and paste all action methods you wish to extract from the original controller into the new controller

3. Copy the necessary view, helper and partial files to the new controller.

4. Copy any necessary filters or private methods to the new controller.

5. Copy the tests which apply to the actions over to the new controller test case and run them. This will tell you if you forgot to copy anything else.

6. Change the actions in the original controller to redirect to the matching actions in the new controller. Update the tests accordingly.

7. Remove the views, partials, helpers, filters, private methods, etc. from the original controller which belonged to the extracted methods and are no longer necessary. Constantly run the tests to make sure nothing breaks as you do this.

8. For every existing partial used by both controllers, decide which controller should "host" it (or place it in a "shared" directory) and update the views to render the partial from that location. Remove the unnecessary, duplicate partials.

9. For any duplicate helper methods that exists in both controllers, move them into the ApplicationHelper module.

10. For any duplicate private/non-action methods that exist in both controllers, move them into the ApplicationController.

11. Change the links in all the views to use this new controller.

12. If you do not need to support external links, remove the actions in the original controller that are just redirecting to the new controller.


If you can think of any other smells or refactorings, consider writing them down and creating your own Rails Refactoring Catalog. You may also want to post a tutorial here describing the refactoring.

One thing interesting to note, some refactorings can lead directly to other smells/refactorings. For example, this refactoring had you move the common code into ApplicationHelper and ApplicationController. If either of these get very large or contain many methods that are only used by a couple controllers, that would be considered a smell. It would be interesting to see what kind of refactorings can solve that problem.

Now go clean up your controllers. smile

Last edited by ryanb (2006-11-16 12:31:36)

Railscasts - Free Ruby on Rails Screencasts

Re: Refactoring on Rails: Multiple Scopes in Controller

Oy, this post is a reminder to me to go clean up one nasty controller that I've been ignoring for months.  Well, here I go.  Thanks for the ever-awesome info Ryan.

Re: Refactoring on Rails: Multiple Scopes in Controller

ryanb wrote:

One thing interesting to note, some refactorings can lead directly to other smells/refactorings. For example, this refactoring had you move the common code into ApplicationHelper and ApplicationController. If either of these get very large or contain many methods that are only used by a couple controllers, that would be considered a smell. It would be interesting to see what kind of refactorings can solve that problem.

I think I'd expect the natural progression of adding common code to ApplicationController to lead to two further refactoring, but neither is Rail-centric, and I've seen minor instances of both in my code so far.  I'm less sure about ApplicationHelper as I haven't had to add to much there yet.

Method one for dealing with an ever growing ApplicationController -- intermediary sub-classed controllers.  If you have code shared by a few, but not all controllers, and the controllers that share code segment themelves into related areans, this works.  Ie If controller A and B share code, as do C and D, but there are no "cross terms" -- no A and C/D or B and C/D.  This however is just a normal refactoring out of Fowler's book -- pushing shared code to the proper level of an inheritence hierarchy and/or introducing a new level.

Method two would be more Ruby-ish and would invovled placing shared code into a module that is then included into the appropriate place in the Controller hierarchy.  I think long term this is the more 'correct' refactoring, but its not quite as simple and I haven't really examined how hard/easy it is to test the resulting module.  "Repeating" the "include foo" in multiple controllers wouldn't appear non-DRY to me, rather than putting "include foo" into the ApplicationController, when other controllers might not need it (the Refused Bequest code smell, to me, is worse than this instance of Duplicated Code).

From looking at the Rails source it obvisions that they use Method two a lot....

My RoR journey  -- thoughts on learning RoR and lessons learned in applying TDD and agile practices.

Re: Refactoring on Rails: Multiple Scopes in Controller

NielsenE wrote:

"Repeating" the "include foo" in multiple controllers wouldn't appear non-DRY to me, rather than putting "include foo" into the ApplicationController, when other controllers might not need it (the Refused Bequest code smell, to me, is worse than this instance of Duplicated Code).

I agree, but I think it depends largely on how many controllers actually need this code. If most of them do, I'd just include the module in the ApplicationController. If most of them don't, then I would include it in each controller. Some things are a little harder to do with a module though, like filters. I wouldn't be surprised if there was a hack around this. Thankfully I haven't had to deal with this problem.

As for the ApplicationHelper, I think creating a separate module is the answer there as well. This seems easier than the ApplicationController problem to me, as the helpers are already modules to begin with.

Railscasts - Free Ruby on Rails Screencasts

Re: Refactoring on Rails: Multiple Scopes in Controller

ryanb wrote:
NielsenE wrote:

"Repeating" the "include foo" in multiple controllers wouldn't appear non-DRY to me, rather than putting "include foo" into the ApplicationController, when other controllers might not need it (the Refused Bequest code smell, to me, is worse than this instance of Duplicated Code).

I agree, but I think it depends largely on how many controllers actually need this code. If most of them do, I'd just include the module in the ApplicationController. If most of them don't, then I would include it in each controller. Some things are a little harder to do with a module though, like filters. I wouldn't be surprised if there was a hack around this. Thankfully I haven't had to deal with this problem.

I tend to be very anti-"Refused Bequest"...  If I had 10 controllers and 9 needed some common code, I'd stick it into an intermediate subclass.  (I commonly end up with an "AuthenticatedController" that enforces all the user-must-be-logged-in checks for the underlying controllers, with only 1-2 controllers descending directly from ApplicationController).  I haven't ended up with too much other shared code in controllers, as most of it ends up in the models....  I haven't had a lot of "Aspect"-esque things to scatter-shot around the controllers which I think is where the inheritence breaks down and the module approach wins...

I think a slightly more complicated, but perhaps cleaner approach for very large projects would be to stick a lot of the code into a module, that's included by ApplicationController, but enabled/configured by the subclasses.  Similar to the way ActiveRecord works -- all classes have the methods defined for "has_many", or "validates_x" but you have to "enable" it in the specific subclass, so I'd take a similar appraoch and create the libraries of shared, generic controller code that all controllers have the ability to activate.

My RoR journey  -- thoughts on learning RoR and lessons learned in applying TDD and agile practices.

Re: Refactoring on Rails: Multiple Scopes in Controller

11. Change the links in all the views to use this new controller.

What if you want to display partials that contain actual content instead of links?  What I mean by that is this kind of refactoring seems to me would work will indeed with links.  But what if you really need to / want to display partials that are quite involved, and you'd like to refactor the controller to clean it up.  Perhaps I'd also be looking at refactoring the controller code so I could reuse it in some other views too.

What is the correct / clean way of refactoring controller code, perhaps even partial view code, when you'd like to re-use this code in multiple views?

As far as I understand it rails apps typically have a single view per webpage that is displayed.  That view typically as one controller.  In interest of refactoring and re-use.

Last edited by Slurpy (2006-11-22 10:18:52)

Re: Refactoring on Rails: Multiple Scopes in Controller

I'm not sure I'm understanding you correctly. From what I can tell you have specific controller code that goes with a partial, and you would like to render this partial in different controllers/actions. This leads to duplication because the partial requires some code in the controller.

There are many ways to solve this problem, and it largely depends upon your specific situation for the best solution.

1. Sometimes you can move the controller's code into the model. For example, if you are doing a complex find, create a custom find method in your model and just call this directly in the view.

2. Move the controller code into a helper method and have that helper method render the partial at the end. You can then just call this helper method whenever you want to render the partial.

3. Move the common code into a before filter.

4. Use components. These are "mini" controllers that go along with partials. There are mixed opinions about them, but if none of the above options are satisfactory, it may be the best solution. Use it if it works for you.

Railscasts - Free Ruby on Rails Screencasts

Re: Refactoring on Rails: Multiple Scopes in Controller

Thanks... let me make the situation a bit more clear.  I'm working on a website, whose pages are comprised of different kinds of logical content.  Ok -- think "widgets" if you want to.

I

Last edited by Slurpy (2006-11-22 15:52:11)

Re: Refactoring on Rails: Multiple Scopes in Controller

It is difficult to give a recommendation because there are so many variables. A lot of it depends upon how many widgets there will be, how you plan to add/maintain widgets, and if you will have  controllers which these widgets will be mimicking. For example, if you have other developers adding their own widgets, the widgets would probably need to stand up on their own without relying on something already implemented within the application. On the other hand, if the widgets are just offering a "miniature" version of an existing full page controller, then you would want to integrate them more with the rest of the application.

Regarding user input, you can easily communicate to any other controller through AJAX. For example, let's say you have a "subscription" widget which accepts an email address. You also have a SubscriptionsController which manages these subscriptions. You can place the widget in a partial:

# in views/widgets/_subscription.rhtml
<div id="subscription" class="widget">
<%= error_messsages_for :subscription %>
<%= form_remote_tag :controller => 'subscriptions', :action => 'create' %>
  <p>Email Address: <%= text_field :subscription, :email %></p>
  <%= submit_tag 'Subscribe' %>
<%= end_form_tag %>
</div>

This partial can be rendered anywhere. You can then respond appropriately through RJS in the controller:

# in subscriptions/create.rjs
if @subscription.errors.empty?
  page[:subscription].replace_html = "Successfully subscribed."
else
  page[:subscription].replace :partial => 'widgets/subscription'
end

The reason I recommend using the primary controller for accepting widget input is in case you want to add subscriptions through some other method, say a traditional subscription page. If you had moved the subscription creation logic into its own "widget" controller/component, you may need to duplicate the logic between the primary controller and the component. If you won't need that functionality, then there are probably better solutions.

As for the widgets which require logic to display them, components seem like a reasonable solution. If you are looking for an alternative you may want to go with the second option I gave (helpers). I would probably do something like this:

module WidgetsHelper
  def recent_topics_widget(limit = 5)
    topics = Topic.find(:all, :order => 'created_at DESC', :limit => limit)
    render :partial => 'widgets/recent_topics', :locals => { :topics => topics }
  end
 
  # all other widgets go here
end

# in application.rb
helper :widgets # loads the WidgetsHelper for all views

# in views/widgets/_recent_topics.rhtml
<div id="recent_topics" class="widget">
<% for topic in topics %>
  <div class="topic"><%= topic.name %></div>
<% end %>
</div>


You can then call this helper method in any view to render the widget:

<%= recent_topics_widget(10) %>

As I said above, it all depends upon the details of your application. You may need widgets to be fully self-contained, in that case components are probably the best solution.

Edit: I should mention, components aren't directly accessible through the URL, so there's really no way they can respond directly to user input without going through a controller. If you don't want to give each widget which accepts user input its own controller, then you may want to build your own abstract widget management system. Each widget could have its own class/module which contains all of the appropriate logic so it is completely self contained. You can create a generic "widgets" controller which handles all user input and forwards it to the appropriate widget.

This is going beyond my experience as I've never had to do anything like it, so take it with a grain of salt.

Last edited by ryanb (2006-11-22 17:41:00)

Railscasts - Free Ruby on Rails Screencasts

Re: Refactoring on Rails: Multiple Scopes in Controller

Thank you for the detailed response.  I'll work through this when I have a bit of time.  But FYI:  This isn't so much about other developers adding more "self-contained" widgets, but rather a question about organizing the code so that it be maintainable and clean, without code duplication.

I don't think that all widgets will have an associated "big brother app", but some will.

Thank you -- I'll give this a try next time I have a chance.