I have a simple method in my Purchases controller that tweaks user form input from the params hash, prior to applying strong parameters.
def sanitize_params
params[:purchase][:invoice] = nil if params[:purchase][:invoice] == ''
params[:purchase][:note] = nil if params[:purchase][:note] == ''
params[:purchase][:product_id] = nil if params[:purchase][:product_id].blank?
end
Rubocop arrests for Metrics/AbcSize… [<3, 19, 5> 19.87/17]
.
How is the ‘Branch’ in the ABC metric calculated as 19?
I can refactor to:
def sanitize_params
unsanitized_purchase_params = params[:purchase]
params[:purchase][:invoice] = nil if unsanitized_purchase_params[:invoice] == ''
params[:purchase][:note] = nil if unsanitized_purchase_params[:note] == ''
params[:purchase][:product_id] = nil if unsanitized_purchase_params[:product_id].blank?
end
This then passes, but at the expense of readability and extra code. Why is ‘Branch’ 19 in the original code? Should I care? (My actual use case has an extra similar line so the metric is even higher there). Is there a better approach? Thanks Daniel
CodePudding user response:
As for now Rails parameters do not implement #deep_transform_values!
, but you can do following:
def sanitize_params
%i[invoice note product_id].each do |param_name|
params[:purchase][param_name] = params[:purchase][param_name].presence
end
end
Or:
def sanitize_params
params[:purchase] = params[:purchase].transform_values(&:presence)
end
Upd. you can also consider using https://github.com/rmm5t/strip_attributes - this gem will replace empty values with nils on model level
CodePudding user response:
As given by the documentation:
Branch -- an explicit forward program branch out of scope -- a function call, class method call, or new operator
So, the "Branch" in the ABC is calculated as "anything that jumps out of the current method".
In the method you described,
def sanitize_params
params[:purchase][:invoice] = nil if params[:purchase][:invoice] == ''
params[:purchase][:note] = nil if params[:purchase][:note] == ''
params[:purchase][:product_id] = nil if params[:purchase][:product_id].blank?
end
these are the 19 calls that need to leave the current method:
01. params (on line 1, before the assignment)
02. params[:purchase] (on line 1, before the assignment)
03. params[:purchase][:invoice] (on line 1, before the assignment)
04. params (on line 1, after the assignment)
05. params[:purchase] (on line 1, after the assignment)
06. params[:purchase][:invoice] (on line 1, after the assignment)
07. params (on line 2, before the assignment)
08. params[:purchase] (on line 2, before the assignment)
09. params[:purchase][:note] (on line 2, before the assignment)
10. params (on line 2, after the assignment)
11. params[:purchase] (on line 2, after the assignment)
12. params[:purchase][:note] (on line 2, after the assignment)
13. params (on line 3, before the assignment)
14. params[:purchase] (on line 3, before the assignment)
15. params[:purchase][:product_id] (on line 3, before the assignment)
16. params (on line 3, after the assignment)
17. params[:purchase] (on line 3, after the assignment)
18. params[:purchase][:product_id] (on line 3, after the assignment)
19. params[:purchase][:product_id].blank? (on line 3, after the assignment)
Important ponts: whenever you call params
, you're accessing a different method, and everytime you access an index on the hash, you are calling a method over this hash to access the desired index.
About if you should care: the ABC size is a standard to measure code quality, so it's usually a good call to stick with it, whenever it makes sense, of course.
But there are some ways how you could refator the code to decrease the ABC complexity. One suggestion would be the following:
def sanitize_params
params.tap do |params|
params[:purchase][:invoice] = nil if params.dig(:purchase, :invoice) == ''
params[:purchase][:note] = nil if params.dig(:purchase, :note) == ''
params[:purchase][:product_id] = nil if params.dig(:purchase, :product_id).blank?
end
end
In this example, the Branch complexity decreases a lot because we only call the external params
once and apply all the changes inside the tap
block. Also, by using the dig
method over the params, we call only one method to go as deep as we want to go.
Another idea would be to break this method into more specific methods to sanitize each attribute, for instance.
I hope this can be helpful.