HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Returns the year for each month by period - nested case statements

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
casetheyeareachperiodstatementsnestedforreturnsmonth

Problem

def return_year_by_month_and_period(month)
pw = self.property_water.first().billing_increment
case pw
  when "12"
    self.property_water.where(period: month).first().year
  when "6"
    case month
      when 1, 2
        self.property_water.where(period: 1).first().year
      when 3, 4
        self.property_water.where(period: 2).first().year
      when 5, 6
        self.property_water.where(period: 3).first().year
      when 7, 8
        self.property_water.where(period: 4).first().year
      when 9, 10
        self.property_water.where(period: 5).first().year
      else
        self.property_water.where(period: 6).first().year
    end
  when "4"
    case month
      when 1, 2, 3
        self.property_water.where(period: 1).first().year
      when 4, 5, 6
        self.property_water.where(period: 2).first().year
      when 7, 8, 9
        self.property_water.where(period: 3).first().year
      else
        self.property_water.where(period: 4).first().year
    end
  when "2"
    case month
      when 1, 2, 3, 4, 5, 6
        self.property_water.where(period: 1).first().year
      else
        self.property_water.where(period: 2).first().year
    end
  else
    self.property_water.first().year
end
end


"Period" in the text above is one of months, bi-monthly, quarterly, semi-annually or annually.

For each of the 12 months in the calendar year, we want to associate a year (e.g. 2015) with the month depending on what year is associated with the period.

First let me say - I think the database modeling for this particular thing is horribly FUBAR. I inherited the project - and re-making the database is on the roadmap, but not the immediate need.

So in the example above, if I had a period of 2 (bi-monthly) - I would only have 6 records in the property_waters table. But I still have 12 months, so I need to say "the 1st period is Jan and Feb" and this logic has to carry. The property_waters table does NOT have a 'month' column, meaning my two attributes are per

Solution

Assuming that billing_increment is always an integer that is a factor of 12, the code should be entirely formulaic.

def return_year_by_month_and_period(month)
  period_len = property_water.first.billing_increment.to_i
  if 12 % period_len != 0
    # TODO: decide what the appropriate behavior should be
    self.property_water.first.year
  else
    nth_period = (month - 1) / period_len + 1
    property_water.where(period: nth_period).first.year
  end
end


I've incorporated additional suggestions from @tokland:

  • In Ruby it's not idiomatic to write self., nor parens on calls without arguments.



  • Write the second case in an else. Full conditional branches are easier to read.

Code Snippets

def return_year_by_month_and_period(month)
  period_len = property_water.first.billing_increment.to_i
  if 12 % period_len != 0
    # TODO: decide what the appropriate behavior should be
    self.property_water.first.year
  else
    nth_period = (month - 1) / period_len + 1
    property_water.where(period: nth_period).first.year
  end
end

Context

StackExchange Code Review Q#77769, answer score: 5

Revisions (0)

No revisions yet.