Code Review: Weary

Let me start with the fact that I’m not picking on Weary. Mark Wunsch, the author of Weary, emailed me just over a month ago and asked if I could take a look at the code and provide any tips or pointers. I haven’t performed a code review for someone that I don’t know, but I thought what the heck.

I spent about 30 minutes or so looking through his code and typing suggestions into an email. When I was done it was one of the longer emails I’ve written, but I sent it to Mark anyway. He liked the suggestions and has already implemented a few of them so I asked him if I could turn it into a post here. He obliged and you all shall now suffer through it. Muhahahahaha!

I’ll try to post snippets of the code or link to the file before each of my comments (which I’ll cut straight from the email I sent him). Please note that what I suggest are just that, suggestions. They aren’t rules by any means and I’ve been wrong once or twice in my life. Maybe. Let’s get started.

Don’t Repeat Yourself

Weary methods declare, post, put and delete are very similar. I’d maybe abstract them out into a builder method and make them one line calls that just pass on name, verb and block. Below are the methods. You can see the repetition pretty quickly. The only difference between them is the verb (:get, :post, :put, :delete).

module Weary
  def declare(name)
    resource = prepare_resource(name,:get)
    yield resource if block_given?
    form_resource(resource)
    return resource
  end
  alias get declare
  
  def post(name)
    resource = prepare_resource(name,:post)
    yield resource if block_given?
    form_resource(resource)
    return resource
  end
  
  def put(name)
    resource = prepare_resource(name,:put)
    yield resource if block_given?
    form_resource(resource)
    return resource
  end
  
  def delete(name)
    resource = prepare_resource(name,:delete)
    yield resource if block_given?
    form_resource(resource)
    return resource
  end
end

Weary::Request#request is repeating a bit. Each option in the case statement is instantiating a class with a request uri. You could wrap up the class in another method, like request_class or something and then just do request_class.new(@uri.request_uri) in the actual request method. Not sure why I like this, just makes methods smaller and again smaller methods are easier to test.

def request
  prepare = case @http_verb
    when :get
      Net::HTTP::Get.new(@uri.request_uri)
    when :post
      Net::HTTP::Post.new(@uri.request_uri)
    when :put
      Net::HTTP::Put.new(@uri.request_uri)
    when :delete
      Net::HTTP::Delete.new(@uri.request_uri)
    when :head
      Net::HTTP::Head.new(@uri.request_uri)
  end
  prepare.body = options[:body].is_a?(Hash) ? options[:body].to_params : options[:body] if options[:body]
  prepare.basic_auth(options[:basic_auth][:username], options[:basic_auth][:password]) if options[:basic_auth]
  if options[:headers]
    options[:headers].each_pair do |key, value|
      prepare[key] = value
    end
  end
  prepare
end

Weary::Request#method= seems like it is doing a little bit too much work. Maybe I overlooked something but why not just do http_verb.to_s.strip.downcase.intern or something to get the verb? Also, Weary::Resource#via= seems to do the same thing. Maybe you need another class for this logic or a shared method somewhere? You could have something like this:

HTTPVerb.new(http_verb).normalize

HTTPVerb#normalize would then figure out which method to return and could be reused in the places you perform that. Also, you can test it separately and then not worry about testing the different verb mutations in the method= tests.

Here are the two methods I was talking about from the Request and Resource classes.

# Request#method=
def method=(http_verb)
  @http_verb = case http_verb
    when *Methods[:get]
      :get
    when *Methods[:post]
      :post
    when *Methods[:put]
      :put
    when *Methods[:delete]
      :delete
    when *Methods[:head]
      :head
    else
      raise ArgumentError, "Only GET, POST, PUT, DELETE, and HEAD methods are supported"
  end
end

# Resource#via=
def via=(http_verb)
  @via = case http_verb
    when *Methods[:get]
      :get
    when *Methods[:post]
      :post
    when *Methods[:put]
      :put
    when *Methods[:delete]
      :delete
    else
      raise ArgumentError, "#{http_verb} is not a supported method"
  end
end

Weary::Response#format= looks just like Weary::Resource#format=. Same thing as above with the http verbs is what I’d recommend.

# Response#format=
def format=(type)
  @format = case type
    when *ContentTypes[:json]
      :json
    when *ContentTypes[:xml]
      :xml
    when *ContentTypes[:html]
      :html
    when *ContentTypes[:yaml]
      :yaml
    when *ContentTypes[:plain]
      :plain
    else
      nil
  end
end

# Resource#format=
def format=(type)
  type = type.downcase if type.is_a?(String)
  @format = case type
    when *ContentTypes[:json]
      :json
    when *ContentTypes[:xml]
      :xml
    when *ContentTypes[:html]
      :html
    when *ContentTypes[:yaml]
      :yaml
    when *ContentTypes[:plain]
      :plain
    else
      raise ArgumentError, "#{type} is not a recognized format."
  end
