Reduce AbcSize metric when sanitizing params hash

user_9678200 0
Viewed 79

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

rubocop ruby-on-rails
2 Answers

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

user_97759920

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.

user_72872030
Related