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?