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.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
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:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
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.
1 2 3 4 5 6
The reason why I set variables for those things, was because the current array I’m getting from |item| is this
I had to do item 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.
1 2 3 4 5 6
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.
1 2 3 4 5
Now that we dumped that into it’s own method, our cart.each do looks a litle bit better.
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.
1 2 3 4 5 6 7 8 9
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.
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!
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?” ?
1 2 3
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.
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.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18
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.
1 2 3 4 5
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.
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.