0
\$\begingroup\$

I have a nasty (IMO) piece of code for a datatable configuration, how do i best remove the duplication here?

I've tried the obvious things I know (collapsing into one statement and trying inline conditionals) but it isn't working and I can't help thinking there must be a more elegant Ruby way here.

if show_status == "true"
  if show_requested_by == "true"
    records.map do |record|
      {
        warning: as_warning(record),
        updated_at: as_time_ago(record.updated_at),
        status: as_status(record.status),
        user: record.user.nil? ? "" : record.user.formal_name,
        manager: record.manager.nil? ? "" : record.manager.formal_name,
        first_date: as_date(record.first_date),
        last_date: as_date(record.last_date),
        days: as_number(record.total_days),
        hours: as_number(record.total_hours),
        units: as_number(record.total_units),
        value: as_number(record.total_value),
        comment: as_comment(record.comment),
        actions: as_actions(record)
      }
    end
  else
    records.map do |record|
      {
        warning: as_warning(record),
        updated_at: as_time_ago(record.updated_at),
        status: as_status(record.status),
        manager: record.manager.nil? ? "" : record.manager.formal_name,
        first_date: as_date(record.first_date),
        last_date: as_date(record.last_date),
        days: as_number(record.total_days),
        hours: as_number(record.total_hours),
        units: as_number(record.total_units),
        value: as_number(record.total_value),
        comment: as_comment(record.comment),
        actions: as_actions(record)
      }
    end
  end
else
  if show_requested_by == "true"
    records.map do |record|
      {
        warning: as_warning(record),
        updated_at: as_time_ago(record.updated_at),
        user: record.user.nil? ? "" : record.user.formal_name,
        manager: record.manager.nil? ? "" : record.manager.formal_name,
        first_date: as_date(record.first_date),
        last_date: as_date(record.last_date),
        days: as_number(record.total_days),
        hours: as_number(record.total_hours),
        units: as_number(record.total_units),
        value: as_number(record.total_value),
        comment: as_comment(record.comment),
        actions: as_actions(record)
      }
    end
  else
    records.map do |record|
      {
        warning: as_warning(record),
        updated_at: as_time_ago(record.updated_at),
        manager: record.manager.nil? ? "" : record.manager.formal_name,
        first_date: as_date(record.first_date),
        last_date: as_date(record.last_date),
        days: as_number(record.total_days),
        hours: as_number(record.total_hours),
        units: as_number(record.total_units),
        value: as_number(record.total_value),
        comment: as_comment(record.comment),
        actions: as_actions(record)
      }
    end
  end
end
\$\endgroup\$
1
\$\begingroup\$

Another way to do this would be to inline the conditionals, returning nil when they are not met and use compact or compact! to remove those entries at the end. Something like:

records.map do |record|
  {
    warning: as_warning(record),
    updated_at: as_time_ago(record.updated_at),
    status: show_status == 'true' ? as_status(record.status) : nil,
    user: show_requested_by=='true' ? (record.user.nil? ? "" : record.user.formal_name) : nil,
    manager: record.manager.nil? ? "" : record.manager.formal_name,
    first_date: as_date(record.first_date),
    last_date: as_date(record.last_date),
    days: as_number(record.total_days),
    hours: as_number(record.total_hours),
    units: as_number(record.total_units),
    value: as_number(record.total_value),
    comment: as_comment(record.comment),
    actions: as_actions(record)
  }.compact
}

Note: If any of your entries can be nil (and it is important to keep them in the hash) then this won't work.

Update: As an alternative that handles nils that are expected you could also go for something like:

records.map do |record|
  {
    status: show_status == 'true' ? as_status(record.status) : nil,
    user: show_requested_by=='true' ? (record.user.nil? ? "" : record.user.formal_name) : nil,
  }.compact
  .merge {
    warning: as_warning(record),
    updated_at: as_time_ago(record.updated_at),

    manager: record.manager.nil? ? "" : record.manager.formal_name,
    first_date: as_date(record.first_date),
    last_date: as_date(record.last_date),
    days: as_number(record.total_days),
    hours: as_number(record.total_hours),
    units: as_number(record.total_units),
    value: as_number(record.total_value),
    comment: as_comment(record.comment),
    actions: as_actions(record)
  }
}

Although, if the user or status, should be nil they still won't be handled correctly.

\$\endgroup\$
  • \$\begingroup\$ yes, this works nicely, as you say just needed to ensure necessary nils become blanks instead. \$\endgroup\$ – Craig McGuff Jan 4 '18 at 7:06
  • \$\begingroup\$ Setting those two values to nil will make no difference because attempting to access them if they do not exist will still return nil. It only matters if you're either 1) Checking against the array of keys or 2) Changing the hash defaults. \$\endgroup\$ – thesecretmaster Jan 6 '18 at 14:41
  • \$\begingroup\$ There are lots of functions that chek for nils, for example in ActiveRecord, User.assign({login: nil}) is not the same as User.assign({}) \$\endgroup\$ – Marc Rohloff Jan 7 '18 at 0:38
2
\$\begingroup\$

This is just a rough thought, so it's untested, but here goes:

A vast majority of the hash is the same every time. So, that vast majority can be stuck into a method. There are two fields that are conditionally added to that hash based on their values ('true' or... not). So, we can create a method which takes a record as an argument and the two 'true'/not options and generates a hash accordingly:

def record_for(record, status: false, requested_by: false)
  record_hash = {
    warning: as_warning(record),
    updated_at: as_time_ago(record.updated_at),
    manager: record.manager.nil? ? "" : record.manager.formal_name,
    first_date: as_date(record.first_date),
    last_date: as_date(record.last_date),
    days: as_number(record.total_days),
    hours: as_number(record.total_hours),
    units: as_number(record.total_units),
    value: as_number(record.total_value),
    comment: as_comment(record.comment),
    actions: as_actions(record)
  }
  record_hash[:status] = as_status(record.status) if status
  record_hash[:user] = record.user.nil? ? "" : record.user.formal_name if requested_by
  record_hash
end

Then, all that remains is to convert the strings to booleans:

def to_bool(str)
  return true if str == 'true'
  false
end

and then iterate over the records:

status = to_bool(show_status)
requested_by = to_bool(show_requested_by)

records.map { |r| record_for(r, status: status, requested_by: requested_by) }

I'd be happy to go more in depth and test the code if you provided a set of test records that I could use or some larger context this code would run in.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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