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_date
and 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 params
.
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.