I was reviewing a controller the other day and I came across a piece of code in rails that modified incoming
params in multiple spots. What follows is a contrived example of a solution implemented to handle a date that had to have a different format on the front-end and so had to be parsed according to the user's chosen date format.
class ShoesController < AdminController def update parse_and_format_date if shoe.update(shoe_params) redirect_to shoe_show_path else redirect_to shoe_edit_path end end def shoe_params params.require(:shoe).permit(:buy_date, :title, :id) end def parse_and_format_date params[:shoe][:buy_date] = DateTime.strptime(params[:shoe][:buy_date], user.date_format) end end
The problem I had with this controller is that we are modifying the
params in multiple places, first in
parse_and_format_dateand then in
shoe_params. Also, we aren't particularly clear about it. Without looking into
parse_and_format_date it is not immediately clear that it modifies the
For me, this is a major no-no but I had trouble articulating why. The only real answer that I could come up with was that debugging later I would have to ask myself where the changes to the
params was happening, and the more places I put the changes, the more places I would have to hunt down to ensure that I was not making a bug. Also,
parse_and_format_date does not clearly show that it updates the
params object. In this file, it's not a big deal, but the bigger the file, the worse it gets, and if you add some
concerns the next person to read this code will start scratching their head.
Source of truth
The shoe_params method should appear to be frozen in time. I should not be able to call it in multiple different places in my
update method and get a different result. What's more is that the manipulations being done to the
params variable should not be so heavy that they can't be done repeatedly if necessary. Also, strong parameters are not usually called multiple times in an action, so it's not crazy to think that it can contain these transforms.
If we made
parse_and_format_date idempotent then we could do something like this:
def shoe_params params[:shoe][:buy_date] = parse_and_format_date params.require(:shoe).permit(:buy_date, :title, :id) end def parse_and_format_date @parsed_date ||= DateTime.strptime(params[:shoe][:buy_date], user.date_format) end
In that situation, we would always now what
shoe_params contains and it would be clear at any point of any action we call in our controller.