Search code examples
ruby-on-railsrubynull-object-pattern

Rails instance methods conditional depending on attributes being set


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


Solution

  • 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:

    1. 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
      
    2. 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
      
    3. 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.

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