Method for gathering the correct data to a specific user

Question

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.

• tokland: The output is an array of arrays? don't you need some flattenning?
• Sebastian Jennings Almnes: @tokland what I have now seem to work fine, what do you suggest?
• tokland: Usually you'd like the method to return an array of questions, not an array of array of questions. For that, just flatten it one level: .flatten(1)

Answer 1

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

• Sebastian Jennings Almnes: Beautiful! I've been thinking about the issue in clause 2 after I figured it could actually happen. Especially with a project that relies on use of time zones etc. Thanks
• Sebastian Jennings Almnes: What if I'd like to make the Time.zone.today, dependent on the users time zone. Currently I do track their time zone in user.time_zone. It does allow blank, and if so should be UTC I guess.
• tokland: Then: date: Date.today.in_time_zone(user.time_zone || "UTC"). You may abstract that snippet so you can write date: today_in_user_zone).

Answer 2

Time.zone.now.strftime('%V').to_i

Use Date#cweek to get the calendar week number:

Time.zone.now.to_date.cweek

• Sebastian Jennings Almnes: This doesn't actually work. Because it belongs to the Date class
• tokland: Sorry, I hadn't seen this when I added the cweek to my answer. Add a to_date there and it will work.
• Spike: @SebastianJenningsAlmnes: Fixed. Added a call to to_date as @tokland pointed out.