validates_length_of byte counting gotcha

Watch out for validates_length_of if you need to make sure a string is a certain number of bytes long. For example, SMS messages can be no longer than 160 bytes in length. I recently got bit by this because some unicode “curly” quotes slipped into a reply message, but they weren’t detected by the validation.

Here’s the problem.

Consider this string:

str = "€"

It is 3 bytes long:

str.size => 3

However, ActiveRecord’s validates_length_of records this as only one character, because it uses str.split(//).size to measure the size.

If you NEED to be certain that a string is less than a certain number of bytes, you’ll need to override the default behavior of validate_length_of.

Fortunately, you can supply your own tokenizer, which makes this easy. The tokenizer is called, and size is called on its return value to find out how many tokens there are. Since String responds to size which returns the number of bytes, you can simply return the attribute value itself as the tokenizer, like this:

validates_length_of :message, :maximum => 160, :tokenizer => lambda { |str| str }

Will this still work in Ruby 1.9? I’m not sure. I now have a test case which will warn me if it doesn’t…

Rails 3: The Great Decoupling

In working on Rails 3 over the past 6 months, I have focsed rather extensively on decoupling components from each other.

Why should ActionController care whether it’s talking to ActionView or just something that duck-types like ActionView? Of course, the key to making this work well is to keep the interfaces between components as small as possible, so that implementing an ActionView lookalike is a matter of implementing just a few methods, not dozens.

While I was preparing for my talk at RubyKaigi, I was trying to find the smallest possible examples that demonstrate some of this stuff. It went really well, but I noticed a few areas that could be improved even further, producing an even more compelling demonstration.

This weekend, I focused on cleaning up those interfaces, so we have small and clearly documented mechanisms for interfacing with Rails components. I want to focus on ActionView in this post, which I’ll demonstrate with an example.

$:.push "rails/activesupport/lib"
$:.push "rails/actionpack/lib"
 
require "action_controller"
 
class Kaigi < ActionController::Http
  include AbstractController::Callbacks
  include ActionController::RackConvenience
  include ActionController::Renderer
  include ActionController::Layouts
  include ActionView::Context
 
  before_filter :set_name
  append_view_path "views"
 
  def _action_view
    self
  end
 
  def controller
    self
  end
 
  DEFAULT_LAYOUT = Object.new.tap {|l| def l.render(*) yield end }
 
  def _render_template_from_controller(template, layout = DEFAULT_LAYOUT, options = {}, partial = false)
    ret = template.render(self, {})
    layout.render(self, {}) { ret }
  end
 
  def index
    render :template => "template"
  end
 
  def alt
    render :template => "template", :layout => "alt"
  end
 
  private
  def set_name
    @name = params[:name]
  end
end
 
app = Rack::Builder.new do
  map("/kaigi") {  run Kaigi.action(:index) }
  map("/kaigi/alt") { run Kaigi.action(:alt) }
end.to_app
 
Rack::Handler::Mongrel.run app, :Port => 3000

There’s a bunch going on here, but the important thing is that you can run this file with just ruby, and it’ll serve up /kaigi and /kaigi/alt. It will serve templates from the local “/views” directory, and correctly handle before filters just fine.

Let’s look at this a piece at a time:

$:.push "rails/activesupport/lib"
$:.push "rails/actionpack/lib"
 
require "action_controller"

This is just boilerplace. I symlinked rails to a directory under this file and required action_controller. Note that simply requiring ActionController is extremely cheap — no features have been used yet

class Kaigi < ActionController::Http
  include AbstractController::Callbacks
  include ActionController::RackConvenience
  include ActionController::Renderer
  include ActionController::Layouts
  include ActionView::Context
end

I inherited my class from ActionController::Http. I then included a number of features, include Rack convenience methods (request/response), the Renderer, and Layouts. I also made the controller itself the view context. I will discuss this more in just a moment.

  before_filter :set_name

This is the normal Rail before_filter. I didn’t need to do anything else to get this functionality other than include AbstractController::Callbacks

  append_view_path "views"

Because we’re not in a Rails app, our view paths haven’t been pre-populated. No problem: it’s just a one-liner to set them ourselves.

The next part is the interesting part. In Rails 3, while ActionView::Base remains the default view context, the interface between ActionController and ActionView is extremely well defined. Specifically:

• A view context must include ActionView::Context. This just adds the compiled templates, so they can be called from the context
• A view context must provide a _render_template_from_controller method, which takes a template object, a layout, and additional options
• A view context may optionally also provide a _render_partial_from_controller, to handle render :partial => @some_object
• In order to use ActionView::Helpers, a view context must have a pointer back to its original controller

That’s it! That’s the entire ActionController<=>ActionView interface.

  def _action_view
    self
  end
 
  def controller
    self
  end

Here, we specify that the view context is just self, and define controller, required by view contexts. Effectively, we have merged the controller and view context (mainly just to see if it could be done )

  DEFAULT_LAYOUT = Object.new.tap {|l| def l.render(*) yield end }

Next, we make a default layout. This is just a simple proc that provides a render method that yields to the block. It will simplify:

  def _render_template_from_controller(template, layout = DEFAULT_LAYOUT, options = {}, partial = false)
    ret = template.render(self, {})
    layout.render(self, {}) { ret }
  end

Here, we supply the required _render_template_from_controller. The template object that is passed in is a standard Rails Template which has a render method on it. That method takes the view context and any locals. For this example, we pass in self as the view context, and do not provide any locals. Next, we call render on the layout, passing in the return value of template.render. The reason we created a default is to make the case of a layout identical to the case without.

  def index
    render :template => "template"
  end
 
  def alt
    render :template => "template", :layout => "alt"
  end
 
  private
  def set_name
    @name = params[:name]
  end

This is a standard Rails controller.

app = Rack::Builder.new do
  map("/kaigi") {  run Kaigi.action(:index) }
  map("/kaigi/alt") { run Kaigi.action(:alt) }
end.to_app
 
Rack::Handler::Mongrel.run app, :Port => 3000

Finally, rather than use the Rails router, we just wire the controller up directly using Rack. In Rails 3, ControllerName.action(:action_name) returns a rack-compatible endpoint, so we can wire them up directly.

And that’s all there is to it!

Note: I’m not sure if I still need to say this, but stuff like this is purely a demonstration of the power of the internals, and does not reflect changes to the public API or the way people use Rails by default. Everyone on the Rails team is strongly committed to retaining the same excellent startup experience and set of good conventional defaults. That will not be changing in 3.0.

Rails 3: The Great Decoupling

In working on Rails 3 over the past 6 months, I have focsed rather extensively on decoupling components from each other.

Why should ActionController care whether it’s talking to ActionView or just something that duck-types like ActionView? Of course, the key to making this work well is to keep the interfaces between components as small as possible, so that implementing an ActionView lookalike is a matter of implementing just a few methods, not dozens.

While I was preparing for my talk at RubyKaigi, I was trying to find the smallest possible examples that demonstrate some of this stuff. It went really well, but I noticed a few areas that could be improved even further, producing an even more compelling demonstration.

This weekend, I focused on cleaning up those interfaces, so we have small and clearly documented mechanisms for interfacing with Rails components. I want to focus on ActionView in this post, which I’ll demonstrate with an example.

$:.push "rails/activesupport/lib"
$:.push "rails/actionpack/lib"
 
require "action_controller"
 
class Kaigi < ActionController::Http
  include AbstractController::Callbacks
  include ActionController::RackConvenience
  include ActionController::Renderer
  include ActionController::Layouts
  include ActionView::Context
 
  before_filter :set_name
  append_view_path "views"
 
  def _action_view
    self
  end
 
  def controller
    self
  end
 
  DEFAULT_LAYOUT = Object.new.tap {|l| def l.render(*) yield end }
 
  def _render_template_from_controller(template, layout = DEFAULT_LAYOUT, options = {}, partial = false)
    ret = template.render(self, {})
    layout.render(self, {}) { ret }
  end
 
  def index
    render :template => "template"
  end
 
  def alt
    render :template => "template", :layout => "alt"
  end
 
  private
  def set_name
    @name = params[:name]
  end
end
 
app = Rack::Builder.new do
  map("/kaigi") {  run Kaigi.action(:index) }
  map("/kaigi/alt") { run Kaigi.action(:alt) }
end.to_app
 
Rack::Handler::Mongrel.run app, :Port => 3000

There’s a bunch going on here, but the important thing is that you can run this file with just ruby, and it’ll serve up /kaigi and /kaigi/alt. It will serve templates from the local “/views” directory, and correctly handle before filters just fine.

Let’s look at this a piece at a time:

$:.push "rails/activesupport/lib"
$:.push "rails/actionpack/lib"
 
require "action_controller"

This is just boilerplace. I symlinked rails to a directory under this file and required action_controller. Note that simply requiring ActionController is extremely cheap — no features have been used yet

class Kaigi < ActionController::Http
  include AbstractController::Callbacks
  include ActionController::RackConvenience
  include ActionController::Renderer
  include ActionController::Layouts
  include ActionView::Context
end

I inherited my class from ActionController::Http. I then included a number of features, include Rack convenience methods (request/response), the Renderer, and Layouts. I also made the controller itself the view context. I will discuss this more in just a moment.

  before_filter :set_name

This is the normal Rail before_filter. I didn’t need to do anything else to get this functionality other than include AbstractController::Callbacks

  append_view_path "views"

Because we’re not in a Rails app, our view paths haven’t been pre-populated. No problem: it’s just a one-liner to set them ourselves.

The next part is the interesting part. In Rails 3, while ActionView::Base remains the default view context, the interface between ActionController and ActionView is extremely well defined. Specifically:

• A view context must include ActionView::Context. This just adds the compiled templates, so they can be called from the context
• A view context must provide a _render_template_from_controller method, which takes a template object, a layout, and additional options
• A view context may optionally also provide a _render_partial_from_controller, to handle render :partial => @some_object
• In order to use ActionView::Helpers, a view context must have a pointer back to its original controller

That’s it! That’s the entire ActionController<=>ActionView interface.

  def _action_view
    self
  end
 
  def controller
    self
  end

Here, we specify that the view context is just self, and define controller, required by view contexts. Effectively, we have merged the controller and view context (mainly just to see if it could be done )

  DEFAULT_LAYOUT = Object.new.tap {|l| def l.render(*) yield end }

Next, we make a default layout. This is just a simple proc that provides a render method that yields to the block. It will simplify:

  def _render_template_from_controller(template, layout = DEFAULT_LAYOUT, options = {}, partial = false)
    ret = template.render(self, {})
    layout.render(self, {}) { ret }
  end

Here, we supply the required _render_template_from_controller. The template object that is passed in is a standard Rails Template which has a render method on it. That method takes the view context and any locals. For this example, we pass in self as the view context, and do not provide any locals. Next, we call render on the layout, passing in the return value of template.render. The reason we created a default is to make the case of a layout identical to the case without.

  def index
    render :template => "template"
  end
 
  def alt
    render :template => "template", :layout => "alt"
  end
 
  private
  def set_name
    @name = params[:name]
  end

This is a standard Rails controller.

app = Rack::Builder.new do
  map("/kaigi") {  run Kaigi.action(:index) }
  map("/kaigi/alt") { run Kaigi.action(:alt) }
end.to_app
 
Rack::Handler::Mongrel.run app, :Port => 3000

Finally, rather than use the Rails router, we just wire the controller up directly using Rack. In Rails 3, ControllerName.action(:action_name) returns a rack-compatible endpoint, so we can wire them up directly.

And that’s all there is to it!

Note: I’m not sure if I still need to say this, but stuff like this is purely a demonstration of the power of the internals, and does not reflect changes to the public API or the way people use Rails by default. Everyone on the Rails team is strongly committed to retaining the same excellent startup experience and set of good conventional defaults. That will not be changing in 3.0.

My Apprenticeship - Monday, July 19, 2004

This summer I'm revisiting my short apprenticeship at Object Mentor. I'll be posting commentary on all my posts from the summer of 2004 exactly 5 years later to the day.

Weird that I just had dinner with David (Chelimsky) and his lovely lady friend Flor and now I'm writing about him teaching me a class. David and I actually live in the same condo complex in Rogers Park. This happened more or less by accident. David currently works for Articulated Man and I for Obtiva -- we've never worked together professionally but have remained friends.

Did you see that part about how you're not supposed to run the install.php file through the command line but by hitting it through a web-browser? I didn't know at the time that Apache would just figure the rest out and the people who wrote the instruction manual probably didn't think they had to point out something so basic. A common problem when I was getting into software was that no explanation was basic enough for the likes of me.

Weekly Digest, 7-19-09

Fever and the Future of Feed Readers

I’m not sure what the solution is here. Feed readers as we’ve known them are dying, but it’s as yet unclear what will take their place. Filtering feeds for relevance algorithmically seems all but fruitless; filtering through the social graph is only a slight improvement, but misses the rare content that may only strike a chord with a small audience.

The Journey to Ruby 1.9

Here’s some tips and tricks for those who want to upgrade their own Ruby install and have their gems to be compatible.

Big Old Rails Template from A Fresh Cup

Here’s a list of what this template sets up...

Don’t offer $50 for your favorite feature

I bet every Mac and iPhone developer (and probably some Windows developers too) has heard this at least once, if not dozens of times, from someone who uses their software: “I will PayPal you $50 right now if you will add this feature for me.”

Structural Tags in HTML5

The HTML5 specification has added quite a few interesting and useful tags for structuring your markup. For a majority of everyday uses, these tags will replace many of our typical div entries from our code. So let’s dig in.

Five Features from Mercurial That Would Make Git Suck Less

This isn’t an attempt to convince you to use Mercurial exclusively. And I’m intentionally skipping any mention of Mercurial’s shortcomings. I want to see these features in upcoming versions of Git.

Serious doubts

I’ve never doubted the viability of running a serious business of writing iPhone apps before. For the first time, now, I am. [The developer of Instapaper on the state of the iPhone App Store.]

rails admin interface roundup

...I evaluated four plugins for admin UI...

Wolfram Alpha and hubristic user interfaces

Strangely, to the developers of intelligent control interfaces, these interfaces appear to work perfectly well. Moreover, when the developers demo these interfaces, the demo comes off without a hitch - and is often quite impressive. This is not the normal result of broken software. This "demo illusion" convinces the developers that the product is ready to ship, although it is not and will never be ready to ship.

Gah! Up is Down! Right is Wrong! Make it Stop!

I'm not a fan of the GPL quite simply because I don't see the GPL as "open". The GPL is not defined by what it is, it's defined by what it isn't. It's "against" proprietary closed source code. It's against corporations. It's against software as a commercial product... Knowledge is expanded when it is shared. When solutions to problems are shared, that frees us up to tackle the next obstacle rather than spending time solving problems that have already been solved by others... If you truly believe that knowledge is not a zero-sum game, and that sharing knowledge tends to increase the sum of societal knowledge, then you don't go putting petty restrictions on your knowledge.

There are no small changes

Adding a plugin to a codebase is easy. Integrating a new feature within an existing application is not. When you’re striving for quality there are no small changes.

How Capitalism Saves Ruby from Corporatism

...or... Owning the Means of Production. This talk was given at FutureRuby in Toronto, Canada in the summer of 2009.

Putting What Little We Actually Know About Chrome OS Into Context

I’m skeptical about the prospects of any new system or product that isn’t intended for use by the people creating it. Gmail, for example, is the best web mail system because it was designed to be used not just by “typical” users but by expert users, including the engineers at Google who made it.

One of the Most Time Consuming Startup Roadblocks

So take a risk this month: outsource your first task and see where it takes you. When was the last time a single tool or work habit offered the opportunity to save 20-60 hours in a month?

Paperclip file rename

While developing an application with Sebastián that allow users to upload videos with some file name restrictions, meaning that it must contain only A-Z and 0-9 digits, underscores (_) as a valid component as well, and also the name must be preceded by it’s own #id, we came up with the need of applying this custom filter to each uploaded video.

YouTube Will Be Next To Kiss IE6 Support Goodbye

Judging by this screenshot taken by an IE6 user who was watching some videos on YouTube, it appears the Google company will be phasing out support for the browser shortly. [Die!]

Hacker News on Zed Shaw: Why I (A/L)GPL

[Interesting comments, as usual. My take: the GPL is like half-assed open-source. Sure, you can see the source, but it's not really open.]

Thinking Sphinx RSpec Matchers

How do you use RSpec to drive the design of your models that will use Thinking Sphinx for search? Say you’re already using Cucumber for integration tests to verify your index builds correctly and searches return the results you expect. For your models’ specs, you’ll want something that is lighter but doesn’t sacrifice your overall test coverage.

To achieve this, I wrote a couple of small RSpec matchers that inspect the Thinking Sphinx index object on your model to ensure that it contains the fields and attributes that you expect.

Spec::Matchers.define(:index) do |*field_names|
  description do
    "have a search index for #{field_names.join('.')}"
  end
  match do |model|
    all_fields = field_names.dup
    first_field = all_fields.pop

    model.sphinx_indexes.first.fields.select { |field|
      field.columns.length == 1 && 
        field.columns.first.__stack == all_fields.map { |s| s.to_sym } && 
        field.columns.first.__name == first_field.to_sym
    }.length == 1
  end
end

Spec::Matchers.define(:have_attribute) do |*attr_names|
  description do
    "have a search attribute for #{attr_names.join('.')}"
  end
  match do |model|
    all_attrs = attr_names.dup
    first_attr = all_attrs.pop

    model.sphinx_indexes.first.attributes.select { |attr|
      attr.columns.length == 1 && 
        attr.columns.first.__stack == all_attrs.map { |s| s.to_sym } && 
        attr.columns.first.__name == first_attr.to_sym
    }.length == 1
  end
end

Put these matchers in your spec_helper.rb or somewhere else handy, and then you can use them in your model specs:

describe Question do
  it { should index(:topic) }
  it { should have_attribute(:state) }
end

They read quite nicely in the single-line format above, and the matchers provide a readable description when you run the spec:

Question
- should have a search index for topic
- should have a search attribute for legacy_mastery

While these matchers work well for me, I feel that sphinx_indexes is perhaps an object I should leave alone, and not something I can rely on having continued access to. Please leave a comment if you have any suggestions for doing this more cleanly!

What I did learn, however, is how simple it was to write custom matchers for RSpec. If you haven’t tried it before, I strongly suggest you give it a go! RSpec’s matcher DSL is straightforward, and the documentation has everything you need to get started.

Thinking Sphinx RSpec Matchers

How do you use RSpec to drive the design of your models that will use Thinking Sphinx for search? Say you’re already using Cucumber for integration tests to verify your index builds correctly and searches return the results you expect. For your models’ specs, you’ll want something that is lighter but doesn’t sacrifice your overall test coverage.

To achieve this, I wrote a couple of small RSpec matchers that inspect the Thinking Sphinx index object on your model to ensure that it contains the fields and attributes that you expect.

Spec::Matchers.define(:index) do |*field_names|
  description do
    "have a search index for #{field_names.join('.')}"
  end
  match do |model|
    all_fields = field_names.dup
    first_field = all_fields.pop

    model.sphinx_indexes.first.fields.select { |field|
      field.columns.length == 1 && 
        field.columns.first.__stack == all_fields.map { |s| s.to_sym } && 
        field.columns.first.__name == first_field.to_sym
    }.length == 1
  end
end

Spec::Matchers.define(:have_attribute) do |*attr_names|
  description do
    "have a search attribute for #{attr_names.join('.')}"
  end
  match do |model|
    all_attrs = attr_names.dup
    first_attr = all_attrs.pop

    model.sphinx_indexes.first.attributes.select { |attr|
      attr.columns.length == 1 && 
        attr.columns.first.__stack == all_attrs.map { |s| s.to_sym } && 
        attr.columns.first.__name == first_attr.to_sym
    }.length == 1
  end
end

Put these matchers in your spec_helper.rb or somewhere else handy, and then you can use them in your model specs:

describe Question do
  it { should index(:topic) }
  it { should have_attribute(:state) }
end

They read quite nicely in the single-line format above, and the matchers provide a readable description when you run the spec:

Question
- should have a search index for topic
- should have a search attribute for legacy_mastery

While these matchers work well for me, I feel that sphinx_indexes is perhaps an object I should leave alone, and not something I can rely on having continued access to. Please leave a comment if you have any suggestions for doing this more cleanly!

What I did learn, however, is how simple it was to write custom matchers for RSpec. If you haven’t tried it before, I strongly suggest you give it a go! RSpec’s matcher DSL is straightforward, and the documentation has everything you need to get started.

Weekly Digest, 7-19-09

Fever and the Future of Feed Readers

I’m not sure what the solution is here. Feed readers as we’ve known them are dying, but it’s as yet unclear what will take their place. Filtering feeds for relevance algorithmically seems all but fruitless; filtering through the social graph is only a slight improvement, but misses the rare content that may only strike a chord with a small audience.

The Journey to Ruby 1.9

Here’s some tips and tricks for those who want to upgrade their own Ruby install and have their gems to be compatible.

Big Old Rails Template from A Fresh Cup

Here’s a list of what this template sets up...

Don’t offer $50 for your favorite feature

I bet every Mac and iPhone developer (and probably some Windows developers too) has heard this at least once, if not dozens of times, from someone who uses their software: “I will PayPal you $50 right now if you will add this feature for me.”

Structural Tags in HTML5

The HTML5 specification has added quite a few interesting and useful tags for structuring your markup. For a majority of everyday uses, these tags will replace many of our typical div entries from our code. So let’s dig in.

Five Features from Mercurial That Would Make Git Suck Less

This isn’t an attempt to convince you to use Mercurial exclusively. And I’m intentionally skipping any mention of Mercurial’s shortcomings. I want to see these features in upcoming versions of Git.

Serious doubts

I’ve never doubted the viability of running a serious business of writing iPhone apps before. For the first time, now, I am. [The developer of Instapaper on the state of the iPhone App Store.]

rails admin interface roundup

...I evaluated four plugins for admin UI...

Wolfram Alpha and hubristic user interfaces

Strangely, to the developers of intelligent control interfaces, these interfaces appear to work perfectly well. Moreover, when the developers demo these interfaces, the demo comes off without a hitch - and is often quite impressive. This is not the normal result of broken software. This "demo illusion" convinces the developers that the product is ready to ship, although it is not and will never be ready to ship.

Gah! Up is Down! Right is Wrong! Make it Stop!

I'm not a fan of the GPL quite simply because I don't see the GPL as "open". The GPL is not defined by what it is, it's defined by what it isn't. It's "against" proprietary closed source code. It's against corporations. It's against software as a commercial product... Knowledge is expanded when it is shared. When solutions to problems are shared, that frees us up to tackle the next obstacle rather than spending time solving problems that have already been solved by others... If you truly believe that knowledge is not a zero-sum game, and that sharing knowledge tends to increase the sum of societal knowledge, then you don't go putting petty restrictions on your knowledge.

There are no small changes

Adding a plugin to a codebase is easy. Integrating a new feature within an existing application is not. When you’re striving for quality there are no small changes.

How Capitalism Saves Ruby from Corporatism

...or... Owning the Means of Production. This talk was given at FutureRuby in Toronto, Canada in the summer of 2009.

Putting What Little We Actually Know About Chrome OS Into Context

I’m skeptical about the prospects of any new system or product that isn’t intended for use by the people creating it. Gmail, for example, is the best web mail system because it was designed to be used not just by “typical” users but by expert users, including the engineers at Google who made it.

One of the Most Time Consuming Startup Roadblocks

So take a risk this month: outsource your first task and see where it takes you. When was the last time a single tool or work habit offered the opportunity to save 20-60 hours in a month?

Paperclip file rename

While developing an application with Sebastián that allow users to upload videos with some file name restrictions, meaning that it must contain only A-Z and 0-9 digits, underscores (_) as a valid component as well, and also the name must be preceded by it’s own #id, we came up with the need of applying this custom filter to each uploaded video.

YouTube Will Be Next To Kiss IE6 Support Goodbye

Judging by this screenshot taken by an IE6 user who was watching some videos on YouTube, it appears the Google company will be phasing out support for the browser shortly. [Die!]

Hacker News on Zed Shaw: Why I (A/L)GPL

[Interesting comments, as usual. My take: the GPL is like half-assed open-source. Sure, you can see the source, but it's not really open.]

Mac and Ruby Roundup: MacRuby AOT, DMGs with ChocTop

MacRuby is steadily moving forward, with a usable Ahead of Time (AOT) compiler coming closer on the experimental branch, which should make Ruby a first class language for Cocoa applications. Also: a look at Dr Nic's ChocTop utility for creating MacOS DMG files. By Werner Schuster

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.

Code Review: Weary

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

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.

Interview:Cameron Purdy on Scaling Out Data Grids

What is Data Grid computing? What makes it different from a database? Is a data grid always scalable? Is the cloud the next step? Cameron Purdy answered these questions and others during an InfoQ interview, and also gave some hints on how to build scalable grids and how to avoid horror stories. By Cameron Purdy

What do we need to get on Ruby 1.9?

A year ago, I was very skeptical of Ruby 1.9. There were a lot of changes in it, and it seemed like it was going to be a mammoth job to get things running on it. The benefits did not seem to outweigh the costs of switching, especially since Ruby 1.9 was not yet adequately stable to justify the big switch.

At this point, however, it seems as though Ruby 1.9 has stabilized (with 1.9.2 on the horizon), and there are some benefits that seem to obviously justify a switch (such as fast, integrated I18n, better performance in general, blocks that can have default arguments and take blocks, etc.).

Perhaps more importantly though, Ruby’s language implementors have shifted their focus to Ruby 1.9. It has become increasingly difficult to get enhancements in Ruby 1.8, because it is no longer trunk Ruby. Getting community momentum behind Ruby 1.9 would enable us to make productive suggestions to Matz and the other language implementors. Instead, we seem to get a new monthly patch fixing Ruby 1.8.

So my question is: what do we as a community need to shift momentum to 1.9. I’m don’t want a generic answer, like “we need to feel good about it”. I’m asking you what is stopping you today from using Ruby 1.9 for your next project. Is there a library that doesn’t work? Is there a new language feature that causes so much disruption to your existing programming patterns to make a switch untenable?

I suspect that we are all just comfortable in Ruby 1.8, but would actually be mostly fine upgrading to Ruby 1.9. I also suspect that there are small issues I’m not personally aware of, but which have blocked some of you from upgrading. Rails 2.3 and 3.0 (edge) work fine on Ruby 1.9, and I’d like to see what we can do to make Ruby 1.9 a good recommended option for new projects.

Thoughts?

What do we need to get on Ruby 1.9?

A year ago, I was very skeptical of Ruby 1.9. There were a lot of changes in it, and it seemed like it was going to be a mammoth job to get things running on it. The benefits did not seem to outweigh the costs of switching, especially since Ruby 1.9 was not yet adequately stable to justify the big switch.

At this point, however, it seems as though Ruby 1.9 has stabilized (with 1.9.2 on the horizon), and there are some benefits that seem to obviously justify a switch (such as fast, integrated I18n, better performance in general, blocks that can have default arguments and take blocks, etc.).

Perhaps more importantly though, Ruby’s language implementors have shifted their focus to Ruby 1.9. It has become increasingly difficult to get enhancements in Ruby 1.8, because it is no longer trunk Ruby. Getting community momentum behind Ruby 1.9 would enable us to make productive suggestions to Matz and the other language implementors. Instead, we seem to get a new monthly patch fixing Ruby 1.8.

So my question is: what do we as a community need to shift momentum to 1.9. I’m don’t want a generic answer, like “we need to feel good about it”. I’m asking you what is stopping you today from using Ruby 1.9 for your next project. Is there a library that doesn’t work? Is there a new language feature that causes so much disruption to your existing programming patterns to make a switch untenable?

I suspect that we are all just comfortable in Ruby 1.8, but would actually be mostly fine upgrading to Ruby 1.9. I also suspect that there are small issues I’m not personally aware of, but which have blocked some of you from upgrading. Rails 2.3 and 3.0 (edge) work fine on Ruby 1.9, and I’d like to see what we can do to make Ruby 1.9 a good recommended option for new projects.

Thoughts?