Refactoring on Rails: Move to Model

ryanb · 2006-09-24 22:56:51

This is the first article in a series which will cover various refactoring techniques that are specific to Rails. Refactoring is used to improve the design of existing code without changing its functionality.

The first refactoring I will be discussing involves taking a bit of logic out of the view and placing it in the model. This can clean up the views quite nicely.

Let's start out with a very simple example. We have a Person model with first_name, last_name, and middle_initial attributes. In our view we have this code:

Code : ruby - fold - unfold

Full Name:
<%= @person.first_name %>
<% unless @person.middle_initial.blank? %>
<%= @person.middle_initial %>.
<% end %>
<%= @person.last_name %>

Not very pretty. Even worse, this code is repeated several times throughout the application. To remove this duplication and to clean up the views, we can define a method in the Person class to handle this logic.

Code : ruby - fold - unfold

class Person < ActiveRecord::Base
def full_name
result = first_name + ' '
unless middle_initial.blank?
result += middle_initial + '. '
end
result += last_name
result
end
end

This makes the view much nicer:

Code : ruby - fold - unfold

Full Name: <%= person.full_name %>

We successfully shoved the logic into the Person model, but I'm still not satisfied. I think we can clean up the full_name method, but before we do that, let's create some tests to make sure everything continues to work as we do the cleaning. Tests are very important when refactoring to make sure we don't break anything as we improve the code.

Code : ruby - fold - unfold

# in person_test.rb
def setup
@person = Person.new(:first_name => 'John', :last_name => 'Smith')
end
def test_full_name
assert_equal 'John Smith', @person.full_name
end
def test_full_name_with_middle_initial
@person.middle_initial = 'A'
assert_equal 'John A. Smith', @person.full_name
end

Good, the tests are passing so now it's time to improve the full_name method. I would like to remove that temporary "result" variable and merge it all onto one line. I can use an array to help me out. Maybe something like this:

Code : ruby - fold - unfold

def full_name
[first_name, middle_initial, last_name].join(' ')
end

That looks nice and tidy, but unsurprisingly it fails both of the tests. It fails the second test because there's no period, so let's create a new method to return a middle initial with a period:

Code : ruby - fold - unfold

def middle_initial_with_period
middle_initial + '.' unless middle_initial.blank?
end
def full_name
[first_name, middle_initial_with_period, last_name].join(' ')
end

The second test is working now, but the first test still fails because there's an extra space in there when no middle initial is defined. We can remove that by using array's "compact" method like this:

Code : ruby - fold - unfold

def full_name
[first_name, middle_initial_with_period, last_name].compact.join(' ')
end

Our tests are all passing now. Yay!

Wait one minute, I hear someone say, the compact method just removes nil values, what if the middle initial is an empty string? Well let's add a test and find out:

Code : ruby - fold - unfold

def test_full_name_with_empty_middle_initial
@person.middle_initial = ''
assert_equal 'John Smith', @person.full_name
end

Surprisingly this passes successfully without us changing anything. Why? Because the middle_initial_with_period method already takes care of empty strings and will return nothing (nil) if middle_initial is blank.

In this tutorial you learned how to refactor code from the view into the model, but this can just as easily be applied to code in the controller or any other part of the application. Your job is to look through your current rails projects for code that can be moved into a model. Usually if it can be moved into a model, it should, but just be careful to keep view/controller specific code out of the model. For example, don't move HTML code into the model.

If you found this useful, keep an eye out for the next Refactoring on Rails article. In the meantime, check out Martin Fowler's excellent book: Refactoring.

Check out the next article in this series: Multiple Scopes in Controller

Last edited by ryanb (2006-11-16 16:29:48)

Kelli · 2006-09-25 16:52:42

Nicely done! I wish I'd seen this before I wrote my online billing system. I did end up doing this, but it was a lot more muddling through that I'd have otherwise had. wink

boyles · 2006-09-25 20:36:26

RAILSforum is turning out to be a great resource for Tutorials.
Nice work

Added this one as well to my growing (140+) collection below

Last edited by boyles (2006-09-25 22:07:55)

jwcooper · 2006-11-15 01:53:16

