Sometimes, clients give us feature requests that we really don’t like. It’s not that we don’t like our clients—we love our clients. It’s not that we don’t like the feature—most client-requested features are aligned perfectly with their business goals and income.
Sometimes, we don’t like a feature request because the easiest way to solve it is to write bad code, and we don’t have an elegant solution on the top of our heads. This will throw many of us on fruitless searches through RubyToolbox, GitHub, developer blogs, and StackOverflow looking for a gem or plugin or example code that will make us feel better about ourselves.
Well, I’m here to tell you: it’s okay to write bad code. Sometimes, bad code is easier to refactor into beautiful code than a poorly thought-out solution implemented under a time-crunch.
This is the process I like to follow when refactoring my horrible band-aid solutions:
For an alternative perspective, here’s the Git commit log for a feature that’s been refactored step-by-step:
Let’s see how it’s done.
Step 1. Start in the Views
Say we are beginning a ticket for a new feature. The client tells us: “Visitors should be able to view a list of active Projects on the welcome page.”
This ticket requires a visible change, so a reasonable place to start work would be in the Views. The problem is straightforward and one that we’ve all been trained to solve multiple times. I’m going to solve it The Wrong Way and demonstrate how to refactor my solution into it’s appropriate areas. Solving a problem The Wrong Way can help us get over the hump of not knowing the right solution.
To begin, assume we have a model called
Project with a boolean attribute called
active. We want to get a list of all
active is equal to
true, so we can use
Project.where(active: true), and loop over it with an
app/views/pages/welcome.haml: %ul.projects - Project.where(active: true).each do |project| %li.project= link_to project_path(project), project.name
I know what you’re saying: “That would never pass a code review” or “My client would surely fire me for this.” Yes, this solution breaks the Model-View-Controller separation of concerns, it might result in stray database calls that are difficult to trace, and it may become difficult to maintain in the future. But consider the value of doing it The Wrong Way:
- You can get this change on staging in under 15 minutes.
- If left in, this block is easy to cache.
- Fixing this Rails faux-pas is straightforward (could be given to a junior developer).
Step 2. Partials
After doing it The Wrong Way, I feel bad about myself and want to isolate my bad code. If this change were clearly only a concern of the View layer, I could refactor my shame into a partial.
app/views/pages/welcome.haml: = render :partial => 'shared/projects_list' app/views/shared/projects_list.haml: %ul.projects - Project.where(active: true).each do |project| %li.project= link_to project_path(project), project.name
That’s a little better. Clearly, we are still making the mistake of a Model query in a View, but at least when a maintainer comes in later and sees my horrible partial, they will have a straightforward way of tackling that problem in particular. Fixing something obviously dumb is always easier than fixing a poorly-implemented, buggy abstraction.
Step 3. Helpers
Helpers in Rails are a way of creating a DSL (Domain Specific Language) for a section of your Views. You have to rewrite your code using content_tag’s instead of haml or HTML, but you get the benefit of being allowed to manipulate data structures without having other developers glare at you for 15 lines of non-printing View code.
If I were to use helpers here, I would probably refactor out the
app/views/shared/projects_list.haml: %ul.projects - Project.where(active: true).each do |project| = project_list_item(project) app/helpers/projects_helper.rb: def project_list_item(project) content_tag(:li, :class => 'project') do link_to project_path(project), project.name end end
Step 4. Move It to the Controllers
Maybe your awful code isn’t just a View concern. If your code still smells, look for queries you can transition from the Views to the Controllers.
app/views/shared/projects_list.haml: %ul.projects - @projects_list.each do |project| = project_list_item(project) app/controllers/pages_controller.rb: def welcome @projects = Project.where(active: true) end
Step 5. Controller Filters
The most obvious reason to move code into a Controller
after_filter is for code that you’ve duplicated in multiple Controller actions. You can also move code into a Controller filter if you want to separate the purpose of the Controller action from the requirements of your views.
app/controllers/pages_controller.rb: before_filter :projects_list def welcome end def projects_list @projects = Project.where(active:true) end
Step 6. Application Controller
Presume that you need your code to show up on every page, or you want to make Controller helper functions available to all controllers, you can move your function into the ApplicationController. If the changes are global, you may want to modify your application layout as well.
app/controllers/pages_controller.rb: def welcome end app/views/layouts/application.haml: %ul.projects - projects_list.each do |project| = project_list_item(project) app/controllers/application_controller.rb: before_filter :projects_list def projects_list @projects = Project.where(active: true) end
Step 7. Refactoring to the Model
As the MVC motto goes: Fat Model, Skinny Controllers, and Dumb Views. We’re expected to refactor everything we can into the Model, and it’s true that most complex functionality will eventually become model associations and model methods. We should always avoid doing formatting/view things in the Model, but transforming data into other types of data is permissible. In this case, the best thing to refactor into the Model would be the
where(active: true) clause, which we can turn into a scope. Using a scope is valuable not only because it makes the call look prettier, but if we ever decided to add an attribute like
outdated, we can modify this scope instead of hunting down all of our
app/controllers/application_controller.rb: before_filter :projects_list def projects_list @projects = Project.active end app/models/project.rb: scope :active, where(active: true)
Step 8. Model Filters
We don’t have a particular use for a Model’s
after_save filters in this case, but the next step I usually take is to move functionality from Controllers and Model methods into Model filters.
Say we had another attribute,
num_views > 50, the Project becomes inactive. We could solve this problem in the View, but making database changes in a View is inappropriate. We could solve it in the Controller, but our Controllers should be as thin as possible! We can solve it easily in the Model.
app/models/project.rb: before_save :deactivate_if_over_num_views def deactivate_if_over_num_views if num_views > 50 self.active = false fi end
Note: you should avoid calling
self.save in a Model filter, as this causes recursive saving events, and the database-manipulation layer of your application should be the Controller anyway.
Step 9. Libraries
Occasionally, your feature is large enough that it could warrant it’s own library. You may want to move it into a library file because it’s reused in a lot of places, or it’s large enough that you’d like to do development on it separately.
It’s fine to store library files in the lib/ directory, but as they grow, you can transfer them into a real RubyGem! A major advantage of moving your code into a library is that you can test the library separately from your model.
Anyway, in the case of a Project List, we could justify moving the
scope :active call from the
Project model into a library file, and bring it back into Ruby:
app/models/project.rb: class Project < ActiveRecord::Base include Activeable before_filter :deactivate_if_over_num_views end lib/activeable.rb: module Activeable def self.included(k) k.scope :active, k.where(active: true) end def deactivate_if_over_num_views if num_views > 50 self.active = false end end end
self.included method is called when a Rails Model class is loaded and passes in the class scope as the variable
In under 15 minutes, we’ve implemented a solution and put it up on staging for user testing, ready to be accepted into the feature set or removed. By the end of the refactoring process, we have a piece of code that lays out a framework for implementing list-able, activate-able items across multiple Models that would pass even the most stringent review process.
In your own refactoring process, feel free to skip a few steps down the pipeline if you are confident in doing so (e.g., jump from View to Controller, or Controller to Model). Just keep in mind that the flow of code is from View to Model.
Don’t be afraid to look stupid. What separates modern languages from old CGI template rendering applications isn’t that we do everything The Right Way every time—it’s that we take the time to refactor, reuse, and share our efforts.