Настройка реакции на результаты опроса


Приведенный ниже код чувствует себя уродливой, как я могу сделать это лучше? (Мне не нравится это ||=. Я также не знаю, если мне нравится полученная структура данных. (Массив хэшей)

def survey_results
    @survey_results = []
    unless params[:step_survey_id].blank?
      SurveyResult.find_by_step_survey_id(params[:step_survey_id]).step_survey.step_survey_questions.each do |survey_question|
          survey_result = {:question => survey_question.value}
          answers = {}        
          survey_question.survey_result_answers.each do |survey_result_answer|
            #Setup initial answer hash with key as the response, only used when needed
            answers[survey_result_answer.response] ||= 0
            answers[survey_result_answer.response] += 1
        end
        survey_result[:answers] = answers
        @survey_results.push(survey_result)
      end   
    end    
  end


312
2
задан 17 марта 2011 в 08:03 Источник Поделиться
Комментарии
2 ответа

Что касается структуры данных: в случаях, подобных этому, где вы всегда используете одни и те же ключи хешей (т. е. :ответы и :вопрос в данном случае), это часто хорошая идея, чтобы использовать простой класс, что есть аксессоры для данного свойства. Вы можете использовать структуру , чтобы легко создавать такие классы. В этом случае это будет выглядеть так:

SurveyResultInformation = Struct.new(:question, :answers)

С помощью класса, а не хэш является более надежной, так как опечатки имена атрибутов будет вызывать исключение. Он также является более расширяемым, поскольку он позволяет добавлять методы к классу, как например most_common_answer.

Однако в данном конкретном случае, когда метод вызывается survey_results и существует класс под названием SurveyResult остается предполагать, что survey_results не мог/не возвращают экземпляры существующих SurveyResult класса вместо.

Я думаю, что вы должны быть возвращая массив SurveyResult объекты и расширить этот класс, чтобы предложить функции, которые вам нужны. Хотя не зная вашего кода трудно сказать наверняка.

По крайней мере, вы должны переименовать метод или класс, чтобы избежать впечатления, что метод возвращает экземпляры этого класса.


Теперь ваш код:

Прежде всего, ваши имена способа звучит, как он будет возвращать собранные результаты опроса, но на самом деле он просто сохраняет их в переменной экземпляра и, видимо, ничего не возвращает значение. Вы должны либо сделать его возвращают результаты (возможно кэширование их в переменной экземпляра) или изменить свое название на что-то повелительно звучащий как initialize_survey_results вместо.


Вы создаете пустой массив, а затем добавляя к ней один раз за итерацию в каждую петлю. Если вы сделаете это, вы можете (и должны) почти всегда использовать карту вместо этого.


answers[survey_result_answer.response] ||= 0
answers[survey_result_answer.response] += 1

Как вы уже отметили, это немного некрасиво. К счастью, Руби хэш класс имеет функцию, которая избавляется от необходимости для ||= Вот: она позволяет установить значения по умолчанию, которое будет возвращено вместо нуля , если ключ не был найден. Если мы заменяет ответы = {} с ответов = хэш.новые(0) чтобы установить значение по умолчанию значение 0, вы можете просто избавиться от ||= строке.


SurveyResult.find_by_step_survey_id(params[:step_survey_id]).step_survey

Это линия в тупик немного, когда я впервые прочитал его. Не вполне понятно, почему вы не просто называете StepSurvey.найти(step_survey_id). Сначала я подумал, что вы не хотите сделать шаг исследования, когда нет результата опроса указывали на это, но в этом случае приведенный выше код вызовет исключение, так что не может быть.


Если я пишу это, я хотел бы сделать что-то вроде этого:

def survey_results
if params[:step_survey_id].blank?
[]
else
StepSurvey.find(params[:step_survey_id]).step_survey_questions.map do |survey_question|
survey_result = SurveyResultInformation.new(survey_question.value)
answers = Hash.new(0)
survey_question.survey_result_answers.each do |survey_result_answer|
answers[survey_result_answer.response] += 1
end
survey_result.answer = answers
survey_result
end
end
end

4
ответ дан 17 марта 2011 в 09:03 Источник Поделиться

Использование:

Вы можете написать:

def survey_results
result = SurveyResult.find_by_step_survey_id(params[:step_survey_id])
questions = result ? result.step_survey.step_survey_questions : []
questions.map do |survey_question|
{
:question => survey_question.value,
:answers => survey_question.survey_result_answers.map(&:response).frequency
}
end
end

2
ответ дан 30 марта 2011 в 02:03 Источник Поделиться