patternrubyrailsMinor
Method for gathering the correct data to a specific user
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
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.
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
endAs 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.
-
-
All those methods you are calling on a
-
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).
-
-
There are some ways to build arrays with conditional items. I like this functional pattern:
I'd write:
-
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
endCode 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
endContext
StackExchange Code Review Q#112716, answer score: 2
Revisions (0)
No revisions yet.