Setting responses on survey results

Question

The below code feels ugly, how can I make it better? (The part i don't like is the ||=. I also don't know if I like the resulting data structure. (An array of hashes)

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

Answer 1

Regarding the data structure: In cases like this where you always use the same keys for the hashes (i.e. :answers and :question in this case), it is often a good idea to use a simple class instead, which has accessors for the given properties. You can use Struct to easily create such classes. In this case this would look like this:

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

Using a class instead of a hash is more robust as mistyped attribute names will cause an exception. It is also more extensible as it allows you to add methods to the class - like for example most_common_answer.

However in this particular case where the method is called survey_results and there exists a class called SurveyResult one has to wonder whether survey_results couldn't/shouldn't return instances of the existing SurveyResult class instead.

My guess is you should be returning an array of SurveyResult objects and extend that class to offer the functionality you need. Though without knowing more of your code it's hard to say for sure.

At the very least you should rename the method or the class to avoid the impression that the method returns instances of that class.

---

Now to your code:

First of all your method names sounds like it would return the collected survey results, but in fact it just stores them in an instance variable and apparently returns nothing of consequence. You should either make it return the results (possible caching them in the instance variable) or change its name to something imperative-sounding like initialize_survey_results instead.

---

You're creating an empty array and then appending to it once per iteration in an each-loop. If you do this you can (and should) almost always use map instead.

---

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

As you already pointed out this is a bit ugly. Luckily ruby's Hash class has a feature that gets rid of the need for ||= here: It allows you to set default values which will be returned instead of nil if a key was not found. If we replaces answers = {} with answers = Hash.new(0) to set the default value to 0, you can just get rid of the ||= line.

---

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

This line stumped be a bit when I first read it. It is not apparent why you're not just calling StepSurvey.find(step_survey_id). First I thought maybe you don't want to get the step survey when there is no survey result pointing to it, but in that case the above code would cause an exception, so that can't be it.

---

If I were to write this, I would do something like this:

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

Answer 2

Using:

• Facets's frequency.
• Functional programming principles (no Enumerable#each and Array#push and other bad companies).

You can write:

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