patternrubyrailsModerate
Setting the default display name for my model
Viewed 0 times
thesettingdefaultnamefordisplaymodel
Problem
I have a method in my Ruby code that sets the default name for my model. Rubocop complains that Assignment Branch Condition Size is too high, 21.24/15. How can I improve this?
def set_default_display_name
return unless display_name.blank?
count = user.credentials.where(type: type).count
if count == 0
self.display_name = name
elsif user.credentials.where(display_name: name).count == 0
self.display_name = name
else
self.display_name = "#{name} #{count + 1}"
end
endSolution
The method is doing three things:
Each of those adds to the Abc metric. The biggest contributor to the Abc metric is determining the default display name. To me, that also seems like the most logically separate. We can lower the Abc metric by extracting that responsibility to its own method.
Extracting
At this point, I'd question whether
Another option, is to keep your code as is and modify your .rubocop.yml file to increase the threshold of the Abc metric, or disable it entirely. The Abc metric is all about code size, not complexity. Theoretically, bugs increase with code size. You'd have to determine if this low of a threshold makes it easier to mask bugs. (Based on an earlier thread, the answer is probably, "yes", since your original code seems to have a bug. I retained the functionality of your original code in this example.)
--- Addendum ---
@ramonrails makes a good point in the comments. I'm usually fine with
This answer was only addressing the ABC question. If this were my code, I'd extract the credentials logic completely from this "view" code.
Something like:
We're far afield from the question at that point though.
- Determining whether or not to set
display_name.
- Determining the default display name.
- Setting
display_nameto the default value.
Each of those adds to the Abc metric. The biggest contributor to the Abc metric is determining the default display name. To me, that also seems like the most logically separate. We can lower the Abc metric by extracting that responsibility to its own method.
def set_default_display_name
self.display_name = default_display_name if display_name.blank?
end
def default_display_name
credentials_of_type = user.credentials.where(type: type)
return name if credentials_of_type.none? || user.credentials.where(display_name: name).none?
"#{name} #{credentials_of_type.count + 1}"
endExtracting
default_display_name makes it slightly simpler to test the default display name logic. You could also use that logic elsewhere, e.g., a UI could show the current display name and ask if they want to reset it to the default display name.At this point, I'd question whether
set_default_display_name is necessary or if it could be inlined.Another option, is to keep your code as is and modify your .rubocop.yml file to increase the threshold of the Abc metric, or disable it entirely. The Abc metric is all about code size, not complexity. Theoretically, bugs increase with code size. You'd have to determine if this low of a threshold makes it easier to mask bugs. (Based on an earlier thread, the answer is probably, "yes", since your original code seems to have a bug. I retained the functionality of your original code in this example.)
--- Addendum ---
@ramonrails makes a good point in the comments. I'm usually fine with
return guard clauses at the top of ruby methods. However, I agree that having a return in the middle of this method is not ideal.This answer was only addressing the ABC question. If this were my code, I'd extract the credentials logic completely from this "view" code.
Something like:
def default_display_name(type_credentials)
type_credentials.any? ? "#{name} #{type_credentials.count + 1}" : name
endWe're far afield from the question at that point though.
Code Snippets
def set_default_display_name
self.display_name = default_display_name if display_name.blank?
end
def default_display_name
credentials_of_type = user.credentials.where(type: type)
return name if credentials_of_type.none? || user.credentials.where(display_name: name).none?
"#{name} #{credentials_of_type.count + 1}"
enddef default_display_name(type_credentials)
type_credentials.any? ? "#{name} #{type_credentials.count + 1}" : name
endContext
StackExchange Code Review Q#86405, answer score: 12
Revisions (0)
No revisions yet.