CreateSend refactoring part 1

Today I'm going to refactor a part of Ruby library called CreateSend. I'll work on lib/createsend/createsend.rb, which contains CreateSend::CreateSend class and some other stuff.

Introduction

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.

First look

The first things declared are default user agent constant (line 3) and some exceptions.

What's good:

• 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

When reading 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 ResultData, Code and 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:

Step 2.1

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 formatted_result_data instead:

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.

Step 2.2

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 CreateSend::Error.

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 Base.

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.

The result:

Step 4: class methods

There are 3 class methods left to refactor: .authorize_url, .exchange_token and .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 CGI.escape and #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.

The result:

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:

Step 4.2.2

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:

Step 4.2.3

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.

.fail_on_erroneous_response accepts 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 (message and err) for essentially the same thing - error message. err can be changed to full_error_message:

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:

Step 4.2.5

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.:

So, I replaced array with a data clump:

At this point I feel happy about .exchange_token.

Step 4.3: refresh_access_token

.refresh_access_token has the same ailments as .exchange_token:

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!

Happy hacking!

Fed up working on bad code? Here's a way out!

For people that that want to stop suffering from bad code I’ve made a FREE course