I have a model that has several attributes that are optional when the model is being saved.
I have several instance methods that use these attributes and perform calculations but I would like to check first if they are not nil as I will get the dreaded no method error nil nil class
Apart from littering my code with .present?
is there a better way of doing this?
EDIT: Here is my code so far
def is_valid?
(has_expired? === false and has_uses_remaining?) ? true : false
end
def has_expired?
expiry_date.present? ? expiry_date.past? : false
end
def remaining_uses
if number_of_uses.present?
number_of_uses - uses_count
end
end
def has_uses_remaining?
number_of_uses.present? ? (remaining_uses > 0) : true
end
I feel like dropping in .present?
to perform checks has a bad code smell, I have looked into the null object pattern but it doesnt seem to make sense here as the object is present but some of the attributes are nil
I think the real issue here is that number_of_uses
can be nil
, which (as you've discovered) introduces a ton of complexity. Try to eliminate that issue first.
If for some reason you can't do that, each of your methods can be improved:
condition ? true : false
is always a code smell. Boolean operators return boolean(ish) values, so let them do their jobs:
def is_valid?
!has_expired? && has_uses_remaining?
end
Personally I think using Rails' Object#try
is usually a code smell, but here it's a pretty good fit:
def has_expired?
expiry_date.try(:past?)
end
Alternatively:
def has_expired?
expiry_date.present? && expiry_date.past?
end
This one can't be improved a whole lot, but personally I prefer an early return
to a method wrapped in an if
block:
def remaining_uses
return if number_of_uses.nil?
number_of_uses - uses_count
end
You could also do number_of_uses && number_of_uses - uses_count
(or even number_of_uses.try(:-, uses_count)
but I think this is clearer.
It's a little weird that this method returns true
if number_of_uses
is nil
bit since it does we can simplify it like so:
def has_uses_remaining?
remaining_uses.nil? || remaining_uses > 0
end
Note that I call remaining_uses.nil?
instead of number_of_uses.nil?
; there's no need to depend on both when we can get the same result from one.
Upon further consideration I think you can make the intent of this code clearer by introducing another method: has_unlimited_uses?
:
def has_unlimited_uses?
number_of_uses.nil?
end
def is_valid?
!has_expired? &&
has_unlimited_uses? || has_uses_remaining?
end
def remaining_uses
return if has_unlimited_uses?
number_of_uses - uses_count
end
def has_uses_remaining?
has_unlimited_uses? || remaining_uses > 0
end
This way there's never any ambiguity about what you're checking for. This will make the code more readable for the next person who reads it (or you six months from now) and make tracking down bugs easier.
It still bothers me, though, that remaining_uses
returns nil
. It turns out that if instead we return Float::INFINITY
, has_uses_remaining?
turns into a simple comparison:
def remaining_uses
return Float::INFINITY if has_unlimited_uses?
number_of_uses - uses_count
end
def has_uses_remaining?
remaining_uses > 0
end