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.
jjinux said…
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.
Anonymous 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"
jjinux said…
Thanks for reading, Andy!

Popular posts from this blog

Drawing Sierpinski's Triangle in Minecraft Using Python

In his keynote at PyCon, Eben Upton, the Executive Director of the Rasberry Pi Foundation, mentioned that not only has Minecraft been ported to the Rasberry Pi, but you can even control it with Python . Since four of my kids are avid Minecraft fans, I figured this might be a good time to teach them to program using Python. So I started yesterday with the goal of programming something cool for Minecraft and then showing it off at the San Francisco Python Meetup in the evening. The first problem that I faced was that I didn't have a Rasberry Pi. You can't hack Minecraft by just installing the Minecraft client. Speaking of which, I didn't have the Minecraft client installed either ;) My kids always play it on their Nexus 7s. I found an open source Minecraft server called Bukkit that "provides the means to extend the popular Minecraft multiplayer server." Then I found a plugin called RaspberryJuice that implements a subset of the Minecraft Pi modding API for B

Ubuntu 20.04 on a 2015 15" MacBook Pro

I decided to give Ubuntu 20.04 a try on my 2015 15" MacBook Pro. I didn't actually install it; I just live booted from a USB thumb drive which was enough to try out everything I wanted. In summary, it's not perfect, and issues with my camera would prevent me from switching, but given the right hardware, I think it's a really viable option. The first thing I wanted to try was what would happen if I plugged in a non-HiDPI screen given that my laptop has a HiDPI screen. Without sub-pixel scaling, whatever scale rate I picked for one screen would apply to the other. However, once I turned on sub-pixel scaling, I was able to pick different scale rates for the internal and external displays. That looked ok. I tried plugging in and unplugging multiple times, and it didn't crash. I doubt it'd work with my Thunderbolt display at work, but it worked fine for my HDMI displays at home. I even plugged it into my TV, and it stuck to the 100% scaling I picked for the othe

Creating Windows 10 Boot Media for a Lenovo Thinkpad T410 Using Only a Mac and a Linux Machine

TL;DR: Giovanni and I struggled trying to get Windows 10 installed on the Lenovo Thinkpad T410. We struggled a lot trying to create the installation media because we only had a Mac and a Linux machine to work with. Everytime we tried to boot the USB thumb drive, it just showed us a blinking cursor. At the end, we finally realized that Windows 10 wasn't supported on this laptop :-/ I've heard that it took Thomas Edison 100 tries to figure out the right material to use as a lightbulb filament. Well, I'm no Thomas Edison, but I thought it might be noteworthy to document our attempts at getting it to boot off a USB thumb drive: Download the ISO. Attempt 1: Use Etcher. Etcher says it doesn't work for Windows. Attempt 2: Use Boot Camp Assistant. It doesn't have that feature anymore. Attempt 3: Use Disk Utility on a Mac. Erase a USB thumb drive: Format: ExFAT Scheme: GUID Partition Map Mount the ISO. Copy everything from