Setting the default display name for my model

Question

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

Answer 1

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

Answer 2

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.

Answer 3

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.

Answer 4

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.