Skip to main content

Rails: authlogic Code Review

I just spent a couple days code reviewing authlogic. First of all, let me say it's good code. Usually, I have a lot to say when I code review someone's code, but this time, I was pretty happy. Here are the corrections I submitted.

Clearly, the author of authlogic knows Ruby a heck of a lot better than I do; I'm still relatively new to Ruby. There were a bunch of idioms, language features, and design decisions that caught my attention as a Python programmer. That's what this blog post is about.

The first thing I noticed is that the code pieces together huge classes by mixing in tons of modules. For instance, in authlogic/session/base.rb:
module Authlogic
module Session
# This is the base class Authlogic, where all modules are included. For
# information on functionality see the various sub modules.
class Base
include Foundation
include Callbacks
include Timeout
include Params
include Cookies
include Session
include HttpAuth
...
There are 22 included modules in all.

Here is one of the included modules, authlogic/session/foundation.rb:
module Authlogic
module Session
module Foundation
def self.included(klass)
klass.class_eval do
extend ClassMethods
include InstanceMethods
end
end

module ClassMethods
...
end

module InstanceMethods
...
end
end
end
end

The "included" method gets called when the module is included. It uses "class_eval" to explicitly mix in some methods. Notice that the "ClassMethods" module is explicitly mixed in using "extend", and the "InstanceMethods" module is explicitly mixed in using "include".

I had to refer to my Ruby book to figure out the difference. A class includes a module when it wants to mix in the module's functions as instance methods. It affects every instance of the class. An object extends a module when it wants to mix in the module's functions just into itself. In this case, the object in question is a class, so it's in essence mixing in functions as class methods. This is a vivid reminder that unlike languages like C++, classes in Ruby are objects too.

By the way, I do worry that this excessive use of mixins will lead to namespace conflicts. I ran self.methods.size in my UserSession class, which inherits from Authlogic::Session::Base. It reported 271 methods. I did the same thing in my User model, which inherits from ActiveRecord::Base. It reported 550 methods!

The next thing I noticed was methods like:
def session_ids
self.class.session_ids
end

def session_class
self.class.session_class
end
In Python, if you call a method on an instance, if it can't find the method among the instance methods, it'll also look at the class methods. Java does this too, although it's frowned upon. Ruby doesn't look among the class methods when you call an instance method. Hence, the code is explicitly delegating to the class in this code. Code like the above happens a surprisingly large number of times across the code base. I'm surprised there isn't a helper like "delegate_to_class :only => [:session_ids, :session_class]".

Another thing I noticed is code like this:
module Callbacks
METHODS = [
"before_password_set", "after_password_set",
"before_password_verification", "after_password_verification"
]
...
private
METHODS.each do |method|
class_eval <<-"end_eval", __FILE__, __LINE__
def #{method}
run_callbacks(:#{method}) { |result, object| result == false }
end
end_eval
end
end
Look closely at '<<-"end_eval", __FILE__, __LINE__...end_eval'. That's actually evaluating a "heredoc" (or at least that's what they call it in other languages) in order to define a method. Using some flavor of "eval" to add methods on the fly is fairly common in Ruby.

Another thing I noticed is code like:
def find_using_perishable_token(token, age = self.perishable_token_valid_for)
In Python, the defaults to a function are evaluated once, as the function is defined. They are not evaluated every time the function is called. If you forget that Python has "static defaults", you'll eventually get bitten by a bug. Apparently, that's not the case in Ruby:
irb(main):001:0> def f(default = [])
irb(main):002:1> default << "hi"
irb(main):003:1> end
=> nil
irb(main):004:0> f
=> ["hi"]
irb(main):005:0> f
=> ["hi"]
authlogic has impressively good docstrings. Ruby uses comments for docstrings. Python uses specially-placed strings. Hence, in Python, you can piece together docstrings at runtime using string interpolation, etc. I often make good use of this to keep my docstrings DRY and to prevent them from going stale. For instance, I might put a comment in a variable, and reuse that same comment in multiple docstrings. The code for authlogic occasionally has to duplicate the same comment.

Ruby programmers tend to use longer function names and they don't often try to limit their code to 80 columns. Here is a mildly comical case:
# A convenience function to merge options into the validates_length_of_login_field_options. So intead of:
#
# self.validates_length_of_password_field_options = validates_length_of_password_field_options.merge(:my_option => my_value)
#
# You can do this:
#
# merge_validates_length_of_password_field_options :my_option => my_value
def merge_validates_length_of_password_field_options(options = {})
self.validates_length_of_password_field_options = validates_length_of_password_field_options.merge(options)
end
I enjoyed the metaprogramming in authlogic. For instance, code like the following is fairly common across the codebase:
self.class.send(:attr_writer, login_field) if !respond_to?("#{login_field}=")
The last thing that caught me off guard was that Ruby supports:
1/0 rescue 'hi'
Apparently, that's an expression-level form of begin/rescue (aka try/finally). The above evaluates to "hi".

Anyway, as I said, authlogic is good code, and I learned a lot :)

Comments

raichu said…
Just to point out:

Unlike the "if/unless" and "while/until" post-expression modifiers, the inline rescue evaluates after the prepended expression.

e.g.

For:
(1 + 1) if (1 + 2)
3 is evaluated first.

But:
(1 + 1) rescue (1 + 2)
2 is evaluated first.

Just to point out.
That seems natural, otherwise you couldn't do this:

check_sanity rescue cry_for_help

cry_for_help shouldn't be evaluated unless check_sanity raises an exception.
andy said…
nice post, not what I was looking for but I still took the time to read & now comment!

it's refreshing to see a ruby review from an "outsider"
Thanks for reading, Andy!