mediafinger · October 25, 2022 07:23 · Oct 25, 2022 · Oct 25, 2022 · Oct 24, 2022 · Oct 24, 2022
diff --git a/refactoring_example.markdown b/refactoring_example.markdown
@@ -64,7 +64,7 @@ def create
   template = authorize_from_uuid(create_params[:template_uuid], RouteTemplate)
   vehicle = authorize_from_uuid(create_params[:vehicle_uuid], Vehicle)
 
-  route_event = RouteEvent.new(company:, event_date:, project:, template:, vehicle:)
+  route_event = RouteEvent.new(company:, project:, template:, vehicle:, event_date: create_params[:event_date])
   authorize! :create, route_event
 
   route_event.save!

diff --git a/refactoring_example.markdown b/refactoring_example.markdown
@@ -6,7 +6,7 @@ I’ve found some _confusing code_ in a project and would like to explain how an
 
 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 only one known to the frontends._
+> _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:_
 ```ruby

diff --git a/refactoring_example.markdown b/refactoring_example.markdown
@@ -6,6 +6,8 @@ I’ve found some _confusing code_ in a project and would like to explain how an
 
 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 only one known to the frontends._
+
 _A snippet of the RouteEventsController:_
 ```ruby
     def route_event_params

diff --git a/refactoring_example.markdown b/refactoring_example.markdown
@@ -0,0 +1,108 @@
+# 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...
+
+_A snippet of the RouteEventsController:_
+```ruby
+    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:
+
+```ruby
+# 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:, event_date:, project:, template:, vehicle:)
+  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:
+
+```ruby
+  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:
+
+```ruby
+  # 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!
No results found