Rails instance methods conditional depending on attributes being set

Question

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

Please edit your question to include the code you have so far. — Jordan Running
– Jordan Running, Commented Jan 3, 2016 at 2:04
@Jordan Just made some edits, hopefully it will be a bit more clear now — Sam Mason
– Sam Mason, Commented Jan 3, 2016 at 2:16
Under what circumstances would number_or_uses not be present? — Jordan Running
– Jordan Running, Commented Jan 3, 2016 at 2:31
@Jordan just added a comment below your answer explaining why number_of_uses sometimes is nil — Sam Mason
– Sam Mason, Commented Jan 3, 2016 at 9:43

Jordan Running · Accepted Answer · 2016-01-03 19:31:32Z

1

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.

Further improvements

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

edited Jan 3, 2016 at 19:31

answered Jan 3, 2016 at 2:54

Jordan Running

107k18 gold badges193 silver badges189 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Sam Mason Over a year ago

Thanks for that amazingly in depth answer. Number of uses can be nil as a user can set that number. E.g this model is about discount codes and so a user can optionally set the attribute if they want to restrict uses if it's nil they can use it infinitely.

Richard Peck Over a year ago

Upvoted for looking at the context, not just the code

Sam Mason Over a year ago

In relation to point 4 the reason it can return nil is because by default if expiry_date is not set by the user and number_of_uses is not set by the user then is_valid? Will be true as the discount code will work. But any of those attributes are set I need to perform the methods to check if the discount code is still valid

Jordan Running Over a year ago

@SamMason I've edited my answer to show some further improvements that could be made around the semantics of nil

dwenzel · Accepted Answer · 2016-01-03 02:37:52Z

Short-circuiting usually works best in these situations.

Before:

if @attribute.present?
  @attribute.do_something
end

Short-circuiting:

@attribute && @attribute.do_something

With the short-circuiting approach, as soon as Ruby sees that the left side of the && operator is nil, it will stop and not run the right side

I would also think hard about why a particular attribute should ever be allowed to be nil (as Jordan asked). If you can think of a way to avoid this, that may be better.

Assuming that you do want number_of_users to be able to be nil, you could rewrite has_uses_remaining?like this:

def has_uses_remaining?
  !number_of_uses || remaining_uses > 0
end

-Side note: your first method can be simplified to this:

def is_valid?
   !has_expired? && has_uses_remaining?
end

Collectives™ on Stack Overflow

Rails instance methods conditional depending on attributes being set

2 Answers 2

Further improvements

4 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Further improvements

4 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related