assigenment branching condition is too high robocop offense

Question

I have written and optimized the code like this

  def issues_json_for_v2(skip_avatar = false)
    @result_json ||= { results: [] }
    return if @result_set['issues'].empty?
    @result_set['total_entries'] = @result_set['meta']['total']
    fr_issues = @result_set['issues']
    user_hash = fr_results_hash(@result_set['users'])
    project_data = fr_results_hash(@result_set['projects'])
    if @suggest
      add_suggest_results_fr_issues(fr_issues, project_data)
    else
      fr_issues.each do |issue|
        p_data = project_data[issue['project_id']] if issue
        if issue && issue['owner_id']
          issue_json = append_fr_user_details_to_issue(safe_send(:fr_issue_json, issue, p_data), user_hash)
        else
          issue_json = safe_send(:fr_issue_json, issue, p_data)
        end
        @result_json[:results] << issue_json
      end
    end
    return @result_json[:results] if @size.nil?
    add_fr_prj_meta_data(fr_issues)
    @result_json[:results]
  end

Still I am getting

Assignment Branch Condition size for issues_json_for_v2 is too high.

what optimization I can do while keeping the readability in the mind.

This is another method for which I am encountering this issue

  def project_json(project, id)
    human_display_id = project['key']
    title = sanitize_fr_data(project['name'])
    description = project['description'].blank? ? '' : project['description']
    description = sanitize_fr_data(description)
    prg = project['progress']
    percent_completion = (prg['done'] == 0) ? 0 : (prg['done'] * 100) / (prg['todo'] + prg['in_progress'] + prg['done'])
    current_time_zone = ActiveSupport::TimeZone::MAPPING[current_user.time_zone]
    start_date = project['start_date'].nil? ? '--' : project['start_date'].in_time_zone(current_time_zone).strftime(date_time_format)
    end_date = project['end_date'].nil? ? '--' : project['end_date'].in_time_zone(current_time_zone).strftime(date_time_format)
    return { result_type: 'project', content: %{#{title} (#{human_display_id})}, path: fr_project_path(project['key']) } if @suggest
    {
      id: id,
      title: title,
      project_display_id: human_display_id,
      description: truncate(description, length: 250),
      owner: project['owner_id'],
      start_date: start_date,
      end_date: end_date,
      progress: {todo: project['progress']['todo'], in_progress: project['progress']['in_progress'], done: project['progress']['done']},
      percent_completion: percent_completion,
    }
  end

Answer 1

ABC metrics are there specifically to help with readability. As a general rule, if you hit the ABC limit, the code is likely to be a lot less readable than you would think because the method is simply doing too much.

I would suggest:

• think about how many different ways there are through the method (branches)
• think about how you would go about testing all of those branches, or at least executing every code path

If you're struggling to figure out what that would really take, then you have a good idea about what the ABC metric is pointing out.

To refactor your code, I would:

• extract @result_json to be behind an accessor method that will ensure it is always created, then in your issues method call result_json instead of @result_json

def result_json
  @result_json ||= { results: [] }
end

• separate out your logic dealing with @result_set - consider creating a class that wraps result set and provides appropriate accessors, e.g.

class ResultSet
  def initialize(hsh)
    @hsh = hsh
  end

  def total_entries
    @hsh['meta']['total']
  end

  def issues
    @hsh['issues']
  end

  def users
    # obviously... you need the fr_results_hash method available here too
    @user_hash ||= fr_results_hash(@hsh['users'])
  end

  def project_data
    @project_data ||= fr_results_hash(@hsh['projects'])
  end
end

then this allows you to use this in your method like result_set = ResultSet.new(@result_set) and call directly to those instance methods like result_set.project_data instead of creating a local variable to hold each of those data.

• Assuming that @result_set['issues'] is an Array, calling .compact on the array (fr_issues.compact.each do |issue|) will remove all of the nil values from the array, which is I think what you're guarding against when checking if issue in your fr_issues.each block. This means that you can remove those conditions because they can't happen.

More generally, there is a very high use of @variables in that method. That makes the method very hard to understand without context. Consider if those really should be instance methods or should they be scoped to within the method itself.

Your second method is having issues because of the overuse of ternary operations and needing to assign those to variables in the method. Creating some helper methods will assist, such as instead of:

start_date = project['start_date'].nil? ? '--' : project['start_date'].in_time_zone(current_time_zone).strftime(date_time_format)
end_date = project['end_date'].nil? ? '--' : project['end_date'].in_time_zone(current_time_zone).strftime(date_time_format)

have:

def format_date(value)
  value.nil? ? '--' : value.in_time_zone(current_time_zone).strftime(date_time_format)
end

then in your method, just use format_date(project['start_date']) and format_date(project['end_date']). Because there is no complexity with calling those, just call them directly in your final hash instead of assigning them to a variable. Similarly, adding a method def project_completion_percent(progress) then calling project_completion_percent(project['progress']) directly in your return hash will get rid of 2 assignments and a branch. Another idea... a method to format your description will mean that you can get rid of 2 lines of code from the method, 2 assignments and a branch.

There are plenty more optimisations, but hopefully that gives enough pointers