TL;DR: Inheriting from Hash will bite you. ¯\_(ツ)_/¯

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.

Read more at the source