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?

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.