refactor helper for default state for rails select with enums

Question

I've created a helper method that is essentially used to get the default state of an select:

<%= select_tag "question_main_frequency",
 options_for_frequency_state(frequency_segment(question)), { class: 'form-control' } %>

The helper method I've created to get the default state looks like this:

 def frequency_segment(question)
 return false if question.id.nil?
 question = Question.find(question.id)
 if question.weekly?
 'weekly'
 elsif question.odd_weeks? || question.even_weeks?
 'Biweekly'
 elsif question.start_of_month? || question.end_of_month?
 'Monthly'
 elsif question.start_of_quarter? || question.end_of_quarter?
 'Quarterly'
 end
 end

The reason why I need this is because that form populates a new select form with the methods you see in the helper. So if they select 'biweekly' then question.odd_weeks and question.even_weeksappear as two options.

and you don't have to be a genius to see that this isn't exactly beautiful. It's also causing:

Perceived complexity for frequency_segment is too high. [10/7]
Method has too many lines. [11/10]
Cyclomatic complexity for frequency_segment is too high.[9/6]

on rubocop. The thing is I'm not entirely sure how to refactor this. The Question methods are enum, set up like this:

class Question < ActiveRecord::Base
 enum frequency: { weekly: 0, odd_weeks: 1, even_weeks: 2,
 start_of_month: 3, end_of_month: 4,
 start_of_quarter: 5, end_of_quarter: 6 }

any ideas on how I can refactor this?

• BCdotWEB: As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles.
• Sebastian Jennings Almnes: @BCdotWEB thanks for good pointers! I've gone ahead and updated my answer, hopefully this is more useful :)

Answer 1

Don't know if there's a good way to do this, since your translation isn't 1:1. E.g. your enum differentiates between odd weeks and even weeks, but your helper doesn't.

So you have to do some sort of work to collapse 7 states into 4 options.

If you're already using I18n to do translation, you might be able to put the strings in there, e.g.:

question:
 frequency:
 - weekly
 - biweekly
 - biweekly
 - monthly
 - monthly
 - quarterly
 - quarterly

and the use the standard Rails I18n helpers to fetch the right string. But that's not too pretty either, and really only hides the problem from Rubocop. It's also a more implicit connection, and more error-prone, so, yeah, you probably shouldn't go down this road.

Frankly, I wouldn't bother too much about Rubocop's opinion about method length or cyclomatic complexity in this case. But...

But there's weird stuff going on too:

• You're passing in a question and checking its id. But it's not a real Question record? Because you immediately go and fetch one from the database. So what are you passing in that's called question, has an id, but isn't actually a Question?

• You're also overwriting the argument variable with the record you fetch, which is not a great idea. Now you have a question that's apparently not really a Question, but then maybe it becomes a question that is actually a Question. Confused? Me too.

• Oh, and are you sure you're going to find something when you call find(id)? If not, you'll get a 404 RecordNotFound error. It might be completely intentional to let that happen, but to have the page break due to a helper method is perhaps a little icky.

• Why return false? If anything, you should return nil. Ideally, you return a string, but the "opposite" of a string isn't false - at best it's nil. And actually, your method could conceivably return nil already, since you don't have an else branch in your logic. If, for whatever reason, none of the if/elsif conditions are true, your method will return nil.

Anyway, without worrying about the question vs question thing and possible exceptions being thrown, I'd suggest at least using a case statement instead of a lot of elsifs:

case
when question.weekly?
 'Weekly'
when question.odd_weeks?, question.even_weeks?
 'Biweekly'
when question.start_of_month?, question.end_of_month?
 'Monthly'
when question.start_of_quarter?, question.end_of_quarter?
 'Quarterly'
else
 nil
end

Oh, and I capitalized "Weekly" to match the other strings.

• Sebastian Jennings Almnes: Hi, thanks for the good reply, the question I get is in-fact a real Question record. I see now that I am over-writing it. On your note about recordnotfound I felt that I avoided that by returning false if it's not present. Is that not optimal, if so how would you do it taking this update into account?
• Flambino: @SebastianJenningsAlmnes Well, if the point is that you want to reload the record to make sure it's up to date, then, well, there's the reload method. As for the id check, I'd actually forgotten it was there. It should protect you... however: The question may have been deleted from the database since you got that ID. It's unlikely, but if those question records are manipulated elsewhere it might happen. If you're sure they're there, then ok, but you are doing a check leading me to think you're not sure about that.
• Spike: You can test question.frequency in the case statement - it returns the string representation of the value (e.g. "odd_weeks")
• Flambino: @Spike True, but I'd rather have Ruby raise a NoMethodError if I've misspelled, .odd_weks? (sic), than silently hit the else case because I've misspelled the same thing in a string.