Excellent tutorial, thank you Ryan. I was starting to wonder what the Model's were for. wink

I should really get a rails book...

cathyb · 2006-11-15 14:16:47

This refactoring is great and I'm looking forward to the rest of this series of tutorials.

I've done something similar in my person model to create a full_address method from all the individual address fields. I'm sure this could be DRYied up somewhat.

Code : - fold - unfold

def full_address
result = ''
unless address_line_1.blank?
result += address_line_1
end
unless address_line_2.blank?
if result == ''
result += address_line_2
else
result += ', ' + address_line_2
end
end
unless address_line_3.blank?
if result == ''
result += address_line_3
else
result += ', ' + address_line_3
end
end
unless address_line_4.blank?
if result == ''
result += address_line_4
else
result += ', ' + address_line_4
end
end
unless town.blank?
if result == ''
result += town
else
result += ', ' + town
end
end
unless county.blank?
if result == ''
result += county
else
result += ', ' + county
end
end
unless postcode.blank?
if result == ''
result += postcode
else
result += ', ' + postcode
end
end
result
end

The above puts whatever parts of the address that exist all on one line with commas after each bit. However in a different view on my app I want to display the address differently with each part on a separate line with <br> in between instead of commas. Would I be best creating a new method full_address_multi_line or can I adapt my existing method to do both?

Also when I am displaying the person's profile details in the view, where a field is not completed I might want to display "Not supplied" or "N/A". Is this best handled in the view or could it be moved to the model too? At the minute I've got a lot of stuff like this in my view:

Code : - fold - unfold

<%= if @person.job_title.blank?
"Not supplied"
else
@person.job_title
end %>

ryanb · 2006-11-15 18:48:49

Very good questions Cathy!

The full_address method can be greatly simplified by using a technique similar to what I did in the article - which is placing the address sections in an array and joining that.

Code : ruby - fold - unfold

def full_address(join_str = ', ')
sections = [address_line_1, address_line_2, address_line_3] # etc...
sections.delete_if(&:blank?) # removes any blank entries from the array
sections.join(join_str)
end

Notice I passed the join string as a parameter, this way you can change it when calling the method.

You can go one step further to clean this up a bit more:

Code : ruby - fold - unfold

def full_address(join_str = ', ')
address_sections.delete_if(&:blank?).join(join_str)
end
def address_sections
[address_line_1, address_line_2, address_line_3]
end

As for your second question, I would hesitate to move this "Not Supplied" logic into the model. First of all you will need to handle this for many different attributes and models. Second, if you override the original attribute it will probably cause problems at the model/controller level. Lastly you may want to handle this differently in different views.

Certainly keeping this in the view isn't optimal either. What I would do is create a helper method that handles this for you. You can then input any value you want and it will return "Not Supplied" automatically when blank. For example:

Code : ruby - fold - unfold

# in view
<%= not_supplied_if_blank @person.job_title %>
# in application_helper
def not_supplied_if_blank(value)
value.blank? ? "Not Supplied" : value
end

You may want to rename that method.

Last edited by ryanb (2006-11-15 18:51:41)

cathyb · 2006-11-16 11:46:41

Thanks Ryan for the lovely code - you've certainly made my full_address method a lot more elegant and useful. cool

After I had posted I found some examples online of using application helpers and ended up writing pretty much what you wrote for showing "Not supplied" when something was blank. I've not really used helpers a great deal yet but am starting to. I guess when things get repetative in the view and don't seem to fit in the model then helpers are a good solution.

Both my view and model are now looking a lot better. Thanks again for the great advice. BTW I think you should link to the other refactoring tutorials at the top of this one.

#1 2006-09-24 22:56:51

#2 2006-09-25 16:52:42

Re: Refactoring on Rails: Move to Model

#3 2006-09-25 20:36:26

Re: Refactoring on Rails: Move to Model

#4 2006-11-15 01:53:16

Re: Refactoring on Rails: Move to Model

#5 2006-11-15 14:16:47

Re: Refactoring on Rails: Move to Model

#6 2006-11-15 18:48:49

Re: Refactoring on Rails: Move to Model

#7 2006-11-16 11:46:41

Re: Refactoring on Rails: Move to Model

Board footer