bgror

What a Year »

Created at: 01.01.2012 20:00, source: RailsTips - Home, tagged: thoughts

The last 12 months have been nuts. My health and professional/personal life were completely at odds.

Between January and August, I had three hernia surgeries. As if that wasn’t enough for one year, the last few months of the year I’ve been plagued by a few other ailments (which are still giving me a hard time). Definitely a rough stretch. I will never take health for granted again and really look forward to getting back to “normal”.

Quite the contrary to my health, Ordered List grew from 2 to 5 people, helped Zynga launch Words with Friends on Facebook, launched Gauges and Speaker Deck while improving Harmony, and, finally, was acquired by the only other company in the world I wanted to be a part of, GitHub.

Here is to a healthy 2012.

I Have No Talent »

Created at: 12.01.2010 20:00, source: RailsTips - Home, tagged: thoughts

The other day someone sent me an IM and thanked me for my open source contributions. They then said something about wishing they had my gem/code creation talents. I didn’t miss a beat and informed them that I have no talent.

It is true. I have no talent. What I do have is a lot of practice. And I am not talking about occasionally dabbling in Ruby on the weekends. I am talking about the kind of practice where I beat code that isn’t working into submission (though often times the code wins).

The kind of practice where all of a sudden I realize that it is 2am and I’m exhausted physically so I should go to bed, but mentally I feel on fire so I let the code have me for another hour or two (I imagine this state to be like a marathon runner or ironman near the end of their race).

The kind of practice that leads to a GitHub profile stuffed with code I regret (and am embarrassed about, but don’t delete to remind me of where I once was) and code I am proud of (not near as much as I am embarrassed about though).

Intelligence

I am also not very smart. I have a good memory (though my wife will tell you it has some missing pieces) and I work really hard. Really hard. I get that from my dad. He is also not very smart (his words, not mine), with a good memory and works really hard. :)

I am sick of hearing people say, “Oh, I love your code, I wish I could do that.” You can. The only reason you can’t is because you don’t practice enough. I used to think that I wasn’t smart enough. I was jealous of those that did crazy code stuff that I couldn’t even comprehend. Then, one day, I ran into something I did not understand and instead of giving up, I pushed through. I sat there in front of my computer for hours and wrestled with class and class instance variables.

That day was a turning point for me. It was the last time I thought that whether or not I was successful depended on my talent or intelligence. It really comes down to hard work people. Ever since then, I have attacked each thing that I do not understand until I understand it.

I will close with this. I still suck. There are still so many people out there who are far better than I am, but that does not stop me anymore. I do not measure myself against the programming greats, but against those projects on my Github profile from years ago.

Code Review: Weary »

Created at: 18.07.2009 23:29, source: RailsTips - Home, tagged: code review thoughts

In which I provide critique on Mark Wunsh’s new gem 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.

Code Review: Weary »

Created at: 18.07.2009 23:29, source: RailsTips - Home, tagged: code review thoughts

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.

What Is The Simplest Thing That Could Possibly Work? »

Created at: 08.06.2009 21:31, source: RailsTips - Home, tagged: productivity simplicity thoughts

In which I summarize my favorite points from an old, but awesome article.

I am always amazed when I read an article from 2004 and find interesting goodies. I’m probably late to the game on a lot of these articles, as I didn’t really dive into programming as a career until 2005, but I just read The Simplest Thing that Could Possibly Work, a conversation with Ward Cunningham by Bill Venners. The article was published on January 19, 2004, but it is truly timeless.

The Shortest Path

Simplicity is the shortest path to a solution.

“Shortest” doesn’t necessarily refer to lines of code or number of characters, but I see it more as the path that requires the least amount of complexity. As he mentions in the article, if someone releases a 20 page proof to a math problem and then later on, someone releases a 10 page proof for the same problem, the 10 page proof is not necessarily more simple.

The 10 page proof could use some form of mathematics that is not widely used in the community and takes some time to comprehend. This means the 10 page version could be less simple as it requires learning to understand, whereas the 20 page uses generally understood concepts.

I think this is a balance that we always fight with as programmers. What is simple? I can usually say simple or not simple when I look at code, but it is hard to define the rules for simplicity.

Work Today Makes You Better Tomorrow

The effort you expend today to understand the code will make you a more powerful programmer tomorrow.

This is one of the concepts that has made the biggest different in my programming knowledge over the past few years. The first time that I really did this was when I wrote about class and instance variables a few years back. Ever since then, when I come across something that I don’t understand, that I feel I should, I spend the time to understand it. I have grown immensely because of this and would recommend that you do the same if you aren’t already.

Narrow What You Think About

We had been thinking about too much at once, trying to achieve too complicated a goal, trying to code it too well.

This is something that I have been practicing a lot lately. You know how sometimes you just feel overwhelmed and don’t want to start a feature or project? What I’ve found is that when I feel this way it is because I’m trying to think about too much at once.

Ward encourages over and over in the article, think about what is the most simple possible thing that could work. Notice he did not say what is the simplest thing that would work, but rather what could work.

This is something that I’ve noticed recently while pairing with Brandon Keepers. Both of us almost apologize for some of the code we first implement, as we are afraid the other will think that is all we are capable of. What is funny, is that we both realize that you have to start with something and thus never judge. It is far easier to incrementally work towards a brilliant solution than to think it in your head and instantly code it.

Start with a test. Make the test pass. Rinse and repeat. Small, tested changes that solve only the immediate problem at hand always end up with a more simple solution than trying to do it all in one fell swoop. I’ve also found I’m more productive this way as I have less moments of wondering what to do next. The failing test tells me.

Anyway, I thought the article was interesting enough that I would post some of the highlights here and encourage you all to read it. If you know of some oldie, but goodie articles, link them up in the comments below.