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
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?