15
\$\begingroup\$

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
end
\$\endgroup\$
0

4 Answers 4

10
\$\begingroup\$

Notes:

  • Conditionals are expressions in Ruby, you can (and, idiomatically, should) move the assignment outside.
  • active_record_relation.count == 0 is ok, but active_record_relation.empty? is more declarative.

I'd write:

def set_default_display_name
  credentials_by_type = user.credentials.where(type: type)  
  credentials_by_name = user.credentials.where(display_name: type)

  self.display_name = case
  when display_name.present?
    display_name
  when credentials_by_type.empty? && credentials_by_name.empty?
    name
  else
    "#{name} #{credentials_by_type.count + 1}"
  end
end
\$\endgroup\$
10
  • \$\begingroup\$ I only set count as for the the first expression because it is being use in the else condition. \$\endgroup\$ Apr 9, 2015 at 15:45
  • 1
    \$\begingroup\$ Maybe when should be indented? \$\endgroup\$
    – Caridorc
    Apr 9, 2015 at 15:46
  • 1
    \$\begingroup\$ Usually people does not indent it. The ruby-style-guide says no indentation: github.com/bbatsov/ruby-style-guide \$\endgroup\$
    – tokland
    Apr 9, 2015 at 15:46
  • \$\begingroup\$ Thanks, it still complains but this is an improvement. 18.36/15. Rubocop does complain that when should be indented as deep as case. \$\endgroup\$ Apr 9, 2015 at 15:50
  • 1
    \$\begingroup\$ Yep, your original code has a bug then. Anyway, updated here. \$\endgroup\$
    – tokland
    Apr 9, 2015 at 19:21
12
\$\begingroup\$

The method is doing three things:

  1. Determining whether or not to set display_name.
  2. Determining the default display name.
  3. Setting display_name to 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}"
end

Extracting 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
end

We're far afield from the question at that point though.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ This solution should be the accepted answer as is way more readable and really follows the spirit of clean code. \$\endgroup\$
    – BooVeMan
    Nov 11, 2015 at 12:28
  • \$\begingroup\$ Abc metric provided the "ah ha!" moment for me. excellent answer \$\endgroup\$ Apr 8, 2016 at 2:34
  • 1
    \$\begingroup\$ Two return points in a 3-line code does not seem to be a good ruby practice. \$\endgroup\$ Sep 7, 2016 at 20:32
2
\$\begingroup\$

Unless I'm mistaken, you can flip just the conditional: display_name will be set to name is either of the counts is zero. So that's an OR clause on the counts being zero, rather than an AND clause on the counts being non-zero.

So that plus ||= and a begin..end will let you do:

def set_default_display_name
  self.display_name ||= begin
    credentials_by_type = user.credentials.where(type: type).count
    credentials_by_name = user.credentials.where(display_name: name).count

    if credentials_by_type.zero? || credentials_by_name.zero?
      self.display_name = "#{name} #{credentials_by_type + 1}"
    else
      self.display_name = name
    end
  end
end

I'm assuming here, that blank? is checking for nil specifically, so you can use ||=. However, if display_name can be an empty string that won't work. In that case, I'd personally keep the early return you have now.

def set_default_display_name
  return unless self.display_name.blank?
  credentials_by_type = user.credentials.where(type: type).count
  credentials_by_name = user.credentials.where(display_name: name).count

  if credentials_by_type.zero? || credentials_by_name.zero?
    self.display_name = "#{name} #{credentials_by_type + 1}"
  else
    self.display_name = name
  end
end

I don't know what Rubocop will think of either of these solutions, though.

\$\endgroup\$
1
  • \$\begingroup\$ .display_name can be an empty string so the early return would likely be best \$\endgroup\$ Apr 9, 2015 at 19:50
0
\$\begingroup\$

Read the comments in the code for an explanation. This is the code of the selected answer, more optimized/DRY.

def set_default_display_name

  # - 'count' takes less memory, faster execution than 'where' results
  #
  by_type = user.credentials.where(type: type).count
  by_name = user.credentials.where(display_name: type).count

  self.display_name ||= ((by_type + by_name) == 0 ? name : "#{name} #{by_type + 1}")
end

update 1 (2016-09-08): Reference code removed from comments. Now the comments explain only the optimizations to the selected answer.

\$\endgroup\$
8
  • \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. \$\endgroup\$
    – Mast
    Sep 7, 2016 at 9:04
  • \$\begingroup\$ @Mast The comments seem to be embedded in the code here. It's not ideal though. In a way this could be a comment (or multiple) to the existing answer, but right now this is an answer (it's not a very good answer, but it is an answer) \$\endgroup\$ Sep 7, 2016 at 10:39
  • \$\begingroup\$ I'd recommend reducing the number of commented lines in this answer. It has potential, but for now I am definitely downvoting. \$\endgroup\$ Sep 7, 2016 at 10:40
  • \$\begingroup\$ @Mast This is not an alternate solution, just more optimized code for the selected answer. That is why the code from the selected answer is also put in comments as a reference. \$\endgroup\$ Sep 7, 2016 at 20:09
  • 1
    \$\begingroup\$ I removed my downvote but I would recommend not putting your explanation of changes within the code block. Answers are better that way. \$\endgroup\$ Sep 7, 2016 at 20:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.