Refactoring in Ruby

Lately I’ve been giving a lot of thought into not just making something work, but making it cleaner. I’ve been reading Sandi Metz’s Practical Object Oriented Design in Ruby, which although not explicitly about refactoring, seems to have gotten me in the frame of mind to think about it. Part of me thinks this marks a shift for me from level beginner to intermediate, but actually no, because I always want to be a beginner, and also, I’m actually still really a beginner. :)

When thinking about effective teaching at Flatiron (side note: I’m back! It’s awesome), we believe in the idea of “Make it work. Make it right. Make it fast”, a concept coined by Kent Beck back in the day. An effective learning process almost demands this concept: get it working so you can learn how it works, and for the sake of accomplishment. I believe this is paramount to being a junior developer. Before you learn how to be good, you need to just learn how to do it. The good part will come with time, practice, and studying eloquent code (see: POODR).

I’ve been doing just that, and have been thinking about giving more time to refactoring and taking pride in making my code the best it can be (and like everything, it can always be better).

I’ve been working on a simple side project that makes calls to the Github API. I’ve been using the Typhoeus gem for making the requests (it’s a great gem to learn to use, for its versatility). Before refactoring, I had a bunch of methods within a class (or model, as it’s a Rails app) that each work as an API call for something specific (to get an organization, repo, etc).

Here’s what it looked like:

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
32
33
34
35
36
37
38
39
40
41
42

class GithubConnection
  attr_reader :username, :token, :orgs, :repos, :issues

  def initialize(github_data)
    @username = github_data["username"]
    @token = github_data["token"]
  end

  def get_organizations
    request = Typhoeus::Request.new(
      "https://api.github.com/user/orgs",
      headers: {Authorization: "token #{token}"}
    )
    response = request.run
    orgs = JSON.parse(response.body).map do |org|
      org["login"]
    end
  end

  def get_repos(organization)
    request = Typhoeus::Request.new(
      "https://api.github.com/orgs/#{organization}/repos",
      headers: {Authorization: "token #{token}"}
      )
    response = request.run
    repos = JSON.parse(response.body).map do |repo|
      repo["name"]
    end
  end

  def get_issues(organization, repo)
    request = Typhoeus::Request.new(
      "https://api.github.com/#{organization}/#{repo}/issues",
      headers: {Authorization: "token #{token}"}
      )
    response = request.run
    repos = JSON.parse(response.body).map do |issue|
      issue
    end
  end

end

Each method (get_organizations, get_repos, and get_issues) has a specific purpose (which is great! A method should be a single unit of work). But something smells, even just a little bit. I’ve been repeating myself, with my request = Typhoeus::Request.new... and handling the responses via parsing. This breaks the number one rule of clean code: DRY. Don’t Repeat Yourself. Whoops. It’s okay, because these methods get the job done. But I can do a little better.

I began to think about how to fix this. What are the units of functionality that are repeated? What are they doing? And, what are the unique portions of those repeated parts (as in, why was I repeating myself)? How can I abstract out the repetition, and account for the unique parts?

Generally, each method makes an API call. The unique part is what the call is calling to. Each method is taking the response of that call and iterating through to get the data that I want. The unique part of that looping is simply what is being iterated over. I can rewrite the above into a single method that given unique data within its parameters, handles this. Note that I need to pass in an optional key; remember above that when iterating through the JSON object, some of the data I’m extracting is the value in a key:value pair.

1
2
3
4
5
6
7

  def get_data(url, key=nil)
    request = Typhoeus::Request.new(url, headers: {Authorization: "token #{token}"})
    response = request.run
    JSON.parse(response.body).map do |to_parse|
      key ? to_parse[key] : to_parse
    end
  end

This method encapsulates two units of work. This breaks the rule of separation of concerns. If we follow that rule, a method should really only be responsible for one unit of functionality.

With that, it’s a bit clearer how I could further refactor. I can make two methods that are each a unit of functionality that take in some unique information and use that information to return different values. First, I’ll make a method that is a scaffold for some API call, given what I want to call to (which is passed in as parameters).

1
2
3
4

  def make_request(url, header)
    request = Typhoeus::Request.new(url, header)
    response = request.run
  end

Then I’ll have a separate method that handles the response of that call (it calls the make_request method within it), parsing it and returning the data I want, given the API url provided.

1
2
3
4
5

  def json_parse(url, key=nil)
    JSON.parse(make_request(url, headers: {Authorization: "token #{token}"}).body).map do |to_parse|
      key ? to_parse[key] : to_parse
    end
  end

For the sake of my application structure, I’d like to have methods for each of those individual API calls, so I can call them on instances of the class within the controller (this is primarily for clarity on the controller end; @instance.get_organizations is easily understood at first glance).

1
2
3
4
5
6
7
8
9
10
11

  def get_organizations
    orgs = json_parse("https://api.github.com/user/orgs", "login")
  end

  def get_repos(organization)
    repos = json_parse("https://api.github.com/orgs/#{organization}/repos", "name")
  end

  def get_issues(organization, repo)
    issues = json_parse("https://api.github.com/repos/#{organization}/#{repo}/issues")
  end

I could take it farther than this and refactor even more, but this is clear, concise, and has just enough syntactic sugar without being unreadable at first glance to fellow developers. The best refactoring should do this without sacrificing readability.

Comments

Comments