An example on how to tackle refactoring in a gentle way that does not change the API.
As for all refactoring tasks, you should have a good test system in place. Since the API stays intact, all tests should pass after the changes are done.
Take the example class Product below and imagine it grew over time and that some special cases got added here and there.
There is an attribute called 'product_type' that changes functionality of the object based on its value. Some products of product_type 'A' might require special care while those of product_type 'B' need to go through a different delivery process.
I've taken an ActiveRecord model as an example, but the concept is not tied to ActiveRecord or Ruby on Rails in any way.
The goal is to make the code more readable and to avoid polluting the Product class with too many special cases while keeping the external API as it is. Unit testing of the individual decoupled components can be done much better/faster afterwards, but this is not in the scope of this demo.
# A bloated example: class Product < ActiveRecord has_one :special_case_here has_one :special_case_there before_create :do_something_wired def do_something_wired # this is very special and only applies on mondays! ... end def attention_required? if product_type == 'A' and some_wired_condition true elsif product_type == 'B' and another_condition true else false end end def wrap_the_product case product_type when 'A' ... when 'C' ... ... end
As a first step I will remove the two special cases from the main class and put them into separate modules that I can include.
The first special case called 'Here':
module SpecialCase module Here def self.included(base) base.has_one :special_case_here base.before_create :do_something_wired end def do_something_wired # this is very special and only applies on mondays! ... end end end
The second one called 'There':
module SpecialCase module There def self.included(base) base.has_one :special_case_there end ... end end
This allows me to remove the code for the special cases and just include the two modules.
The Product now looks like this:
# A less bloated example: class Product < ActiveRecord include SpecialCase::Here include SpecialCase::There def attention_required? if product_type == 'A' and some_wired_condition true elsif product_type == 'B' and another_condition true else false end end def wrap_the_product case product_type when 'A' ... when 'C' ... ... end
This already looks much better. The Product class does not spell out those special cases anymore.
As a next step I would like to remove all conditional jumps that are based on the product_type.
I am not using STI, since I'd like the code to be decoupled as much as possible for fast testing and flexibility.
The new version below makes the something called 'ProductTypeDelegator' responsible for calling the right code for a specific task that is dependent on the current product_type attribute:
# final unbloated example class Product < ActiveRecord include SpecialCase::Here include SpecialCase::There def attention_required? ProductTypeDelegator.new(self)._attention_required? end def _attention_required? false # the default if the TypeDelegator can not find a method end def wrap_the_product ProductTypeDelegator.new(self)._wrap_the_product end def _wrap_the_product ... # the default if the TypeDelegator can not find a method end ... end
Since API changes are not allowed, I use the original method name to delegate the task.
# I can call my methods like I am used to: product.wrap_the_productThen I use a second method to hold the code for those cases where the ProductTypeDelegator can not find a method to execute and delegates the task back to the calling Product object.
I am not allowed to change the API.. so I will stick to those kind of ugly looking pairs of 'method' and '_method'.
The ProductTypeDelegator is responsible for calling the correct code based on the product_type.
It should/could be generic it and should/could have no knowledge of who is calling and to what it is delegating.
It is not that hard to write a GenericTypeDelegator that you would initialize in a way that it allows for more than just this use case. This would be nice. But it is a bit overkill for this demo. I will allow to ProductTypeDelegator to have some hard coded logic.
Looking at the original code, I will need at least 3 separate targets to take care of the product_type being either 'A','B' or 'C'. I will call those Decorators.
The method_missing method is used to allow arbitrary calls to be delegated to the specific Decorators.
require 'delegate' class ProductTypeDelegator < SimpleDelegator def initialize(myobj) @caller = myobj end def method_missing(sym, *args, &block) if @caller.product_type == 'A' ProductTypeADecorator.new(@caller).send(sym,*args,&block) elsif @caller.product_type == 'B' ProductTypeBDecorator.new(@caller).send(sym,*args,&block) elsif @caller.product_type == 'C' ProductTypeCDecorator.new(@caller).send(sym,*args,&block) else if @caller.respond_to? sym # calling the original object @caller.send sym, *args, &block else # method is missing from the product model super end end end end
The Decorators that contain the specific code that needs to run for a specific product_type:
class ProductTypeADecorator < SimpleDelegator def attention_required? some_wired_condition end def wrap_the_product ... end ... end class ProductTypeBDecorator < SimpleDelegator def attention_required? another_condition end ... end class ProductTypeCDecorator < SimpleDelegator def attention_required? some_wired_condition and something_else end def wrap_the_product ... end ... end
The beauty of this approach is that it allows you to change the product_type and the behavior at runtime without leaving the Product class. No switching from something like TypeAProduct < Product to TypeBProduct < Product is needed like you would when using STI.
If API changes would be allowed, one could write the code even cleaner:
class Product < ActiveRecord include SpecialCase::Here include SpecialCase::There def attention_required? false # the default end def wrap_the_product ... # the default end ... end
and then wrap calls to the Product object into the TypeDelegator:
# if api changes would be allowed: TypeDelegator.new(product).wrap_the_product
Do you have a better/nicer way? Does this make sense to you? Something missing? Something wrong?
Let me know via: semi analog pingback
phone: +49 22 47 90 80 250
mobile/signal: +49 157 3131 4424
GPG/PGP-Key: (pub 4096R/069DD13C 2014-02-13) local copy pgp.mit.edu
GPG/PGP-Key: fingerprint: 9BAD 94D3 9176 5BD1 F64F 542E 37E4 3542 069D D13C