Rubocop Metrics/CyclomaticComplexity in case when

Question

Metrics/CyclomaticComplexity: Cyclomatic complexity for notification_path is too high. [9/8] And I don't understand how to fix that. Can you help me, someone? Thanks

def notification_path
  return h.company_trial_activation_index_path if User.current.company.trial_pending?

  dot = object.eventable

  case object.event.to_sym
  when :new_app, :updates_approved, :updates_disapproved, :new_partial_app, :completed_app, :received_lead
    h.company_dot_application_path(id: dot.id)
  when :bg_check_received, :bg_check_failed, :bg_check_completed
    h.company_dot_application_background_checks_path(dot)
  when :voe_completed, :voe_corrected, :voe_invalid,
    :voe_driving_school_completed, :voe_driving_school_corrected, :voe_driving_school_invalid
    h.company_dot_application_applications_requests_path(dot_application_id: dot.id)
  when :voe_instructions, :voe_driving_school_instructions
    h.company_dot_application_applications_requests_path(dot.id)
  when :email_reply, :note, :sms_reply
    h.company_dot_application_communications_path(dot.id)
  when :follow_up_task, :follow_up_task_on_due_date
    h.company_dot_application_follow_up_tasks_path(dot_application_id: dot.id)
  when :bulk_actions_background_location, :bulk_actions_background_assign_user, :bulk_actions_background_lead_source
    h.home_users_path
  else
    h.home_users_path
  end
end

Answer 1

Before we get to cyclomatic complexity, let's address two bigger problems with that function. The giant lists of arrays, and the globals. Because the purpose of Rubocop is to make your code easier to read and maintain and reduce the possibility of bugs, not ticking things off a list.

Things would be a lot more readable if the symbol lists were named.

WHATEVER_THESE_ARE = [
  :new_app, :updates_approved, :updates_disapproved,
  :new_partial_app, :completed_app, :received_lead
].freeze
BG_CHECK_DONE = [
  :bg_check_received, :bg_check_failed, :bg_check_completed
].freeze
...and so on...

def notification_path
  return h.company_trial_activation_index_path if User.current.company.trial_pending?

  dot = object.eventable

  case object.event.to_sym
  when WHATEVER_THESE_ARE
    h.company_dot_application_path(id: dot.id)
  when BG_CHECK_DONE
    h.company_dot_application_background_checks_path(dot)
  ...and so on...
  end
end

---

Then notice that your function takes no arguments. It uses two globals, the vaguely named object and h. I don't know what these really are, the names don't help, but maybe you sanitized them for the question. Using them as globals makes the code more complex. They should be passed in as parameters, or at least descriptively named.

def notification_path(better_name_for_h, better_name_for_object)
  ...
end

---

Ok, on to cyclonic complexity. A case is basically a big if/elsif/else with each when adding a point of complexity. With a table this large it seems like there isn't much we can do, but let's start by observing that the case only depends on object.event, object.eventable (event table? why is it named dot?), and h. We can do an extract method.

def notification_path(h, object)
  return h.company_trial_activation_index_path if User.current.company.trial_pending?

  dot_to_application_path(object.event, object.eventable, h)
end

def dot_to_application_path(event, dot, h)
  case event.to_sym
  when WHATEVER_THESE_ARE
    h.company_dot_application_path(id: dot.id)
  when BG_CHECK_DONE
    h.company_dot_application_background_checks_path(dot)
  ...
  else
    h.home_users_path
  end
end

That cut off a grand total of 1 giving us 8. Others have observed the final condition is redundant, so cut that off giving 7. Sometimes you just know better than Rubocop that the logic really is this complex and can add an exception.

# rubocop:disable Metrics/CyclonicComplexity
def dot_to_application_path(event, dot, h)

This is a last resort. It's always worth considering Rubocop's complaints. We've already improved the code quite a bit by trying to fix the Rubocop issues.

Or we can dig further.

We can observe that this table is doing two things. It's selecting the appropriate path method and normalizing how it's called. Every path method takes the dot slightly differently and this makes things more complex and error prone.

If the methods were consistent we could replace the case with a hash lookup.

WHATEVER_THESE_ARE = [
  :new_app, :updates_approved, :updates_disapproved,
  :new_partial_app, :completed_app, :received_lead
].freeze
BG_CHECK_DONE = [
  :bg_check_received, :bg_check_failed, :bg_check_completed
].freeze
...

# You'll want to put this somewhere else.
def flatten_keys(hash)
  hash.each_with_object({}) { |(keys, value), new_hash|
    keys.each { |key| new_hash[event] = value }
  }
end  

EVENT_TO_APPLICATION_PATH_METHOD = flatten_keys({
  WHATEVER_THESE_ARE => :company_dot_application_path,
  BG_CHECK_DONE => :company_dot_application_background_checks_path,
  ...
}).freeze

def notification_path(h, object)
  return h.company_trial_activation_index_path if User.current.company.trial_pending?

  method = EVENT_TO_DOT_APPLICATION_PATH_METHOD[object.event.to_sym] || :home_users_path
  h.send(method, dot)
end

Now we can see how simple this code can be!

To accomplish you could refactor the methods so they are consistent, or you can write wrapper methods which make it consistent. Refactoring is the preferable option, but that might be well out of your scope.

Finally, this might all be complex enough to warrant putting all the logic into a class.

Answer 2

I noticed that you use

h.company_dot_application_applications_requests_path(dot_application_id: dot.id)

in one place and

h.company_dot_application_applications_requests_path(dot.id)

in another. If these calls are equal, the method can be simplified a bit:

def notification_path
  return h.company_trial_activation_index_path if User.current.company.trial_pending?

  dot = object.eventable

  case object.event.to_sym
  when :new_app, :updates_approved, :updates_disapproved, :new_partial_app, :completed_app, :received_lead
    h.company_dot_application_path(id: dot.id)
  when :bg_check_received, :bg_check_failed, :bg_check_completed
    h.company_dot_application_background_checks_path(dot)
  when :voe_completed, :voe_corrected, :voe_invalid, :voe_driving_school_completed, :voe_driving_school_corrected, :voe_driving_school_invalid, :voe_instructions, :voe_driving_school_instructions
    h.company_dot_application_applications_requests_path(dot.id)
  when :email_reply, :note, :sms_reply
    h.company_dot_application_communications_path(dot.id)
  when :follow_up_task, :follow_up_task_on_due_date
    h.company_dot_application_follow_up_tasks_path(dot_application_id: dot.id)
  else
    h.home_users_path
  end
end

Answer 3

Just a comment to say that I really disagree with @Schwern answer.

The purpose of this rubocop rule is to improve readability, because a high cyclomatic complexity is often an indication of bad readability (but not always).

I think your statements could be refactor and improved, but the switch-case pattern is here far more appropriate than meta-programming !

Meta-programming will obfuscate the business logic of that code and make thing harder to refactor later.

Against complexity, the key is simplicity, not more complexity :).

I don't have the whole context, but I have so clues :

• try to refactor your when conditions. Each one should correspond to a specific and clear business case
• company_dot_application_applications_requests_path interface does not seems to be optimal. Keep in mind that clear class and module interface (methods, Constants, etc...) is one key point of clean code
• remove the last when (useless)