Double Shot #1297

Drone – Continuous integration platform built on top of Docker.
Announcing gitsh – A dedicated shell for running git subcommands.

YAGNI methods are killing me

June 3, 2014

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.

[…]

The Ruby Rails Aggregator

Double Shot #1297

Leave a Reply Cancel Reply