Refactoring - Theory of Coding

Refactoring Code

Why refactor?

After spending hours writing this piece of code, why would you want to go back to spend more time on refactoring it? It already works! Dealing with the big chunk of code you just wrote seems daunting and tiresome, but it is necessary for adding new features without making huge changes to existing code. Also, it is just easier to read and easier to go back to when you read it again in the future. Depending on what you wrote, there are millions of ways to refactor, but this post just shows what I did to refactor my 28 line method into 12 lines of code. You can check out Sandi Metz’s video ‘All the litting things’ to get better at refactoring.

Make it go green first

Before refactoring, you must have the code working first. No matter how messy and ugly your code is, just make it work. Make the test pass green!

Here is some code I wrote for the Green Grocer lab, where the apply_coupons method take in a cart and coupon hash argument, and applying the coupon on the cart items if applicable. It should return a hash of the items with the correct quantity, and the “item name W/COUPON” with the discount.

def apply_coupons(cart:[], coupons:[])
    apply_coupons_hash = {}
      coupon_items = [] # get coupon item names
        coupons.each do |item|
          coupon_items << item[:item]
        end
    cart.each do |item|
      name = item[0]
      attributes = item[1]
      num_of_coupon = 0
      coupon_price = 0
      coupons.each do |coupon_hash| #get num and cost of that particular item from coupons hash
        if coupon_hash[:item] == name
          num_of_coupon = coupon_hash[:num]
          coupon_price = coupon_hash[:cost]
        end
      end
      num_of_coupon #quantity of item required for coupon to work

      if ((coupon_items.include?(name)) && (attributes[:count] >= num_of_coupon))
      #for the current item, if the count is >= coupon quantity requirement
      #decrease the item count and add the coupon hash 
      quantity = (attributes[:count] / num_of_coupon) #item quantity/coupon quantity
        apply_coupons_hash[name] = {:price => attributes[:price], :clearance => attributes[:clearance], :count =>(attributes[:count] - (num_of_coupon * quantity))}
        apply_coupons_hash[name + ' W/COUPON'] = {:price => coupon_price, :clearance => attributes[:clearance], :count => quantity}
      else #if no coupon for this item, then just add the existing item hash to apply_coupons_hash
        apply_coupons_hash[name] = {:price => attributes[:price], :clearance => attributes[:clearance], :count => attributes[:count]}
      end
    end
    apply_coupons_hash
 end

This monstrous piece of code is 28 lines long… No way am I going to remember what each part does a week later.

Here is my refactored version of the code:

def apply_coupons(cart:[], coupons:[])
    apply_coupons_hash = {}
    cart.each do |item|
      item_hash = build_item_hash(item)
      matching_coupon_for_item = coupons.detect {|coupon| coupon[:item] == item_hash[:name]}
      if matching_coupon_for_item && can_apply_coupon?(item_hash, matching_coupon_for_item)
        quantity = (item_hash[:count] / matching_coupon_for_item[:num])
        apply_coupons_hash[item_hash[:name]] = {:price => item_hash[:price], :clearance => item_hash[:clearance], :count =>(item_hash[:count] - (matching_coupon_for_item[:num] * quantity))}
        apply_coupons_hash[item_hash[:name] + ' W/COUPON'] = {:price => matching_coupon_for_item[:cost], :clearance => item_hash[:clearance], :count => quantity}
      else
        apply_coupons_hash[item_hash[:name]] = {:price => item_hash[:price], :clearance => item_hash[:clearance], :count => item_hash[:count]}
      end
    end
    apply_coupons_hash
end

It’s only 12 lines long compared to my 28 line code before!! There is hope.

Change data structure to fit your needs

I can totally get rid of these lines of code if I had the data structure I wanted.

cart.each do |item|
      name = item[0]
      attributes = item[1]
      num_of_coupon = 0
      coupon_price = 0
      coupons.each do |coupon_hash| #get num and cost of that 

The reason why I set variables for those things, was because the current array I’m getting from |item| is this

["AVOCADO", {:price => 3.0, :clearance => true, :count => 2}]

I had to do item[0] to get the name of the item, and etc,. All I wanted to do here was to get the information from the item so I can use it later in my if statement. These 4 lines of code can be replaced into 1 if I had the data structure to simply say something like item_hash[:name] to JUST get the name.

cart.each do |item|
  item_hash = {}
  item_hash[:name] = item_array.first
  item_hash.merge!(item_array.last)
  #this is the return value
  #=> {:name=>"AVOCADO", :price=>3.0, :clearance=>true, :count=>2}

Don’t be afraid to make new methods

Knowing the single responsibility rule, getting the item hash seems like it should be it’s own method. Rebuilding the datastructure is for our own ease, and has nothing to do with apply_coupons. Don’t worry about adding more methods and adding more lines of code. This is usually the result of refactoring; abstracting parts away into their own methods make them reusable and clear to the reader what this function is doing.

def build_item_hash(item_array)
  item_hash = {}
  item_hash[:name] = item_array.first
  item_hash.merge!(item_array.last)
end

