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

Method for gathering the correct data to a specific user

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

Problem

I have quite a lengthy method that I know for sure can be optimized, yet I am not sure how to do it. The method is in my User model, and essentially gets the questions assigned to the user in the current week. Questions can be assigned to a variety of intervals, so it's important that for each week we get the right ones.

def collect_right_questions_for_this_week
    current_week = Time.zone.now.strftime('%V').to_i

    odd_or_even_week = current_week.odd? ? :odd_weeks : :even_weeks

    beginning_week_of_month =
      Time.zone.now.beginning_of_month.strftime('%V').to_i
    end_week_of_month       =
      Time.zone.now.end_of_month.strftime('%V').to_i

    beginning_week_of_quarter =
      Time.zone.now.beginning_of_quarter.strftime('%V').to_i
    end_week_of_quarter       =
      Time.zone.now.end_of_quarter.strftime('%V').to_i

    frequency_values = [questions.frequencies[:weekly]]
    frequency_values << questions.frequencies[odd_or_even_week]

    frequency_values << questions.frequencies[:start_of_month] if
      current_week == beginning_week_of_month

    frequency_values << questions.frequencies[:end_of_month] if
      current_week == end_week_of_month

    frequency_values << questions.frequencies[:start_of_quarter] if
      current_week == beginning_week_of_quarter

    frequency_values << questions.frequencies[:end_of_quarter] if
      current_week == end_week_of_quarter
    frequency_values
  end


As you can see here, it's quite lengthy and repetitive. Essentially what I'd like to achieve is for it to be reduced, and better written, it doesn't have this nice flow to it.

I also recall that a method should only do 1 thing, while at least in my opinion this method does two things. It sets dates, and then gets questions.

Solution

Some notes:

-
Use local variables for values that are used many times.

-
time = Time.zone.now: This local variable prevents race conditions from occurring if the day changed in the middle of the method (unlikely, but why risk it?).

-
All those methods you are calling on a Time object are also available on Date. This will simplify your code.

-
You can send the time/date as (an optional) argument so the method works for any moment of the year (also, it will be easier to test).

-
time.strftime("%V").to_i is ok, but there is a better way: Date#cweek.

-
There are some ways to build arrays with conditional items. I like this functional pattern: values = [value1, (value2 if condition), value3].compact.

I'd write:

def questions_for_week(date: Time.zone.today)
  frequencies = questions.frequencies
  week = date.cweek
  [
    frequencies[:weekly],
    frequencies[week.odd? ? :odd_weeks : :even_weeks],
    (frequencies[:start_of_month] if week == date.beginning_of_month.cweek),
    (frequencies[:end_of_month] if week == date.end_of_month.cweek),
    (frequencies[:start_of_quarter] if week == date.beginning_of_quarter.cweek),
    (frequencies[:end_of_quarter] if week == date.end_of_quarter.cweek),
  ].compact
end

Code Snippets

def questions_for_week(date: Time.zone.today)
  frequencies = questions.frequencies
  week = date.cweek
  [
    frequencies[:weekly],
    frequencies[week.odd? ? :odd_weeks : :even_weeks],
    (frequencies[:start_of_month] if week == date.beginning_of_month.cweek),
    (frequencies[:end_of_month] if week == date.end_of_month.cweek),
    (frequencies[:start_of_quarter] if week == date.beginning_of_quarter.cweek),
    (frequencies[:end_of_quarter] if week == date.end_of_quarter.cweek),
  ].compact
end

Context

StackExchange Code Review Q#112716, answer score: 2

Revisions (0)

No revisions yet.