Today I'm continuing to refactor a library called CreateSend. The first part is here.
In part 1 I've finished all class methods of Base classs and now I'm going to refactor instance methods.
Step 1: initialize method
| def initialize(*args) | |
| if args.size > 0 | |
| auth args.first # Expect auth details as first argument | |
| end | |
| end |
initialize (see above ↑) chooses not to use named arguments, and treats method arguments as an array of many arguments. And yet, it only processes one argument from the whole args array.
It's misleading to accept arguments and throw them away. It will require a look into source code to find out why some passed arguments caused no change in behaviour. On the other hand, if we state that initialize only accepts one argument, Ruby will complain if we pass any other parameters. Easier to debug. Thus:
| def initialize(new_auth = nil) | |
| auth new_auth if new_auth | |
| end |
This change required changing subclasses of Base, and they all pass in auth argument.
Step 2: auth method
| # Authenticate using either OAuth or an API key. | |
| def auth(auth_details) | |
| @auth_details = auth_details | |
| end |
auth is probably short for authenticate, and if so, it's misleading. No authentication is happening here, just an assignment. It can be replaced with an attr_accessor. It could be even replaced with nothing (meaning, @auth_details could be enough to have, and instance variables don't need to be declared), but I don't know enough, maybe it's part of public API. Thus:
| # Holds either OAuth or an API key. | |
| attr_accessor :auth_details |
Step 3: refresh_token method
| # Refresh the current OAuth token using the current refresh token. | |
| def refresh_token | |
| if not @auth_details or | |
| not @auth_details.has_key? :refresh_token or | |
| not @auth_details[:refresh_token] | |
| raise '@auth_details[:refresh_token] does not contain a refresh token.' | |
| end | |
| tokens = Base.refresh_access_token @auth_details[:refresh_token] | |
| @auth_details = { | |
| :access_token => tokens.access_token, | |
| :refresh_token => tokens.refresh_token} | |
| tokens | |
| end |
The conditional on the lines 3-7 (see above ↑) is overly complex, using many nots and an unnecessary has_key? (line 4). has_key? :refresh_token is redundant because we later check @auth_details[:refresh_token] value. So, if the key isn't present, the conditinal evaluates to false. If we don't check for key, value of :refresh_token would be nil, leading to the same false. And, if key is present, value check will determine true or false. Thus:
| # Refresh the current OAuth token using the current refresh token. | |
| def refresh_token | |
| raise '@auth_details[:refresh_token] does not contain a refresh token.' \ | |
| unless @auth_details && @auth_details[:refresh_token] | |
| tokens = Base.refresh_access_token @auth_details[:refresh_token] | |
| @auth_details = { | |
| :access_token => tokens.access_token, | |
| :refresh_token => tokens.refresh_token} | |
| tokens | |
| end |
I'd use a variable for @auth_details[:refresh_token] result, to avoid querying hash twice, but I couldn't think of a good name for it, as refresh_token is already taken by method name.
Step 4: API wrapper methods
| # Gets your clients. | |
| def clients | |
| response = get('/clients.json') | |
| response.map{|item| Hashie::Mash.new(item)} | |
| end | |
| # Get your billing details. | |
| def billing_details | |
| response = get('/billingdetails.json') | |
| Hashie::Mash.new(response) | |
| end |
These methods (see above ↑) wrap access to JSON API. I've included two methods, but there are more of them. I'm going to skip them as I don't see how to improve them.
Step 5: get, put, post and delete methods
| def get(*args) | |
| args = add_auth_details_to_options(args) | |
| handle_response Base.get(*args) | |
| end | |
| alias_method :cs_get, :get | |
| def post(*args) | |
| args = add_auth_details_to_options(args) | |
| handle_response Base.post(*args) | |
| end | |
| alias_method :cs_post, :post | |
| def put(*args) | |
| args = add_auth_details_to_options(args) | |
| handle_response Base.put(*args) | |
| end | |
| alias_method :cs_put, :put | |
| def delete(*args) | |
| args = add_auth_details_to_options(args) | |
| handle_response Base.delete(*args) | |
| end | |
| alias_method :cs_delete, :delete |
All these methods (see above ↑) are almost the same, and can be reduced to something like cs_method :get, :post, ... using metaprogramming. Like this:
| class << self | |
| private | |
| def cs_method(*names) | |
| names.each { |name| define_cs_method name } | |
| end | |
| def define_cs_method(name) | |
| define_method(name) do |*args| | |
| args = add_auth_details_to_options(args) | |
| handle_response Base.send(name, *args) | |
| end | |
| alias_method "cs_#{name}", name | |
| end | |
| ... | |
| cs_method :get, :post, :put, :delete | |
| ... |
Step 6: add_auth_details_to_options
add_auth_details_to_options is used in step 5 methods to "add auth details to options", at the moment not clear, why it's options and not args. Here it is:
| def add_auth_details_to_options(args) | |
| if @auth_details | |
| options = {} | |
| if args.size > 1 | |
| options = args[1] | |
| end | |
| if @auth_details.has_key? :access_token | |
| options[:headers] = { | |
| "Authorization" => "Bearer #{@auth_details[:access_token]}" } | |
| elsif @auth_details.has_key? :api_key | |
| if not options.has_key? :basic_auth | |
| options[:basic_auth] = { | |
| :username => @auth_details[:api_key], :password => 'x' } | |
| end | |
| end | |
| args[1] = options | |
| end | |
| args | |
| end |
It does look overwhelming at the first glance. But I have no intention of being overwhelmed by it. I'm going to simplify it step by step.
The first thing I see is that at line 18 (see above ↑), args is returned unchanged, if there's no @auth_details present. This is typical guard clause case.
| def add_auth_details_to_options(args) | |
| return args unless @auth_details | |
| options = {} | |
| if args.size > 1 | |
| options = args[1] | |
| end | |
| if @auth_details.has_key? :access_token | |
| options[:headers] = { | |
| "Authorization" => "Bearer #{@auth_details[:access_token]}" } | |
| elsif @auth_details.has_key? :api_key | |
| if not options.has_key? :basic_auth | |
| options[:basic_auth] = { | |
| :username => @auth_details[:api_key], :password => 'x' } | |
| end | |
| end | |
| args[1] = options | |
| args | |
| end |
Step 6.1: options
At lines 4-7 (see above ↑) we see that so called options are expected as the 2nd element of args array, and we use it if present. It's actually quite hard to reason about this code because if args[1] is present and options get assigned it, and it's nil, we might get an exception later on, if @auth_details has certain data in it and it tries to use nil as a Hash. I want to simplify it!
The lines 8-16 (see above ↑) add stuff to options. And the line 17 (see above ↑) assigns options back as the second element of args.
It's way too complicated. According to Single Responsibility Principle, a method should have one responsibility only. For this method it means adding stuff to options. Here's the result:
| def add_auth_details_to_options(options) | |
| return unless @auth_details | |
| if @auth_details.has_key? :access_token | |
| options[:headers] = { | |
| "Authorization" => "Bearer #{@auth_details[:access_token]}" } | |
| elsif @auth_details.has_key? :api_key | |
| if not options.has_key? :basic_auth | |
| options[:basic_auth] = { | |
| :username => @auth_details[:api_key], :password => 'x' } | |
| end | |
| end | |
| end |
None of that nasty business with args[1] is present anymore, much simpler! And look how all the args[1] business is in one place here:
| def define_cs_method(name) | |
| define_method(name) do |*args| | |
| options = {} | |
| if args.size > 1 | |
| options = args[1] | |
| end | |
| args[1] = options | |
| add_auth_details_to_options(options) | |
| handle_response Base.send(name, *args) | |
| end | |
| alias_method "cs_#{name}", name | |
| end |
It is the same code (lines 3-7 ↑), same functionality, but it's all in one place! Being in one place means there's no switching of contexts required, which means it's easier to read.
Step 6.2: putting stuff into options
Let's continue with improving add_auth_details_to_options. I'm placing the same code here again, so it's easier to compare with the code I'll have refactored:
| def add_auth_details_to_options(options) | |
| return unless @auth_details | |
| if @auth_details.has_key? :access_token | |
| options[:headers] = { | |
| "Authorization" => "Bearer #{@auth_details[:access_token]}" } | |
| elsif @auth_details.has_key? :api_key | |
| if not options.has_key? :basic_auth | |
| options[:basic_auth] = { | |
| :username => @auth_details[:api_key], :password => 'x' } | |
| end | |
| end | |
| end |
Lines 4-6 (see above ↑) are pretty straightforward, just adding an entry into options if :access_token is present in @auth_details. For some reason the code is really careful to put even nil values of @auth_details[:access_token] into authorization header (if it checked for value instead of key presence, it'd not put the authorization header in at all).
Lines 7-12 (see above ↑) are similar, but line 8 can be merged into line 7. Guess, it's the most boring refactoring in this article:
| def add_auth_details_to_options(options) | |
| return unless @auth_details | |
| if @auth_details.has_key? :access_token | |
| options[:headers] = { | |
| "Authorization" => "Bearer #{@auth_details[:access_token]}" } | |
| elsif @auth_details.has_key?(:api_key) && !options.has_key?(:basic_auth) | |
| options[:basic_auth] = { | |
| :username => @auth_details[:api_key], :password => 'x' } | |
| end | |
| end |
Now, add_auth_details_to_options looks neat and tidy.
Step 6.3: back to define_cs_method
| def define_cs_method(name) | |
| define_method(name) do |*args| | |
| options = {} | |
| if args.size > 1 | |
| options = args[1] | |
| end | |
| args[1] = options | |
| add_auth_details_to_options(options) | |
| handle_response Base.send(name, *args) | |
| end | |
| alias_method "cs_#{name}", name | |
| end |
So, we have a variable number of args at line 2 (see above ↑), but why? It turns out, line 10 calls method name on Base class, meaning, it'll execute Base.get, Base.post, etc. Those are methods from HTTParty, and HTTParty uses .get(*args, &block)-like API, so it's understandable that CreateSend also uses it. Thus, my plan to introduce named arguments is foiled and I have to find another way to improve the code.
From what I know about args (from looking at HTTParty examples), the 1st argument is path and the 2nd argument is options. Lines 3-7 (see above ↑) can be made to better explain arguments they're dealing with:
| def define_cs_method(name) | |
| define_method(name) do |*args| | |
| path, options, *rest = *args | |
| options ||= {} | |
| add_auth_details_to_options(options) | |
| handle_response Base.send(name, path, options, *rest) | |
| end | |
| alias_method "cs_#{name}", name | |
| end |
The code at lines 3-4 (see above ↑) isn't equivalent to the original code. The original code would fail if nil was passed as options (because we wouldn't assign options to be {}). But whether that behaviour was intentional or accidental, I have no idea. So, I rely on the fact that tests still pass.
That's all that's of interest left in the Base class. Hope you enjoyed it.
Happy hacking!
