At my work we have a North America team and a China team.  Part of our workflow is to review each others merge requests before merging.  As a junior developer I often see things that I have never seen before and must make an educated decision about whether the unfamiliar pattern is a genuine coding error or deviation from best practices, or just something new I don't know about.  And because the person who wrote the code is in China in an opposite time zone, it's not easy to ask them directly about it.  So, while reviewing an MR I saw a use of the try method that I had never seen before.  Here is a simplified version:

class Order
  def grand_total
    sub_total.try(:*, sales_tax)
  end

  def sub_total
    FooBar.baz
  end
end

Here the asterisk is the multiplication operator.  Try is trying to multiply sub_total by sales_tax.  If sub_total is nil, it will return nil, instead of erroring out as would happen when trying to multiply nil by a number.

This seemed harmless enough but when I showed it to the team architect in North America he said that using try is like introducing cancer into the code base.  I believe his exact words were "OMG make it stop that try needs to die with fire".  It seems harmless but then other people see it and start to use it and if allowed to continue can introduce a lot of uncertainty into the code.  It is better to know what kind of result your methods are returning.  We then had a pairing session to use alternative ways to handle nil cases without using try, and here are two of the alternatives we used:

The first alternative is to use an if statement.

class Order
  def grand_total
    if subtotal
      (sub_total * sales_tax)
    else
      nil
    end
  end

  def sub_total
    FooBar.baz
  end
end

If there's a subtotal it will perform the multiplication, if there is not it will return nil.  Since by default nil is what is returned by else, you can take out the else and make the statement one line.

class Order
  def grand_total
    (sub_total * sales_tax) if sub_total
  end

  def sub_total
    FooBar.baz
  end
end

What if we wanted to return 0 instead of nil if there was no sub_total?  We could go back to the if else statement but sometimes it can be better to use a Null Object pattern, which is the second alternative.

class Order
  def grand_total
    sub_total * sales_tax
  end

  def sub_total
    result = FooBar.baz
    if result
      result
    else
      NullOrder.new.sub_total
    end
  end
end

class NullOrder
  def sub_total
    0
  end
end

Going into the sub_total method, if we get a result from FooBar.baz we return it, if it is nil then we return an instance of NullOrder which has a sub_total of 0.

We then no longer have to have branching inside the grand_total method.  It will either be the actual sub_total or the sub_total on the NullOrder which is 0.

This is an example of Duck Typing.  Duck Typing is when we don't care what type of object we're dealing with only that it has certain behavior.  In this case  we just care that Order and NullOrder both have a sub_total method.

In order to prevent an error in case someone tries to call an undefined method on the NullOrder, we can override the method_missing method to return whatever we want.  For example:

class NullOrder
  def method_missing(*_args)
    nil
  end

  def sub_total
    0
  end
end

If someone were to call NullOrder.new.whatever, the method_missing method gets called which by default will raise an error.  Instead we override it to return nil.  We add a splat argument in case someone calls an undefined method on it that has arguments.  The underscore in  front of "args" denotes that it's something that's not supposed to be used.

In summary, the try method introduces uncertainty into the code because it makes it ok to not know what is being returned by a method.  Alternatives to using try are using if statements and the Null Object pattern.

Write a Response

Upload Background Image
Drop File