Now that we dumped that into it’s own method, our cart.each do looks a litle bit better.

cart.each do |item|
      item_hash = build_item_hash(item)

Now, I tried to iterate over coupons to grab the coupon that matches the current item name, so that I can check if items in the cart meet the coupon requirement.

coupons.each do |coupon_hash| #get num and cost of that particular item from coupons hash
        if coupon_hash[:item] == name
          num_of_coupon = coupon_hash[:num]
          coupon_price = coupon_hash[:cost]
        end
      end
      num_of_coupon #quantity of item required for coupon to work

      if ((coupon_items.include?(name)) && (attributes[:count] >= num_of_coupon))

Look for specialized methods to do the job

Sure, you can use the broad .each method on anything, but there are many methods out there that you can use that will make your life easier. Here is a list of ruby methods for reference! You can see methods you can use for arrays, strings, etc,.

Instead of iterating through the coupons to check if the item name matched with the coupon name, and then returning the quantity requirement (num) for the coupon, I can just approach this differently and just use a more specialized method, ‘detect’ to see if the coupon exists for this item.

matching_coupon_for_item = coupons.detect {|coupon| coupon[:item] == item_hash[:name]}

Those 6 lines just turned into a 1 liner, after using .detect instead of .each

Writing code I wish I had

Sometimes if you don’t know what to write, you can start by writing a method that you wish you had, and then try to make the method!

if ((coupon_items.include?(name)) && (attributes[:count] >= num_of_coupon))

After all of the refactoring I did above, I FINALLY reach my if statement to see if there is a coupon that matches the current item, AND see if the quantity requirement of the coupon was met by the item count. If someone was reading this, they’d have to find out what attributes is , and what num of coupon means also. Wouldn’t it be nice to just write “can apply coupon?” ?

def can_apply_coupon?(item_hash, matching_coupon_for_item)
  (item_hash[:count] >= matching_coupon_for_item[:num])
end

I made this into a new method. Although it did not decrease my lines of code, it did make it clear what I was trying to do in my if statement.

if matching_coupon_for_item && can_apply_coupon?(item_hash, matching_coupon_for_item)

Now, I can proceed to do what I wanted to do all along, for this one apply coupons method. For the refactored version, you can see the resulting method goes straight into just applying the coupons to the cart and returning the new hash if there was a coupon that exists and if the requirement is met.

def apply_coupons(cart:[], coupons:[])
    apply_coupons_hash = {}
    cart.each do |item|
      item_hash = build_item_hash(item)
      matching_coupon_for_item = coupons.detect {|coupon| coupon[:item] == item_hash[:name]}
      if matching_coupon_for_item && can_apply_coupon?(item_hash, matching_coupon_for_item)
        quantity = (item_hash[:count] / matching_coupon_for_item[:num])
        apply_coupons_hash[item_hash[:name]] = {:price => item_hash[:price], :clearance => item_hash[:clearance], :count =>(item_hash[:count] - (matching_coupon_for_item[:num] * quantity))}
        apply_coupons_hash[item_hash[:name] + ' W/COUPON'] = {:price => matching_coupon_for_item[:cost], :clearance => item_hash[:clearance], :count => quantity}
      else
        apply_coupons_hash[item_hash[:name]] = {:price => item_hash[:price], :clearance => item_hash[:clearance], :count => item_hash[:count]}
      end
    end
    apply_coupons_hash
end
# returns the new hash of the updated cart after coupons are applied
#=> {"AVOCADO"=>{:price=>3.0, :clearance=>true, :count=>0},
#=>  "AVOCADO W/COUPON"=>{:price=>5.0, :clearance=>true, :count=>1}}

In summary, these are things you should keep in mind when refactoring:

Make it go green first
Change data structure to fit your needs
Don’t be afraid to make new methods
Look for specialized methods to do the job
Writing methods I wish I had

Don’t freak out when your tests turn red, it’ll take a while before it turns green again, but it’s worth it.

In general, when you use .each and your code looks like a sandwich, you should be thinking of other iterators that is made specifically for the type of function you’re trying to do.

new_array = []
[1,2,3,4,5].each do |numbers|
  new_array << numbers.even?
end
new_array

In this sandwich, you set a new empty array, do the iteration to grab stuff and put it into the empty array, and then return the new array. This can be easily done with select, or collect.

[1,2,3,4,5].select { |num|  num.even?  }

Most of the time, when there is duplication of code, you can probably abstract it away into a method. However, don’t make the wrong abstraction. It’s better to have duplication than the wrong abstraction. As Sandi Metz says, it’s like surfing farther and farther out into the ocean, when you’re doing the wrong abstraction.

When writing code, always try to be explicit as possible, because it’ll make it easier for other people to understand your code. You can use comments to explain what you’re doing here, but try to make your code so clear that comments are not needed at all to explain what you’re trying to do.

I only recently started refactoring code, but I hope you’ll keep these things in mind when you decide to refactor. Sandi Metz gave a talk on refactoring called ‘All the litting things’ and you should check it out to get better at it! Here is a cheat sheet on refactoring.