Question

I was recently debugging a bit of code where the previous implementation looked something like this:

# controller for group/customers
def index
  @customers = current_user.available_customers(param[:group_id].to_i)
end

# user
def available_customers(group_id)
  # some other code, that sometimes returns early    

  # this is the line of failure
  # self.accessible_group_ids returns an array of integers
  # as does Group.children_ids
  # This line is attempting to get the subset of group ids that the user
  # is *allowed* to touch, and the hierarchy of group ids under the selected one. 
  ids = (self.accessible_group_ids & ([group_id] + Group.children_ids(group_id)))
  return Customer.where(group_id: ids)
end

I was refactoring the calling method to available_customers in the controller and missed the specific conversion of params[:group_id].to_i, therefore introducing a bug where when user input is sent via the params, it's a string, and the comparison in the ids = line doesn't work as expected - it excludes the current group_id from the set, because an string isn't an int, even if it has the same digits in it. :)

I see this original implementation as... troublesome... leaving aside the dependencies on the Group and Customer classes. Needing to do the type conversion before passing the data to the method seems likely to fail (as it did in my case) and requires the callee to have knowledge of how the method works (and fairly intimate knowledge too!).

I don't think the callee should have to know that the method requires an int, and in a loosely typed language such as Ruby, it's okay to accept all types of Ducks in method arguments as long as they quack.

My solution was:

def available_customers(group_id)
  # nil handling
  group_id = group_id.to_i
  # everything else
end

Now the available_customers method is the only place that needs to know that group_id needs to be an integer.

Where do you think is a good place to put this? Would you put this somewhere else? Why?

Was it helpful?

Solution

I don't think the callee should have to know that the method requires an int, and in a loosely typed language such as Ruby, it's okay to accept all types of Ducks in method arguments as long as they quack.

Dynamic languages like Ruby are great at rapid prototyping, but they are terrible at carefully defining and checking the contract of each method. That's where static typing is at a decided advantage.

You are mistaken that the caller doesn't have to know that they have to provide an int. If they don't provide an int or int-covertible value, the method will not work. The caller needs to know that.

With your method, you have three alternatives to require some integer-like value:

  • you require nothing and trust that the caller knows what they are doing.
  • you require that the caller provides an integer, or fail fast.
  • you require that the caller provides something that can be converted to an integer.

By asserting no preconditions, you retain maximum flexibility, but it is easy to to accidentally break the system. Is this flexibility worth the additional difficulty of debugging?

By requiring an integer, you lose a bit of flexibility: you can no longer accept values that quack like an integer, even if the caller knows what they are doing. However, you gain maximum debuggability – if a caller doesn't provide an integer, they'll get an exception at once. The only drawback compared to static typing is that this is only checked at runtime. That the caller has to type an extra .to_i is just a minor syntactical annoyance.

By requiring a value that can be converted to an integer, you don't actually gain anything over requiring an integer directly. The caller still has to provide something integer-convertible. You end up with an integer either way. You do however lose a lot of debuggability: if the value can't be converted to an integer, they'll see some error deep into your method. That's more confusing than an error that says “when calling this function from here, you have to provide an integer”.

This leads me to believe that dynamic languages leave us with two actual choices:

  • completely ignore any types and trust that everyone knows what they are doing. This is a valid approach for small systems.
  • completely ignore any dynamic typing and religiously check all method arguments for public methods. No implicit conversions. To quote the “Zen of Python”: “Explicit is better than implicit”. That means the caller is responsible.

Note that good method docs are necessary to explain the contract of a method, but that documentation cannot enforce a contract. In the absence of static types that would prove correctness, you'll have to write the validation code by hand.

Licensed under: CC-BY-SA with attribution
scroll top