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

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 period and year. From the period, I can determine which months belong in which group, and return the year of the period.

The code works - but even after I wrote it I knew I could do better. I just can't see it, so please help me learn a better way to do this.

EDIT: Billing increment for every record is always the same integer as the period. So if we have 6 records for billing period of bi-monthly, all 6 records have billing_increment = 6.

Ridiculous I know. We can just count the records to return this same thing...

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:

1. In Ruby it's not idiomatic to write self., nor parens on calls without arguments.
2. Write the second case in an else. Full conditional branches are easier to read.
• Thanks for the reply, month is not a factor of 12. Month is an integer 1-12 representing Jan-Dec. billing period would be a factor of 12: 1, 2, 3, 4, 6, 12 are possible values. The count of records in the table match the billing period. If we get 6 bills per year, we have 6 records in our table. – notaceo Jan 18 '15 at 23:50
• Oops, I misspoke. – 200_success Jan 18 '15 at 23:51
• Wrote a lot more but my phone hung up and it was all erased. I apologize. Thanks for the suggestion. – notaceo Jan 19 '15 at 0:02

It seems to me you can eliminate the inner case statements using simple math to calculate the right period:

case pw
when "12"
self.property_water.where(period: month).first().year
when "6"
self.property_water.where(period: (month - 1) / 2 + 1).first().year
when "4"
self.property_water.where(period: (month - 1) / 3 + 1).first().year
when "2"
self.property_water.where(period: (month - 1) / 6 + 1).first().year
else
self.property_water.first().year
end


You may wish to consider writing this as follows:

First obtain the number of months per billing period:

months_per_billing_period = property_water.first().billing_increment


and then for any month (1-12), compute:

return_year_by_month_and_period(months_per_billing_period, month)


where:

def return_year_by_month_and_period(months_per_billing_period, month)
property_water.where(month_to_period(months_per_billing_period,
month)).first().year
end

def month_to_period(months_per_billing_period, month)
(month.to_f / months_per_billing_period).ceil.to_i
end


A few points:

• I don't believe you need self. anywhere.
• For 12 months per billing period, don't you want property_water.where(period: 1).first().year, as there is only one period per year in that case?
• The method return_year_by_month_and_period should have a more descriptive name.