# Refactor or simple code RoR

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
self.sent_at = Time.now
self.save
rescue Exception
logger.error "Could not send email gift card ##{self.id}"
end

begin
rescue Exception
logger.error "Could not send deliver confirmation email gift card #{self.id}"
end

end

-

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!
end

private

def comment_on_exception(msg)
yield
rescue => exc
logger.error(msg)
false
end

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