end

Break Big Methods into Classes with Tiny Methods

Weary#craft_methods is doing a lot. I understand generally what you are trying to do but without digging in, it is hard to tell. I’d break that out into another class, maybe MethodCrafter. Then, each of those if and unless statements could be moved into their own methods and would be easier to test. MethodCrafter.code could return the code to be eval’d. I use to make long methods, but lately I’ve found breaking them out into classes makes things easier to digest and test.

I have talked about tiny methods before as well. Here is the code for the craft_methods method that I recommended moving to a class.

def craft_methods(resource)
  code = %Q{
    def #{resource.name}(params={})
      options ||= {}
      url = "#{resource.url}"
  }
  if resource.with.is_a?(Hash)
    hash_string = ""
    resource.with.each_pair {|k,v| 
      if k.is_a?(Symbol)
        k_string = ":#{k}"
      else
        k_string = "'#{k}'"
      end
      hash_string << "#{k_string} => '#{v}',"
    }
    code << %Q{
      params = {#{hash_string.chop}}.delete_if {|key,value| value.empty? }.merge(params)
    }
  end
  unless resource.requires.nil?
    if resource.requires.is_a?(Array)
      resource.requires.each do |required|
        code << %Q{  raise ArgumentError, "This resource requires parameter: ':#{required}'" unless params.has_key?(:#{required}) \n}
      end
    else
      resource.requires.each_key do |required|
        code << %Q{  raise ArgumentError, "This resource requires parameter: ':#{required}'" unless params.has_key?(:#{required}) \n}
      end
    end
  end
  unless resource.with.empty?
    if resource.with.is_a?(Array)
      with = %Q{[#{resource.with.collect {|x| x.is_a?(Symbol) ? ":#{x}" : "'#{x}'" }.join(',')}]}
    else
      with = %Q{[#{resource.with.keys.collect {|x| x.is_a?(Symbol) ? ":#{x}" : "'#{x}'"}.join(',')}]}
    end
    code << %Q{ 
      unnecessary = params.keys - #{with} 
      unnecessary.each { |x| params.delete(x) } 
    }
  end
  if resource.via == (:post || :put)
    code << %Q{options[:body] = params unless params.empty? \n}
  else
    code << %Q{
      options[:query] = params unless params.empty?
      url << "?" + options[:query].to_params unless options[:query].nil?
    }
  end
  unless (resource.headers.nil? || resource.headers.empty?)
    header_hash = ""
    resource.headers.each_pair {|k,v|
      header_hash << "'#{k}' => '#{v}',"
    }
    code << %Q{ options[:headers] = {#{header_hash.chop}} \n}
  end
  if resource.authenticates?
    code << %Q{options[:basic_auth] = {:username => "#{@username}", :password => "#{@password}"} \n}
  end
  unless resource.follows_redirects?
    code << %Q{options[:no_follow] = true \n}
  end
  code << %Q{
      Weary::Request.new(url, :#{resource.via}, options).perform
    end
  }
  class_eval code
  return code
end

As you can see, that method bears a heavy burden. Also, the method is actually declared as private which means it is even harder to test (I won’t get into testing private methods right now). If this was broken out into an object, you could heavily unit test that object and then craft_methods could look more like this:

def craft_methods
  code = MethodCrafter.new(resource).to_code
  class_eval code
  return code
end

Unless vs. If

Weary::Resource#with= unless is kind of a brain twister. If you have an else, just use if and reverse the conditionals. I have talked about unless before.

def with=(params)
  if params.is_a?(Hash)
    @requires.each { |key| params[key] = nil unless params.has_key?(key) }
    @with = params
  else
    unless @requires.nil?
      @with = params.collect {|x| x.to_sym} | @requires
    else
      @with = params.collect {|x| x.to_sym}
    end
  end
end

Overall Reactions

So those are the specifics. Now to the more general reactions. You seem to care about your code and that is important. I see a bit of HTTParty in there and I think that is a good call. One of the best ways to learn in coding is to copy. I’ve stolen from lots of projects. :)

As far as the API for weary, I find it a bit over the top. When you are creating a code API that another programmer will use, you have to balance readability and verbosity. on_domain and as_format read nice but could be just as effective named domain and format which saves a few characters, an underscore and having to remember which is on, as, construct, with, and set. Mark took this advice already and changed the API.

I think the method builders that take a block (get, post, etc.) are interesting and I’m sure you learned a lot creating the project, which is the most important thing. I’m betting some people will like this better than HTTParty as everyone has different brains. Great work.

Conclusion

I found reviewing the code fun and was surprised by how many comments I had for Mark. I guess I have messed up a lot over the years and that has given me an opinion on this stuff. Hope others find it helpful. Let me know if you would like to see more posts like this.