Refactoring Example

I’ve found some confusing code in a project and would like to explain how and why I refactored it. As this is a small, very focused refactoring one can do in a "drive-by" manner while implementing another small change in this method, I think it serves well as an example.

Before

The first thing that confused me was that our frontend is not sending the parameters we explicitly permit in the route_event_params method (see below), instead it is sending *_uuid parameters...

And yes, this project unfortunately has id as primary_key and later added uuid which is the one known to the frontends.

A snippet of the RouteEventsController:

    def route_event_params
      params.permit(:event_date, :template_id, :company_id, :project_id, :vehicle_id) 
    end

    def create
      transform_and_authorize_uuid(:company, Company)
      transform_and_authorize_uuid(:project, Project)
      transform_and_authorize_uuid(:template, RouteTemplate)
      transform_and_authorize_uuid(:vehicle, Vehicle)

      route_event = RouteEvent.new(route_event_params)
      authorize! :create, route_event

      route_event.save!

      render json: route_event, root: 'route_event', serializer: RouteEventSerializer
    end

And then I was wondering, how anything actually works, when we only intialize RouteEvent.new(event_date: params[:event_date]) without any of the associations...

Reading this code makes it really hard to determine which parameters are actually coming from the frontend and what is happening.

Spoiler: the frontend passes: :event_date, :template_uuid, :company_uuid, :project_uuid, :vehicle_uuid - which we then rewrite to the *_id versions (after loading and authorizing the objects).

Which means our “strong” parameter method is rather weak.

After

So I refactored this to:

# Improvements:
# * better naming, as the `route_event_params` were only used in the `create` method
#   * and we already know we are in the `RouteEventsController`
# * alphabetical sorting
# * actually checking the parameters that are send by the frontend

def create_params
   params.permit(:company_uuid, :event_date, :project_uuid, :template_uuid, :vehicle_uuid)
end

# Improvements:
# * uses the strong parameters method
# * explicitly mentions which parameters are used
# * explicitly lists the data used to create the RouteEvent
# * I can now look at this action and directly understand what is happening

def create
  company = authorize_from_uuid(create_params[:company_uuid], Company)
  project = authorize_from_uuid(create_params[:project_uuid], Project)
  template = authorize_from_uuid(create_params[:template_uuid], RouteTemplate)
  vehicle = authorize_from_uuid(create_params[:vehicle_uuid], Vehicle)

  route_event = RouteEvent.new(company:, project:, template:, vehicle:, event_date: create_params[:event_date])
  authorize! :create, route_event

  route_event.save!

  render json: route_event, root: 'route_event', serializer: RouteEventSerializer
end

Bonus

Obviously I had to introduce a new helper method for this, but even this is much simpler! Instead of using the existing method which makes it un-obvious to understand what is happening:

  def transform_and_authorize_uuid(param_name, associated_class)
    uuid = params["#{param_name}_uuid".to_sym]
    return if uuid.blank?

    associated_record = associated_class.find_by!(uuid:)
    authorize! :associate, associated_record
    params["#{param_name}_id".to_sym] = associated_record.id
    params.delete "#{param_name}_uuid".to_sym
  end

Spoiler: it rewrites params

I’ve added and use now:

  # Improvements:
  # * readability
  # * does not rewrite params
  # * can be used for other `authorize_actions`
  # * returns the record

  def authorize_from_uuid(uuid, klass, authorize_action: :associate)
    record = klass.find_by!(uuid:)

    authorize! authorize_action, record
  end

Which looks like another win to me!

mediafinger/refactoring_example.markdown

Refactoring Example

Before

After

Bonus