I'm a Floridian web engineer who planted his roots in Paris. Some of my favorite past-times are analyzing and optimizing development processes, building solid APIs, mentoring junior devs, and long walks on the beach.

Control your points of modification

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