3
\$\begingroup\$
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...

\$\endgroup\$
5
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – notaceo Jan 18 '15 at 23:50
  • \$\begingroup\$ Oops, I misspoke. \$\endgroup\$ – 200_success Jan 18 '15 at 23:51
  • \$\begingroup\$ Wrote a lot more but my phone hung up and it was all erased. I apologize. Thanks for the suggestion. \$\endgroup\$ – notaceo Jan 19 '15 at 0:02
3
\$\begingroup\$

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
\$\endgroup\$
1
\$\begingroup\$

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.
\$\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.