This is Yet Another Post about preferring composition over inheritance, but I will try to drive it home with Real World Examples™. I’m not saying “don’t use inheritance”, I am saying “use inheritance conservatively, and when it is appropriate” (I know it’s not very controversial). I think I can confidently say “don’t inherit from String, Hash, or Array in Ruby”, and in this post we’ll look at two concrete examples about why you shouldn’t inherit from those classes.

YAGNI methods

YAGNI methods are baggage that your subclasses are carrying around. They’re methods that you may never use, but you have to deal with them because your parent class implemented them. The thing about these methods is that when you sit at your keyboard, innocently pecking out the characters class Parameters < Hash, you don’t realize that these YAGNI methods are there waiting in the shadows getting ready to ruin your day.

Let’s look at a couple examples of how these YAGNI methods make our lives harder.

Memory Leaks

I showed this off during my Keynote at RailsConf, but we’ll dive in a little more here. As of e2a97adb, this code leaks memory:

require 'action_controller'

params = ActionController::Parameters.new({
  foo:[Object.new]
})

loop do
  params[:foo]
  params.delete :foo
  params[:foo] = [Object.new]
end

If you run this code, you’ll see that the process’s memory grows unbounded.

ActionController::Parameters is used for the parameters hash in your controller. When you do params[:id], params returns an instance of ActionController::Parameters. This class inherits from ActiveSupport::HashWithIndifferentAccess, which in turn inherits from Ruby’s Hash.

Why does this leak?

There are two methods involved in our leak. One method is the delete method, and ActionController::Parameters does not implement that method. The other method is the [] method, so let’s look there.

If you read the implementation of the square brace method, you’ll see it calls convert_hashes_to_parameters, which calls convert_value_to_parameters.

Here is the convert_value_to_parameters method:

def convert_value_to_parameters(value)
  if value.is_a?(Array) && !converted_arrays.member?(value)
    converted = value.map { |_| convert_value_to_parameters(_) }
    converted_arrays << converted
    converted
  elsif value.is_a?(Parameters) || !value.is_a?(Hash)
    value
  else
    self.class.new(value)
  end
end

This method seems to do some sort of conversions on Array, and appends the conversion to the converted_arrays object. Each time we iterate through the loop, we delete a key, but that value is never deleted from the converted_arrays object. Each time we access the Array value, it gets “converted”, and that converted array is added to the converted_arrays object. So, the converted_arrays object grows unbounded.

Why do we need to convert stuff? Why do we need to mutate a thing? This method leaves me with more questions than I have time to deal with here, but presumably the function is necessary.

How do we fix it?

Well, we need to implement the delete method. Whenever the delete method is called, we need to remove any “converted arrays” from the converted_arrays list.

This solution may work for the delete method, but what about the delete_if method? What about the merge! method? How about keep_if? How about reject!? How about select!? The list goes on, and soon we feel like we’re playing whack-a-mole with method implementations.

Rather than “How do we fix it?” I think a better question is “Do we need it?”. For these mutation methods, I think the answer is “no”. The author of convert_value_to_parameters probably didn’t think about these mutation methods, and I’m willing to bet that very few people actually mutate their own params object. I’ll also bet that of the people who do mutate their params object, 100% of those people would be OK with calling to_hash on params before making mutations.

Ok! Lets remove the method!

Whoa! Not so fast there, friend. We can’t just remove the method. We need to add deprecation warnings so that we don’t break applications when people upgrade. That means we need to add a warning on merge!, and keep_if, and delete_if, and select!, and reject!, etc.

Feels like we’re playing method whack-a-mole again, doesn’t it?

These methods are what I think of as YAGNI methods. Methods that we probably don’t need, but we got them for “free” through inheritance. Remember that even in life, inheriting something isn’t always a good thing.

So how do we really fix it?

¯\_(ツ)_/¯

We should probably switch this to use composition, but it will be a breaking change so it must wait until Rails 5.

The End

I said I was going to write about two issues with YAGNI methods, but I don’t really want to right now. I will write more later. However, the next installment will be about ActionView::OutputBuffer which is (eventually) a subclass of String. We will talk about how the YAGNI methods on OutputBuffer are hurting performance of your Rails application.

Remember: stay conservative with your API. It’s easier to add a new method than it is to take one away.

[…]

The Ruby Rails Aggregator

February 2016

December 2015

February 2015

January 2015

June 2014

March 2014

February 2014

January 2014

May 2013

March 2013

February 2013

December 2012

September 2012

July 2012

June 2012

February 2012

January 2012

December 2011

November 2011

October 2011

June 2011

March 2011

January 2011

December 2010

April 2010

January 2010

December 2009

October 2009

September 2009

June 2009

May 2009

April 2009

February 2009

December 2008

November 2008

October 2008

September 2008

August 2008

July 2008

June 2008

May 2008

April 2008

March 2008

February 2008

January 2008

December 2007

November 2007

October 2007

September 2007

August 2007

July 2007

June 2007

May 2007

April 2007

March 2007

February 2007

January 2007

December 2006

November 2006

October 2006

September 2006

August 2006

July 2006

June 2006

May 2006