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.