Why Not To Use My Library clj-record

In 2008 I discovered Clojure, and I was extremely excited to finally have an easy entry-way to Lisp. Pretty early on I wanted to get my head around one of Lisp's killer features: macros. A little playing around in the REPL was easy but not satisfying: I wanted to build something powerful.

Having spent a few years doing Ruby on Rails, I thought ActiveRecord's macro-like capabilities (has_many and whatnot) would be a good goal. So I built clj-record.

In retrospect, I went too far trying to build in Rails-like magic, so I want to point out the design flaws that I recommend you avoid in your own work and recommend that you choose something other than clj-record for your Clojure projects that use an RDBMS.

The API of clj-record is all in one macro: clj-record.core/init-model. It commits a few macro sins.

• It looks at the namespace from which you call it to find the name of your model.
• It intentionally captures a db var from your namespace.
• It defs a bunch of fns in your namespace (most of which are just partially applied versions of clj-record.core fns).

So you do this:

and that turns into something like this (leaving out the ns):

This provided really good practice (and an example I've gone back to more than once) for quoting, syntax quoting, unquoting, and unquote-splicing.

But this sucks!

What init-model should do is return a data-structure that you then pass to clj-record's find, insert, and update fns. In addition to being simpler to understand than the hidden registry of model metadata that clj-record has to maintain, it would make it more natural to write generic, composable code, since the model itself would be a value.

If that were all init-model needed to do, the code (both inside clj-record and user code) would probably be even simpler if there were no macro at all. A set of functions could enhance the model data-structure as needed so that even model setup code could be composed like any other series of fn-calls.

Maybe it would be worthwhile having a macro that went something like this:

This macro would do a def, but it's completely intuitive what that def is. The third "sin" on my list above was about defining a bunch of fns in your namespace. I call this a sin because it's not clear from reading the code that there are a bunch of vars defined, making both discovery and debugging more difficult. There may be cases where "hidden defs" are justified by performance gains or code reduction, but the fns defined by init-model don't pass that test (because (clj-record.core/find-records widget attributes) is no worse and more easily composed with other generic model-processing fns than (my.model.widget/find-records attributes)).

If no one else were writing libraries, I'd try to remedy this in the next major revision of clj-record, but someone else has done this work (or something very much like it) already. Chris Granger wrote korma, and it offers similar functionality in a saner way. I'm also not using Clojure with a SQL database, so I wouldn't be able to eat my own dogfood.

If you're modeling entities in a relational database, I'd recommend korma over clj-record. There may be even better options for you (including just using clojure.java.jdbc directly), so look around.

If you're writing macros, don't capture vars, don't make their functionality depend on globals like *ns*, and consider alternatives to creating vars in the caller's namespace. While you're at it, consider whether the job would be better done with plain old functions.

Using Aleph's Asynchronous HTTP Client for OAuth

If you're implementing authentication in your web app on the server-side using some third party's OAuth, you ideally want to dedicate minimal system resources to waiting for HTTP responses. If you're working in Clojure, you can do this using Aleph's asynchronous http client. Here I'll walk through using it to do server-side authentication with Facebook.

[There's nothing auth-specific about this. Any integration with HTTP end points might benefit from similar treatment.]

Here's what our application needs to do:

1. Send the user to Facebook's OAuth "dialog," passing along a redirect_uri parameter. On the dialog they'll see what permissions your app wants and click a button to say they want to log in. Facebook will redirect them back to your redirect_uri with a code parameter in the query string.
2. Make a server-side request for an access-token for the user, passing along the code parameter and your own redirect_uri
3. Make a second server-side request for whatever information the app needs (e.g., the user's name and email address), passing along the access-token.

We can't do anything for the user between steps two and three, so we'll perform them one after another. It's easy to picture what that might look like if we aren't worried about tying up a thread waiting for responses.

Ideally we'd like an asynchronous implementation to read just as clearly.

We'll start with the easiest implementation to understand, which uses on-realized to register success and error callbacks on each HTTP request. The fn now takes success and error callbacks. In an aleph web application, those would each enqueue a ring-style response map onto the response channel.

Unfortunately this reads terribly due to the nested callbacks.

We can use lamina's run-pipeline macro to create a cleaner version of the same thing.

At this point, however, we realize that we're missing something. In addition to the user data we get from the second HTTP request, we want the "fb-user" passed to the success callback to have the access-token and access-token-expiration we got in response to the first HTTP request.

We can achieve that by breaking the pipeline into two and holding onto the result of the first so that we can refer to it again later (in the call to combine-results.

Note the deref ("@") in that second reference to access-token-result. It's needed because the pipeline returns a result-channel which may not yet be realized. We don't have to worry about the deref blocking, since the value is guaranteed to be realized before the second pipeline will proceed past the initial value, but it looks like something that might block. The whole thing's also a bit sloppy. Zack Tellman pointed out that it's a bit cleaner to split them like so.

The deref is no longer needed because the nested fn will be called with the realized value once it's ready.

If we extract the details so that we have the same level of abstraction we had in the initial (synchronous) version, we end up with this.

Comparing that last fn to the synchronous version, we come out looking pretty good here! The only thing I find a little awkward is that the options to run-pipeline are the second argument. It breaks up the flow when the first "value" is just a call to another pipeline.

Assorted Thoughts

Reusing the connection

Aleph's http-client fn lets you reuse one connection for multiple requests to the same host. It mucks things up a bit, since you want to be sure the connection gets closed at the end.

Not being able to just wrap a (try ... (finally ...)) around the whole thing is a bummer, but it's still not awful. Zach has said there will likely soon be a cleaner way to do "finally" in pipelines.

The :error-handler

The empty :error-handler fns on the "get-fb-" fns suppress "unhandled exception" logging from lamina. Since both of those pipelines are run as part of the outer pipeline, when either of them errors, the error-handler from the outer pipeline will be run, so that's the only place we need to use the error-callback. In my real app, those inner error-handlers just log, but you could just as easily leave the options out completely if you're ok with lamina's logging.

The rest

I've left out many details, like the FB-specific URL-templates and response parsing. If there's interest, I'm happy to share those. I just thought the lamina/aleph stuff was the interesting part.

Cleaner Utility Modules with Module#module_function or extend self

[Disclaimer: GLoc is a nice and generally well written library, and I'd encourage anyone who needs to translate strings to consider it. Also strongly consider i18n, particularly if you're using Rails 2.x.]

My pair and I were looking at the code for GLoc this week. When a class includes the GLoc module, it makes its methods available on instances of the class (as one would expect with include) as well as on the class itself (which would normally only happen with a call to extend). Those same methods are also available as module methods of GLoc itself. So all of the following will work.

GLoc.l(:hi)

class Greeter
  include GLoc
  
  def greet
    l(:hi)
  end
  
  def self.greet
    l(:hi)
  end
end

What caught our eye was how the library goes about making itself so available. Minus all the functionality, here's what it does.

module GLoc
  module InstanceMethods
    # ...
    # useful methods defined here
    # ...
  end
  
  include ::GLoc::InstanceMethods
  
  module ClassMethods
    include ::GLoc::InstanceMethods
  end
  
  def self.included(target)
    super
    class << target
      include ::GLoc::ClassMethods
    end
  end
  
  class << self
    include ::GLoc::InstanceMethods
  end
end

Patrick's comment was "I don't know off-hand what the cleanest way is to do that, but I know that ain't it."

So what's the cleanest way?

First let's clear away the noise of the nested modules. The convention of nesting a ClassMethods module to extend on including classes makes sense when there are distinct behaviors to add at the instance and class levels, but when you want the same methods in both places, it's unnecessary noise.

Here's what we get when we just define the behavior in the top-level module.

module GLoc
  # ...
  # useful methods defined here
  # ...
  
  def self.included(target)
    super
    target.extend self
  end
  
  class << self
    include ::GLoc::InstanceMethods
  end
end

That's much easier on the eye. (Note that calls extend on target rather than having its singleton class include the module. That's important because otherwise the included hook method goes into an (indirect) infinite recursion and you end up with a SystemStackError: stack level too deep.)

Next up, we want a nicer way to expose the methods on the module itself (so you can call GLoc.l(...)).

There's a little-used method on Module for exactly that: Module#module_function. It's a visibility modifier like public, private, and protected and supports the same two usage patterns: you can pass it the names of previously defined methods as symbols or you can use it like a pseudo-keyword that affects subsequently defined methods. It's a weird visibility modifier though: it makes the methods it's applied to private (in the Ruby sense, so they're callable only with an implicit self as receiver) but it also creates public copies of the methods as singleton methods of the module. That gives us two of the three usages that GLoc wants to provide, leaving only the "pretend the including class also called extend" feature to implement in the included hook.

Using module_function gets us down to this.

module GLoc
  module_function
  
  # ...
  # useful methods defined here
  # ...
  
  def self.included(target)
    super
    target.extend self
  end
end

Very nice.

Unfortunately that wouldn't actually work for GLoc.

You can't tell from these snippets, because I've hidden the useful methods, but GLoc's useful methods rely on some lower-level methods that the author didn't want to expose as module functions. Private methods (or any other methods that don't have module_function applied to them) aren't copied into the module's singleton class like the module functions, so they're not there to be called: NoMethodError. Luckily the same sort of flexible API can be created without any copying of methods from one method table to another by having the module extend itself.

module GLoc
  extend self
  
  # ...
  # useful methods defined here
  # ...
  
  def self.included(target)
    super
    target.extend self
  end
end

Using extend self is a bit mind-bending. After all it creates this strange situation.

GLoc.is_a? GLoc    # => true

But keep in mind that it's not for use in modules that actually model anything, just for modules that are bundles of functions. In other words, it's for the kinds of objects where you'd never ask what it is_a?, so the surprising line above shouldn't worry you.

GLoc is what triggered this write-up, but the most instantly grok-able example of a utility module is Ruby's Math. A class might include Math so it has convenient access to sin, cos, sqrt, or whatever. While it's far from intuitive that an instance of that class is_a?(Math), we know why Math is sitting in the class's ancestors list: it's a side-effect of how mix-ins work. (For other kinds of modules, it's quite intuitive that mixing in alters the inheritance chain: it's not surprising, for example, that [1, 2, 3].is_a?(Enumerable).)

So in the pros column module_function is clear and declarative. In the cons column it falls down if you want to compose module functions out of lower-level functions and don't want to expose those the same way. It also scores at best a below average on the principle of least surprise test (at least by my intuition) because for each method you def, methods are added to two different method tables, and modifying the module later will only change one of those. If those cons trouble you (either purely on principle or because of actual requirements), you can use extend self. The only real con there is that it's a slightly weird looking idiom.

I'd like to see either module_function or extend self used on every utility module to communicate about what sort of module it is. What do you think?

In defense of helpers

...in which I defend helpers[1] as good OO, if you use them just so; point out an aspect of the convention that stands in the way of that style; and provide an alpha plugin that tries to change that.

By Rails convention helper modules are where view logic belongs. During request processing, a controller will automatically look for a module in the helpers directory with a name matching the controller. If found, the module will be mixed in to the controller's response's template object, an instance of ActionView::Base (and self in the rendered erb template). A controller can also specify additional helper modules to mix in using the helper class method.

The typical approach I've seen is to define helper methods that take model objects or their attributes as arguments (where the model was typically put into an instance variable by the controller). So the template does something like

<span class='contributors'>
  <%= contributors_list @project.contributors %>
</span>

to use a helper like this

module ProjectsHelper
  
  TOO_MANY = 10
  
  def contributors_list contributors
    if contributors.size < TOO_MANY
      contributors.to_sentence
    else
      contributors[0...(TOO_MANY - 1)].join(', ') + ', and more'
    end
  end
end

I think it's because of this functional style of helper method that I've seen a fair amount of bias against helpers. OO developers like encapsulation, and helper modules generally encapsulate logic but not the information needed to apply that logic.

For example, the first Rails project I was on didn't use any application helpers. The team had created a parallel construct called presenters. The "final" state of the presenter stuff evolved over months of development, but by the end, a page-specific presenter object was always made available in the @presenter instance variable (thanks to some frameworkey extensions in our ApplicationController based on a naming convention), and eventually a method_missing was monkey-patched into ActionView::Base to automatically delegate everything to @presenter so our templates weren't cluttered.

By the time the method_missing went in, we'd come back around to something very close to Rails' built in helpers, and I had a little bit of an aha moment. The helper is the page (because it's mixed in), I thought, why would I pass it my instance variables?

The approach of taking in arguments for things that could have been pulled from instance variables is consistent with a general rule in Ruby that modules ought not to mess with instance variables if they can avoid it. This rule makes good sense in general-purpose modules (like Enumerable or Comparable) because by design these modules are meant to be mixed in to all sorts of objects, and they don't want to put weird constraints on their hosts. (Imagine if the documentation for Enumerable told you "in addition to providing an #each method, the object should take care to avoid using instance variables called @_cursor, @_enumerators, @...." No one would like that.)

Helper modules aren't like that though. They're designed for a specific page or set of pages (i.e., a specific view concern) in your application. The only reason they're modules rather than classes is that you might have multiple view concerns on the same page.

So I thought it might be interesting to let the helper know more and the template know less about Ruby by moving knowledge of the controller-exposed instance variables into the helper. It worked and felt good.

For a while.

Then I realized that the helper wasn't a page definition: it was a few of them. Since all the actions on a controller get the controller's helpers mixed in, a typical controller would have listing pages, detail pages, and edit pages all with the same helper modules.

Blast!

So I wanted helpers to be selected per action rather than controller-wide. But they weren't. So after talking about it for a while, last Friday morning I finally rolled a Rails plugin to make it the way I wanted: it's called ActionHelper and you can find it on GitHub.

For the moment, it does the naming convention thing that you'd probably expect: when processing UsersController's show action, the module UsersShowHelper will be mixed in to the template if it exists. It also allows actions to declare what helper modules they want by calling action_helper inside the action. (You'd expect a class method, and I agree there should be one, but I haven't figured out a pretty API yet, so for now it's not there.) See the README for an actual example.

If you have thoughts on a pretty declarative class method API for this (whether it's annotation-style or more Railsey), call it out in the comments. Better yet, fork the repository on GitHub and send me a pull request once you've got something going. (ActionHelper has been my "get comfy with git" mini-project.)

Thanks for reading.

---

1^ Note that I'm talking about the helpers in your application, not the ones Rails provides in ActionView::Helpers. Those are general-purpose modules.

Loving to_proc

Anytime I'm pairing with a developer who isn't familiar with ActiveSupport's Symbol#to_proc extension, it's fun to see their reaction when they see how it works. It's the most radical example I've seen of a simple (and pure-Ruby) extension to a core class allowing for awesome improvements in readability.

[If you're not familiar with the method, read about it here or here. For a bizarre step beyond (that I'm not personally fond of) take a look at this old Dr. Nic post.]

The one frustrating thing about it is that it's so limited: you can only send one method with no arguments to each yielded object. If you need to pass arguments or do any more complex calculation, you're back to passing an associated block the old-fashioned way.

A couple of weeks ago Patrick and I were pairing and noticed a beneficial side-effect of the limitations of Symbol#to_proc: sometimes when you can't use it right off the bat, it's because the behavior you were going to put in the block would be better off living in the objects you're working with.

Here's a simple example. Imagine you want to expose the area codes represented in a collection of phone numbers. Your initial implementation might look like this.

def area_codes
  self.phone_numbers.collect do |phone_number|
    phone_number[0..2]
  end.uniq
end

"Shame that block's so ugly," you might think. Well you're right! It is a shame, and it doesn't have to be that way. It's ugly because it knows about the guts of phone numbers. If phone numbers knew more about themselves, using them could be prettier.

def area_codes
  self.phone_numbers.collect(&:area_code).uniq
end

Of course sometimes you really need to pass arguments. We toyed around with introducing an Array#to_proc that looked like this.

Array.class_eval do
  def to_proc
    lambda {|target| target.send *self}
  end
end

[1,2,3,4,5].select &[:between?, 2, 4]  # => [2, 3, 4]

In the end we decided that, while nicely brief, it wasn't pretty enough to put into our code base -- too much punctuation -- and stuck with the old-fashioned way.

Are there any other core extensions that have really floated your boat? Do share.

Abuse of method_missing?

Am I the only one who thinks the following DSL-ey trickery is an abuse of method_missing?

Here's the creation of some named routes.

ActionController::Routing::Routes.draw do |map|
  map.home '', :controller => 'main', :action => 'start'
  map.user_page 'users/:user', :controller => 'users', :action => 'show'
end

You call arbitrary methods on the map object, and that creates a route whose name is the method you called.

Here's the declaration of an ActiveRecord model's attributes using Hobo's new migration-generating style.

class User < ActiveRecord::Base
  fields do
    name :string, :null => false
    email :string
    about :text, :default => "No description available"
  end
end

I haven't looked into the code, but I assume the block is instance_eval'ed against some object whose method_missing builds up attribute meta-data where the name of the missing method becomes the attribute name.

Introducing new symbols into your system by invoking them as methods on bizarre (sometimes hidden) objects strikes me as a nearly useless twisting of Ruby's flexibility. If you're working with something that's purely DSL-ish, that's one thing, but if we're talking about a tiny little internal DSL embedded in otherwise idiomatic Ruby code, and, needless to say, being edited by Ruby developers, this just seems to introduce confusion.

Of the two uses, I actually prefer Hobo's because it goes farther than Rails' away from idiomatic Ruby and towards a DSL. Since I actually see that I'm sending messages to map when creating a named route, it frustrates me that this object exposes its functionality through method_missing, and I'm therefore unable to look up the API reference in the normal way. What is that thing? Does it have any methods that might conflict with my route names? We know about connect. Hopefully that's the only one.[1]

Do you think this sort of use of method_missing is advisable? How have you used and abused it?

[1] Actually, a look at routing.rb shows that ActionController::Routing::Routes.draw yields an ActionController::Routing::RouteSet::Mapper, which also (in the neighborhood of line 1000[2]) defines named_route: not a likely name conflict, but arguably a clearer way to define your routes.

ActionController::Routing::Routes.draw do |map|
  map.named_route :home, '', :controller => 'main', :action => 'start'
  map.named_route :user_page, 'users/:user', :controller => 'users', :action => 'show'
end

[2] Yeah, line one-thousand. The Rails team are trained professionals: please don't try that at home.

Towards More Readable Tests

Making code readable is probably my number two priority after making it function correctly. I want every developer on the team to be able to look at code written by anyone else on the team and understand its intent. Of course that goes for the tests too.

We've had a few new developers join the team recently, and I've heard more than one of them say "Huh, I've never seen that before" about a pattern that shows up in some of our unit tests. It's something I've done and liked, but on further reflection, I'm starting to lean away from it.

Here's a test that could be clearer. (Let's take the interaction being tested as a given and focus on how the tests read. (Let's also not assume I have any interest in fishing. I don't know how I landed on this example.) The mocking library is Mocha.)

test "catches, kills, and bags tasty fish" do
  river = mock
  fish = mock
  river.expects(:give_it_up!).returns(fish)
  fish.expects(:tasty?).returns(true)
  fish.expects(:die!)
  bag = mock
  bag.expects(:put).with(fish)
  
  fisher = Fisher.new(river, bag)
  fisher.fish
end

Here's what makes it hard to read:

  river = mock                              # setup
  fish = mock                               # setup
  river.expects(:give_it_up!).returns(fish) # expectations
  fish.expects(:tasty?).returns(true)       # expectations
  fish.expects(:die!)                       # expectations
  bag = mock                                # setup
  bag.expects(:put).with(fish)              # expectations

As you read that, you have to change gears back and forth as you read lines that create testing fixtures interspersed with lines that specify the interaction we're working on. No good.

After reading a few tests like that (and tests with the same problem magnified), lines like these

  river = mock
  fish = mock
  # ... snip
  bag = mock

start to feel like pure noise. I suppose that was my mindset when I started changing those sorts of tests to look like the following.

test "catches, kills, and bags tasty fish" do
  (river = mock).expects(:give_it_up!).returns(fish = mock)
  fish.expects(:tasty?).returns(true)
  fish.expects(:die!)
  (bag = mock).expects(:put).with(fish)
  
  fisher = Fisher.new(river, bag)
  fisher.fish
end

Here, the creation of mocks is all inlined so that you can read straight through the interaction. The assignment of local variables embedded at the first occurence of each item in the interaction (thanks to Ruby's lack of variable declaration) isn't too distracting once you're accustomed to seeing it. Each test takes up less screen real estate than before, and the world's all around better than it was.

Or at least that's how I felt based on the before and after states of some pretty long tests I had to work with.

But here's something that's no better than before (probably worse): look at the test above and tell me which collaborators come into play when a Fisher catches a tasty fish, as opposed to the following, in which the fish gets thrown back.

test "catches and releases non-tasty fish" do
  (river = mock).expects(:give_it_up!).returns(fish = mock)
  fish.expects(:tasty?).returns(false)
  river.expects(:put).with(fish)
  
  fisher = Fisher.new(river, :ignored_bag)
  fisher.fish
end

It requires some pretty annoying code scanning when mock creation is embedded at various funny spots along the way. Those lines that felt like noise when they interrupted the specification of the interaction suddenly seem a lot more useful.

Having reconsidered, I now think the clearest presentation is closer to the first version, but different in a key way.

test "catches and releases non-tasty fish" do
  river = mock
  fish = mock
  
  river.expects(:give_it_up!).returns(fish)
  fish.expects(:tasty?).returns(false)
  river.expects(:put).with(fish)
  
  fisher = Fisher.new(river, :ignored_bag)
  fisher.fish
end

test "catches, kills, and bags tasty fish" do
  river = mock
  fish = mock
  bag = mock
  
  river.expects(:give_it_up!).returns(fish)
  fish.expects(:tasty?).returns(true)
  fish.expects(:die!)
  bag.expects(:put).with(fish)
  
  fisher = Fisher.new(river, bag)
  fisher.fish
end

Like the list of characters at the beginning of a script, these tests tell the reader who's involved in the interaction before jumping into the story, which makes the story much easier to follow. The tests take up more lines, but are easier for the reader to comprehend, particularly one who isn't interested in all the details. (Imagine looking through your codebase before a big refactoring wondering "how often do we use that FishingBag I want to change?" If collaborators are called out, you can quickly focus on the relevant interactions and read just those in detail.)

In the case of the longer tests that pushed me towards the inline style, the list of players would have been quite a bit uglier than the one above. That shouldn't have discouraged me from keeping it separated from the rest of the test: it should have told me that the object I was trying to test had too much on its plate. When something like that happens, hopefully I can figure out a design that won't require more players than I can easily keep track of all at once. If not, at least the next developer who comes along will have a clean tally of all the collaborators and a fighting chance of improving the design herself.

Good luck!