Return to Answer

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).

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).

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 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).

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).

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.

Indentation: 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 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 raising 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).

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 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).