CODEWITHSUNDEEP
ruby-on-railsruby
greyoxide
greyoxide

Reputation: 1287

Rails - is there a better way to write this? looking for refactor advice

In my limited knowledge I dont know how to write this without passing the volunteer as an argument to the status method. Which normally wouldnt be an issue, except when I need all irregular volunteers. Then I have to loop through Volunteers.all and evaluate each against the status method. I feel like the overhead here is too great.

Class Volunteer < ActiveRecord::Base

  has_many :reports

  def status(volunteer)
    case
    when volunteer.reports.nought.where(:report_month => Report_range.months(6)).count > 5
        return 'inactive'
    when volunteer.reports.nought.where(:report_month => Report_range.months(6)).count >= 3
        return 'irregular'
    else
        return 'active'
    end
  end
end


class Report < ActiveRecord::Base
  belongs_to :volunteer
  scope :nought, -> {where("hours IS NULL") || where("hours: 0")}
end

module Report_range
  def self.months(range)
   Volunteer.first.reports.order("report_month desc").limit(range).pluck(:report_month)
  end
end

Thanks in advance for helping a noob!

Upvotes: 0

Views: 86

Answers (4)

engineersmnky
engineersmnky

Reputation: 29588

You could also specify it as using @Magnuss' code as a base point

class Volunteer < ActiveRecord::Base

  has_many :reports

  STATUSES = {(0..2) => 'active', (3..5) => 'irregular'}

  def status
    STATUSES.find(->{['inactive']}){|range,_| range === number_of_recent_nought_reports}.pop 
  end

  private

   def number_of_recent_nought_reports
     # You could move the where call to scope in your `Report` class.
     @recent_nought_reports ||= reports.nought.last_n_months(6).count
  end
end

class Report < ActiveRecord::Base
  belongs_to :volunteer
  scope :nought, -> {where("hours IS NULL") || where("hours: 0")}
  scope :last_n_months, ->(months){ where(:report_month => Report_range.months(months)) }
end

This allows more flexibility in my opinion for adding more statuses by just redefining STATUSES as you see fit.

What this does is find the first result where the number_of_recent_nought_reports is in a range defined in STATUSES in the event that it falls outside of any of the ranges ['inactive'] is returned then pop pulls out just the status.

I also moved the report_month scope to Report as last_n_months.

Upvotes: 1

Cary Swoveland
Cary Swoveland

Reputation: 110745

You could write:

def status(volunteer)
  case volunteer.reports
                .nought
                .where(:report_month => Report_range.months(6))
                .count
  when (6..Float::INFINITY) then 'inactive'
  when (3..5)               then 'irregular'
  else                           'active'
end

Upvotes: 0

NullRef
NullRef

Reputation: 3743

def status(volunteer)
  case
  when volunteer.reports.nought.where(:report_month => Report_range.months(6)).count > 5
    return 'inactive'
  when volunteer.reports.nought.where(:report_month => Report_range.months(6)).count >= 3
    return 'irregular'
  else
    return 'active'
  end
end

You don't need to pass the volunteer because you are calling the method status on an instance of Volunteer that already has direct access to reports. So the above becomes:

def status
  case
  when reports.nought.where(:report_month => Report_range.months(6)).count > 5
    return 'inactive'
  when reports.nought.where(:report_month => Report_range.months(6)).count >= 3
    return 'irregular'
  else
    return 'active'
  end
end

Additionally, you are running the count query two times which is not optimal. I suggest:

def status
  number_of_reports = reports.nought.where(:report_month => Report_range.months(6)).count
  case
  when number_of_reports > 5
    return 'inactive'
  when  number_of_reports >= 3
    return 'irregular'
  else
    return 'active'
  end
end

Finally the case returns the matching value so you don't need the returns. 

def status
  number_of_reports = reports.nought.where(:report_month => Report_range.months(6)).count
  case
  when number_of_reports > 5
    'inactive'
  when  number_of_reports >= 3
    'irregular'
  else
    'active'
  end
end

There is more but that is a start.

Upvotes: 1

Magnuss
Magnuss

Reputation: 2310

I'd do something like this:

class Volunteer < ActiveRecord::Base

  has_many :reports

  def status
    if number_of_recent_nought_reports > 5
      'inactice'
    elsif number_of_recent_nought_reports >= 3
      'irregular'
    else
      'active'
    end
  end

  private

  def number_of_recent_nought_reports
    # You could move the where call to scope in your `Report` class.
    @recent_nought_reports ||= reports.nought.where(:report_month => Report_range.months(6)).count
  end
end

Upvotes: 1

Related Questions