#91 Refactoring Long Methods

Feb 03, 2008 | 7 minutes | Active Record, Refactoring

In this episode I walk you through an exercise in refactoring by taking a long method and making it more concise.

Download:
mp4Full Size H.264 Video (11.3 MB)
m4vSmaller H.264 Video (7.61 MB)
webmFull Size VP8 Video (22.8 MB)
ogvFull Size Theora Video (14.4 MB)

Nice cleaning up!
I also really like the one liners, it keeps your code clean.
Keep up the good work, I already learned a lot from your railscasts.

Artūras Šlajus almost 17 years ago

I would not cache total_weight, but instead swap if/elsif statements for case statement. It only evaluetes condition once.

irb(main):001:0> def call_me; puts 'called!'; 16; end
=> nil
irb(main):002:0> call_me
called!
=> 16
irb(main):003:0> case call_me
irb(main):004:1> when (0..3)
irb(main):005:1> puts 'zero to three'
irb(main):006:1> when (4..10)
irb(main):007:1> puts 'something else'
irb(main):008:1> when (11..20)
irb(main):009:1> puts 'a lot'
irb(main):010:1> else
irb(main):011:1* puts 'whee'
irb(main):012:1> end
called!
a lot
=> nil

Ted almost 17 years ago

I think you've introduced a bug by caching total_weight. If you add items to the cart after calling total_weight, the cached value will not be updated, no?

Karl almost 17 years ago

Ted:
You have a point. Depends if he is saving the cart instance (as in a session). If he just saves the cart.id in the session and does Cart.find(session[:cart_id]) then he's safe.

I like the case statements approach better too.

Aaron H. almost 17 years ago

First off, great screencasts. Love them all and look forward to new ones every week.

I want to jump in and agree with Artūras and Ted. In this situation, using the instance variable could introduce unusual behavior down the line that is difficult to track down. In fact, I was surprised when the screencast ended that you didn't show that.

(In an effort to help any rails noobs out there with specific code for this, I thought I'd include the actual code I'd use).

def shipping_price
  case total_weight
    when 0
      0.00
    when 0..3
      8.00
    when 3..5
      10.00
    else
      12.00
    end
end

Finally, you can also leverage the DB's sum calls to get total amounts as well. For example, your cart model could have:
has_many :products, :through => :line_items

Then, your total weight function would look like
def total_weight
products.sum :weight, :conditions => {:for_download => false}
end

If you had a cart with a huge number of items, this would be quite beneficial - especially if you haven't loaded (and won't need) all of the line items.

Inside almost 17 years ago

Great work! I'm waiting for the next ;)

[]'s

crayz almost 17 years ago

@Ted - the instance variables will only be cached for the lifecycle of the object, which unless you're doing something really bizarre will be contained in the lifecycle of the request

So the memoization shouldn't be an issue

foozilla almost 17 years ago

Can you show us how to get the autocomplete and in-place edit work in rails 2.0? I am not sure how.

What about that JRails with JQuery stuff? I think there is a plugin for it.

thanks as always

Brian almost 17 years ago

I think having a 4 row db for shipping prices is a bit much. Using a case statement could reduce that down. If you didn't want the numbers in the file I'd store them in a yaml file or plain text before a database.

Ryan Bates almost 17 years ago

@Artūras, Ted, and Aaron. Very good points. I agree your solution is better. Thanks for pointing this out.

@Brian, I thought of using a YAML file too, and I agree it is better than placing it in the code. But I still prefer having a full model. This way it will have a ruby class in front and allow us to refactor more code into there if need be. We can also move calculations into the database to improve performance if we need to as Aaron mentioned.

Darryl Ring almost 17 years ago

The use of the instance variable comes down to intelligent choice, really. It seems unlikely that you're going to calculate the total weight, alter the cart, then calculate the weight again within one request. It depends if you're looking for the simplest solution that works in your application or a solution that is perfect in every edge case.

kino mk over 16 years ago

The first known use of the term "refactoring" in the published literature was in a September, 1990 article by William F. Opdyke and Ralph E. Johnson.[5] Opdyke's Ph.D. thesis[2], published in 1992, also used this term.[3] The term "refactoring" was almost certainly used before then.