Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

A little background first. I've been working on a fairly large Rails application which quickly grew into a breeding ground for "smelly" code. One antipattern we were using a lot was storing objects in an STI lookup table and having lots of scattered logic based on the name of the object.

Here's a dumbed down example of what we had:

class Lookup < ActiveRecord::Base
end

class AlarmStatus < Lookup
  has_many :alarm_logs
end

class AlarmLog < ActiveRecord::Base
  belongs_to :alarm_status
end

Then all over our application we would do things like:

case @alarm_log.alarm_status.name
when "active"
  # Do something
when "verified"
  # Do something
when "inactive"
  # Do something
end

This is obviously bad for a lot of reasons, but let's just say it was everywhere. After a few months of this, a consultant came along and pointed out our "code smell" and made some suggestions on how to refactor, which included a Ruby implementation of Java's Enum class.

We ended up using this frequently in our app, and have extracted into a ruby gem called classy_enum. The premise of classy_enum is that you get the power of OOP mixed with the strictness of an enumerated type, which is pretty much what we were trying to mimic with our STI Lookup model.

Example of defining an enum:

class Priority < ClassyEnum::Base
  enum_classes :low, :medium, :high

  def send_email?
    false
  end
end

class PriorityHigh < Priority
  def send_email?
    true
  end
end

The gem's README has some other usage examples, including how to integrate it with a Rails project.

I may be asking for a lot here, but I would love some feedback from anyone willing to give it. It seems like a really cool solution to our problem and I can't imagine we're the only ones who have attempted to solve this before. Here's a list of questions that would be really useful for us:

  • Is this a common problem?
  • Does our solution make sense? Is there a better way to do it?
  • Is the documentation clear?
  • Would you use this in your application? Why or why not?
  • How could we improve it?
  • How is the code quality?
share|improve this question
add comment

1 Answer 1

up vote 3 down vote accepted

Yes, it is a common problem.

I think this is a great way to solve this, I wish I'd done it! Back in my java days I did use the typesafe enum pattern. In ruby I tend to create method objects after the third or so repetition in cases like this, but this is a much cleaner solution and I'm going to start using it for sure.

I found the documentation really clear. I do intend to use it in my app, because it is an excellent way to formalize what I am doing when I create method objects and suggest to others that they replace their long nested if statements with method objects.

The only think I think of at the moment that I might want to improve on it would be to have an option to create a back reference to the owning object for cases where double dispatch will further remove conditional logic. (or like for example where I had a a set of alarms that each should ring at some percentage of the owning objects master volume)

share|improve this answer
    
Thanks Michael, I appreciate the feedback. I'm not sure I follow your suggestion about creating a back reference. Would it be possible for you to provide an example of how you'd want to use it? –  Beerlington Feb 5 '11 at 14:47
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.