4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The output is an array of arrays? don't you need some flattenning? \$\endgroup\$ – tokland Dec 4 '15 at 21:42
  • \$\begingroup\$ @tokland what I have now seem to work fine, what do you suggest? \$\endgroup\$ – Sebastian Jennings Almnes Dec 6 '15 at 11:07
  • \$\begingroup\$ 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) \$\endgroup\$ – tokland Dec 6 '15 at 11:18
2
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – Sebastian Jennings Almnes Dec 3 '15 at 12:29
  • \$\begingroup\$ 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. \$\endgroup\$ – Sebastian Jennings Almnes Dec 6 '15 at 11:13
  • \$\begingroup\$ 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). \$\endgroup\$ – tokland Dec 6 '15 at 11:22
0
\$\begingroup\$
Time.zone.now.strftime('%V').to_i

Use Date#cweek to get the calendar week number:

Time.zone.now.to_date.cweek
\$\endgroup\$
  • \$\begingroup\$ This doesn't actually work. Because it belongs to the Date class \$\endgroup\$ – Sebastian Jennings Almnes Dec 4 '15 at 13:32
  • \$\begingroup\$ Sorry, I hadn't seen this when I added the cweek to my answer. Add a to_date there and it will work. \$\endgroup\$ – tokland Dec 4 '15 at 16:46
  • \$\begingroup\$ @SebastianJenningsAlmnes: Fixed. Added a call to to_date as @tokland pointed out. \$\endgroup\$ – Spike Dec 5 '15 at 19:53

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.