July 18, 2009

Posted by John

Tagged thoughts

Older: MongoMapper, The Rad Mongo Wrapper

Newer: Uploadify and Rails 2.3

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.

6 Comments

  1. Hey John,

    Nice comments. I’m sure Mark appreciated them, and seeing them here is a great look into two coder’s styles, which is a bit unique. Maybe I can throw in a third:

    “I’d maybe abstract them out into a builder method and make them one line calls that just pass on name, verb and block.”

    The piece about a builder method reminds me a bit of the Java jokes about FactoryFactories and build type methods…I haven’t seen the end result, so this is just a bit of curiousity: do you have any sort of method when looking at “WET” code and deciding on what method to use to avoid it?

    The above example could easily be done with a little metaprogramming that loops through a list of symbols and builds the methods right there, and I tend to use this sort of solution more than builder methods as you can easily see the method definition right in front of you. Of course, you can go crazy with this and have a bunch of poorly named variables with “#{var}_thing” type strings all over the place, so there’s definitely a sweet spot.

    Perhaps in the end it come down to all of our unique preferences and styles. I do think thats a bit of a dangerous place with ruby, as you can have JavaRuby, LispRuby, MetaMetaRuby, and CRuby all mixed into projects if they’ve crossed a variety of people’s workstations.

    Anyway, just curious to see what you and others will say. I’m not sure any of us really know how close we are to “oh yeah, obviously that way” or “wtf does that even do”, and seeing a variety of solutions on a problem is always interesting to me.

    Jack

  2. @Jack I only use metaprogramming that defines the methods or class evals stuff when I have to. When I can just put a method in and it doesn’t actually have to be dynamic, I just put the method in. I think it is easier to read code that is calling builder type method than metaprogrammed code. Might just be my personal opinion.

  3. For Weary::Request#method= and friends you mentioned above, it looks like he’s just got a hash of arrays, :get => [all the ways you can specify get]. I’d be inclined to invert the hash, and then instead of a case comparison against arrays, it’s just a hash lookup (‘GET’ => :get, ‘get’ => :get, etc). If the original hash direction is easier to maintain, it’s not a lot of code to add to the end of the hash literal to inject it into the inverse.

  4. @Jamie – Yeah, I would handle it differently too.

  5. This is a great post. I’d love to see more code reviews and such. Very concrete advice and examples and it escapes the sort of… contrived feeling that a tutorial where the code examples and the suggestions are written by the same person. This is real code, so the advice is provably applicable. Great post, John and thanks for letting yourself be critiqued in public, Mark.

  6. @Ben Glad you liked it. I’m thinking of doing this more as a lot of people seemed to enjoy it.

Sorry, comments are closed for this article to ease the burden of pruning spam.

About

Authored by John Nunemaker (Noo-neh-maker), a web developer and programmer who has fallen deeply in love with Ruby. More about John.

Syndication

Feed IconRailsTips Articles - An assortment of howto's and thoughts on Ruby and Rails.

Feed IconRails Quick Tips - Ruby and Rails related links.