Refactor or simple code RoR

Question

I have a GiftCard class where I have method can_deliver? and deliver! that I think can be refactored to better looking code :

  def can_deliver?
    (self.sent_at.nil? && self.scheduled_at.nil?) || (self.sent_at.nil? && self.scheduled_at && self.scheduled_at < Time.now)
  end

  def deliver!
    return unless self.can_deliver?
    begin
      Notifier.gift_card_code_mail(self).deliver
      self.sent_at = Time.now
      self.save
    rescue Exception
      logger.error "Could not send email gift card ##{self.id}"
      self.line_item.order.comments.create!(:content => "Can not send gift card mail")
    end

    begin
      Notifier.gift_card_delivered(self).deliver
    rescue Exception
      logger.error "Could not send deliver confirmation email gift card #{self.id}"
      self.line_item.order.comments.create!(:content => "Can not send confirmation delivery of gift card")
    end

  end

Answer 1

Some notes:

• self: You write self.attribute to access attributes. I won't say that's bad practice, not at all, mainly because it's easier to see if you're accessing local variables or instance methods. However, in Ruby it's idiomatic not to use self in this case.

• variable.nil?: this is something we often see but 99% of the time is unnecessary verbose. You want to know if a variable is not set? write !variable. The only case a explicit nil? is needed is when you want to tell false from nil, not the case here.

• Layout: Don't write long lines, 80/100 is a sound limit. It usually pays off in form of clarity also: in can_deliver? for example, if you break on the || operator the boolean expression is much more clear.

• Early returns: return unless self.can_deliver?. Again, I won't say that's bad, in a language that has no syntax for guards (like other languages have), it's a handy way to do early returns when pre-conditions are not met. As a rule of thumb, however, I'd recommend to write a full conditional. Yeah, I know, it's more verbose and you get an extra indentation level, but on the other hand the layout of the function/method helps understanding what the method it's doing.

• The cool thing about the way you wrote can_deliver? (using an expression instead of a bunch of imperative returns) is that you can apply boolean algebra. Notice here than (!p && !q) || (!p && q && q < t) -> !p && (!q || q < t). Of course this kind of simplifications must only be done when the resulting expression is at least as declarative as the original (we are not trying to save some NAND gates here). If you read it out loud and it makes sense then it's ok.

• You have two (and probably more throughout the code) almost identical begin/rescue blocks. That's a call for abstraction (it this case an abstraction using block wrappers).

• Don't write rescue Exception, that's a common pitfall. Check this SO question.

• Shouldn't deliver! return something? how would the caller know if it was successful? you should always return something, either a value (a boolean seems fit in this case) or, yes, return nil but raise an exception on error. Personally I like to return values and leave exceptions for really exceptional things (like, I don't know, the hard disk exploded).

Applying all these points, and with a declarative approach in mind, I'd write:

class GiftCard
  def can_deliver?
    !sent_at && (!scheduled_at || scheduled_at < Time.now)
  end

  def deliver!
    can_deliver? && send_card_code_email && send_gift_card_email
  end

private

  def comment_on_exception(msg) 
    yield
  rescue => exc
    logger.error(msg)
    line_item.order.comments.create!(:content => msg)
    false
  end

  def send_card_code_email
    comment_on_exception("Could not send email gift card #{id}") do
      Notifier.gift_card_code_mail(self).deliver
      update_attribute(:sent_at, Time.now)
    end
  end

  def send_gift_card_email
    comment_on_exception("Could not send deliver confirmation email gift card") do
      Notifier.gift_card_delivered(self).deliver
    end
  end
end

If you found some of these advices useful, take a look at the RubyIdioms page I maintain (and its RubyFunctionalProgramming companion). Comments most welcome.