CreateSend is a base class for accessing CampaignMonitor API. It provies
.user_agent to set HTTP user agent and
.exchange_token to get OAuth access token.
In summary, it’s a Ruby wrapper for accessing API. Classes like
Campaign inherit from
CreateSend to add specific methods to work with campaigns, etc.
The first things declared are default user agent constant (line 3) and some exceptions.
- long string split into two (lines 12-13), instead of having overly long string.
- thorough comments for error classes, detailing which HTTP error codes they represent (lines 17-26)
Step 1: concise public interface
CreateSendError class (lines 6-15 ↑) I notice that there’s too much information there that I don’t really need to know (lines 10-13 ↑). I believe, formatting of error message can be moved to a private method, and people reading the code after that can just skip any private methods, because they aren’t essential for understanding.
So, I extract method:
Step 2: minor fixes
The comment on line 13 (see above ↑) doesn’t really add anything to understanding. It’s clear that the code references
Message, so the comment just repeats what code tells. It is safe to remove it.
On line 14 (see above ↑),
extra gets assigned an empty string if
ResultData isn’t present.
nil interpolated into string will yield a
"" as well, so there’s no need to assign it.
Here’s the result:
On line 2 (see above ↑),
extra variable is used to store extra result data. It’s kinda easy to remember what’s stored in the variable, since it’s used on the next line (and we could have used
a as the variable name). But I want to make it not necessary to remember at all!
What we really have here is not
extra, but result data, to be precise, formatted result data. So, I’m going to use
I realise that that
formatted isn’t ideal name for variable contents concantenated with description. If you have a better idea, please leave it in the comments.
Taking one last look at the
CreateSendError class, I realise that it’s not DRY, as it’s inside
CreateSend module, which makes it
CreateSend::CreateSendError. Thus, I’m going to make it
Step 3: introducing CreateSend class
Take a look:
We can see that the class uses a HTTP library HTTParty, has some notion of authentication, uses a certificate and can set HTTP user agent.
First thing I don’t like about it, is that it’s
CreateSend::CreateSend class, not DRY at all. From the class comment and from looking at other code, I know that this class is used as a base (for example,
Subscriber class is a descendent of it). So, it seems it’d be better to call it
Next up, on line 7 is certificate setup. Too much going on that I don’t need to know. Thus, I’m going to move that code to a separate module.
Here’s the result:
Step 3.1: setting user agent
Take a look:
.user_agent sets up
User-Agent HTTP header. It accepts a single argument, and if it’s falsey, the default user agent is used instead.
user_agent nil or
user_agent false is a pretty obscure way to say “please present yourself as Createsend to HTTP servers”. I think
.default_user_agent expresses it better.
Step 4: class methods
There are 3 class methods left to refactor:
.refresh_access_token. They all have to do with OAuth. Even though I’ve never had to deal with OAuth, I’ll have no problem refactoring them.
Step 4.1: authorize_url method
authorize_url sounds like it authorizes something, but in fact, it just returns an URL constructed from its arguments. Take a look:
At the very least, the method name should be
authorization_url, at most
construct_authorization_url. I’ll opt for
construct_authorization_url as the most descriptive.
On line 2 (see above ↑), the comment repeats argument list. It is redundant information that only leads to longer reading time and doesn’t add anything to understanding. After removing that, I ended up with “Construct authorization URL for your application”, and that essentially repeats method name
construct_authorization_url. So, I’ve removed the comment altogeter.
Next, there’s a lot of duplication with
#to_s. It turns out, there’s a Hash#to_query in ActiveSupport that can build a HTTP query string. So I’m going to use that.
I would like to simplify it further, by treating arguments not as named attributes, but as an array, but that’d probably be an overkill.
Step 4.2: exchange_token
exchange_token does something OAuth-related. It constructs ULR parameters, makes a call to HTTP API and returns result (raising exception on failure). Take a look:
Lines 4-8 (see above ↑) construct URL parameters, same thing as in the step 4.1, I’m going to use
Hash#to_query here as well:
Line 9 (see above ↑) is used to add
options variable, used on the next line. I think it makes code harder to read. If I inline it into line 10, it won’t be required to read it, which is a win:
It’s still hard to read, so I’m going to extract some methods:
.request_token constructs URL and makes a HTTP call, returning a hash. Since I’ve already refactored the code that comprises it, there’s nothing to change there.
message argument and raises an exception on erroneous response. One reason to have
message argument is to know the error message inside of
.exchange_token as it helps to understand what’s going on.
Step 4.2.4: fail_on_erroneous_response
At this point (lines 12-18 above ↑) we have two variables (
err) for essentially the same thing – error message.
err can be changed to
I still find it hard to read, the code tells how it does its job, instead of expressing intent. Thus, I’ll use extract method again:
After refactoring extracted parts of
.exchange_token, there’s still that part where it returns the result (line 6):
I dislike to use such APIs because I have to remember what it returns before I need to use it. E.g.:
access_token, expires_in, refresh_token = Base.exchange_token(...)
So, I replaced array with a data clump:
At this point I feel happy about
Step 4.3: refresh_access_token
.refresh_access_token has the same ailments as
And is easy to fix in one go:
There’s a lot left to be refactored in
Base class, and I will do it in part 2!