Ruby Cyclomatic and Perceived complexity

Question

I have this service object that parses an excel file and persists the data in it. Rubocop is flagging it as:

app/services/csv_upload.rb:17:3: C: Metrics/AbcSize: Assignment Branch Condition size for parse_spreadsheet is too high. [<14, 34, 11> 38.38/35]
  def parse_spreadsheet ...
  ^^^^^^^^^^^^^^^^^^^^^
app/services/csv_upload.rb:17:3: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for parse_spreadsheet is too high. [10/6]
  def parse_spreadsheet ...
  ^^^^^^^^^^^^^^^^^^^^^
app/services/csv_upload.rb:17:3: C: Metrics/PerceivedComplexity: Perceived complexity for parse_spreadsheet is too high. [11/7]

How do i fix this? How should I refactor this code? The parse_spreadsheet method is where it is being flagged on.

require "spreadsheet"

class CsvUpload
  attr_accessor :review

  def initialize(file)
    Spreadsheet.client_encoding = "UTF-8"
    tempfile = File.open(file.tempfile)

    @spreadsheet = Spreadsheet.open tempfile
    @exception_sheet = @spreadsheet.worksheet 3
    @review = []

    parse_spreadsheet
  end

  def parse_spreadsheet
    @exception_sheet.each_with_index 4 do |row, index|
      employee_no = row[1]
      department = row[2]
      date = row[3].to_datetime
      time_in = row[4].to_datetime unless row[4].nil?
      time_out = row[5].to_datetime unless row[5].nil?

      if department == "Night"
        next_row = @exception_sheet.to_a[index + 5]
        next_date = next_row[3].to_datetime
        next_time_in = next_row[4].to_datetime unless next_row[4].nil?
        start_time = set_date_time(date, time_out) unless time_out.nil?
        end_time = set_date_time(next_date, next_time_in) unless next_time_in.nil?
      else
        start_time = set_date_time(date, time_in) unless time_in.nil?
        end_time = set_date_time(date, time_out) unless time_out.nil?
      end

      @employee = Employee.find_by(employee_no: employee_no)

      next if @employee.nil?
      @attendance_item = AttendanceItem.create(
        start_time: start_time, end_time: end_time, employee_id: @employee.id
      )
    end
  end

  def to_review
    @review
  end

  private

  def set_date_time(date, time)
    DateTime.new(
      date.year,
      date.month,
      date.day,
      time.hour,
      time.min,
      time.sec,
      "+0800"
    ).in_time_zone
  end
end

That's right - strip out most of the innards of parse_spreadsheet into their own submethods - you're being told that there is too much logic inside that one method — Mark, Jan 21 '20 at 09:32
If you want more reading on how it's calculated, check out https://blog.usejournal.com/ruby-software-complexity-metrics-part-one-prerequisites-b6f86d76a4a5 — Mark, Jan 21 '20 at 09:33

score 2 · Accepted Answer · answered Jan 21 '20 at 09:46

I'd start by having a separate method for each of your items in @exception_sheet:

def parse_spreadsheet
  @exception_sheet.each_with_index 4 do |row, index|
    handle_exception_row(row, index)
  end
end

def handle_exception_row(row, index)
  employee_no = row[1]
  department = row[2]
  date = row[3].to_datetime
  time_in = row[4].to_datetime unless row[4].nil?
  time_out = row[5].to_datetime unless row[5].nil?

  if department == "Night"
    next_row = @exception_sheet.to_a[index + 5]
    next_date = next_row[3].to_datetime
    next_time_in = next_row[4].to_datetime unless next_row[4].nil?
    start_time = set_date_time(date, time_out) unless time_out.nil?
    end_time = set_date_time(next_date, next_time_in) unless next_time_in.nil?
  else
    start_time = set_date_time(date, time_in) unless time_in.nil?
    end_time = set_date_time(date, time_out) unless time_out.nil?
  end

  @employee = Employee.find_by(employee_no: employee_no)

  next if @employee.nil?
  @attendance_item = AttendanceItem.create(
    start_time: start_time, end_time: end_time, employee_id: @employee.id
  )
 end
end

That fixes the parse_spreadsheet method but creates new errors with handle_exception_row. Then stripping that down:

def handle_exception_row(row, index)
  employee_no = row[1]
  department = row[2]
  date = row[3].to_datetime
  time_in = row[4].to_datetime unless row[4].nil?
  time_out = row[5].to_datetime unless row[5].nil?

  handle_attendance_creation(employee_no: employee_no, department: department, date: date, time_in: time_in, time_out: time_out)
end

def handle_attendance_creation(employee_no:, department:, date:, time_in:, time_out:)
  if department == "Night"
    next_row = @exception_sheet.to_a[index + 5]
    next_date = next_row[3].to_datetime
    next_time_in = next_row[4].to_datetime unless next_row[4].nil?
    start_time = set_date_time(date, time_out) unless time_out.nil?
    end_time = set_date_time(next_date, next_time_in) unless next_time_in.nil?
  else
    start_time = set_date_time(date, time_in) unless time_in.nil?
    end_time = set_date_time(date, time_out) unless time_out.nil?
  end

  @employee = Employee.find_by(employee_no: employee_no)

  next if @employee.nil?
  @attendance_item = AttendanceItem.create(
    start_time: start_time, end_time: end_time, employee_id: @employee.id
  )
 end
end

You'll probably still get an error on handle_attendance_creation, but hopefully you can see where this is going. Strip out further smaller methods that take the required arguments and you'll get there

Ruby Cyclomatic and Perceived complexity

1 Answers1