Taking data from a survey

Question

I wrote this Ruby code that takes data from a survey. Seeing as it is my first Ruby project, I know it can be written much better. How could some of this code be written in more idiomatic Ruby? This code is extracted from a Rails model (the rest of the model is tame).

def prepare_anova_data

  n = surveys_completed

  (1..n).each do |i|
    anova_summary = create_anova_summary!(no_clusters: i)

    Axis.all.each do |axis|
      clusters_for_axis = cluster(axis.id, i)

      clusters_for_axis.each_with_index do |cluster_axis_summary, index|

        cluster_name = cluster_axis_summary[:name]
        scores = cluster_axis_summary[:data]

        scores.each do |key, value|
          anova_summary.anova_datas.create!(no_clusters: i, cluster_no: index, question_code: key, mean_score: value)
        end
      end
    end
  end
end

def cluster(axis_id, no_clusters)

  # Get all the questions for the axis specified
  @questions ||= self.survey.questions.where(axis_id: axis_id).pluck('id')

  clusterer =diana_clusterer(@questions, no_clusters)

  # Calculate the mean scores per cluster, eg. Cluster 1: question 1: 2, question 2, 3.5 etc.
  series_data = []
  clusterer.clusters.each_with_index do |cluster, index|

    question_means_for_cluster = {}

    # For every question, go through the current cluster and add up all of the scores to
    # caluculate the cluster mean
    for i in [email protected]
      sum = 0
      count = 0

      # data_items is an array containing each respondent's survey scores (as an array)
      cluster.data_items.each do |item|
        sum += item[i]
        count += 1
      end

      question_means_for_cluster[Question.survey_code(@questions[i])] = (sum.to_d / count).round(2)
    end

    # Chartkick requires a hash in this format
    series_data << {name: "Cluster #{ index + 1 }", data: question_means_for_cluster}
  end

  # Don't forget to add the overall mean scores to the result.
  series_data << overall_mean_values(clusterer, @questions)
end

def overall_mean_values(clusterer, questions)

  overall_question_means = {}

  # For each question, go through every item (array of individual survey scores) in every cluster and add up the scores
  # This gets the Survey Group average for each question in the survey
  for i in 0...questions.count
    sum = 0
    count = 0

    # Unpack the clusters here, we don't need the clusters, we just want to use the already retrieved data for
    # this survey group and merely sum it all up by question.
    clusterer.clusters.each do |cluster|
      cluster.data_items.each do |item|
        sum += item[i]
        count += 1
      end
    end

    overall_question_means[Question.survey_code(questions[i])] = sum.to_d / count
  end

  # This format is required for the Chartkick libary
  { name: 'Overall', data: overall_question_means }
end

private

def diana_clusterer(questions, no_clusters)

  # Get all the responses for the survey group into an array (of arrays)
  answers = []
  self.members.each do |member|
    answers << member.responses.where(question_id: questions).pluck('score')
  end

  # Use the AI4R library to create a cluster using the Diana Algorithm
  data_set = DataSet.new(data_items: answers, data_labels: questions)
  Diana.new.build(data_set, no_clusters)
end

Answer 1

Thanks for posting your code. You've done a good job with:

• formatting
• variable and method names
• relatively small, focused methods

You may be new to Ruby, but I don't think you're new to programming.

There's not much to be improved here. A few Ruby idioms you missed, and a few matters of style that aren't all that important.

Matters of Style

Line width

Some of the lines are a bit long. While there is no longer universal agreement that lines of code should be kept to less than 80 characters, there are many benefits to doing so. One of them is that, when you paste code into StackOverflow, the reader won't have to bother with horizontal scroll bars. Consider breaking long lines up. Usually, you can just hit enter where you want it, indent, and the code will still work fine. Sometimes, you'll need to add a \ (continuation) to the end of a line to let Ruby know that the rest of the statement is on the following line.

For example, this:

series_data << {name: "Cluster #{ index + 1 }", data: question_means_for_cluster}

could be formatted like this:

series_data << {
  name: "Cluster #{ index + 1 }",
  data: question_means_for_cluster
}

Vertical white space

There's more vertical white space than I'd use. I'd get rid of most, if not all of it. For example, instead of this:

  clusters_for_axis = cluster(axis.id, i)

  clusters_for_axis.each_with_index do |cluster_axis_summary, index|

    cluster_name = cluster_axis_summary[:name]

this:

  clusters_for_axis = cluster(axis.id, i)
  clusters_for_axis.each_with_index do |cluster_axis_summary, index|
    cluster_name = cluster_axis_summary[:name]

In general, I don't consider vertical white space within a method bad, in itself, but when it is used to separate lines into groups that have some similar purpose, I wonder if those lines could be moved into separate methods.

Code comments

Similarly, comments that say what the code is doing, while not bad, are better if they can be replaced with code that says what the comment used to say. Often the lines after a comment can be moved into a method with the method name serving the purpose the comment used to serve.

Ruby idioms

Let's let Ruby do some of the busywork for us.

Replacing a loop with reduce

In this code:

  sum = 0
  count = 0

  # data_items is an array containing each respondent's survey scores (as an array)
  cluster.data_items.each do |item|
    sum += item[i]
    count += 1
  end

We are calculating a sum and a count. The count doesn't need to be calculated: just use cluster.data_items.size. To calculate the sum, this code using reduce is idiomatic Ruby:

  sum = cluster.data_items.reduce(:+)

Map

Ruby has a wonderful idiom for transforming one sequence to another, map. Whenever you see code like this:

  series_data = []
  clusterer.clusters.each_with_index do |cluster, index|
    ...
    series_data << {
      name: "Cluster #{ index + 1 }",
      data: question_means_for_cluster
    }
  end

You can do this instead:

  series_data = clusterer.clusters.map.with_index do |cluster, index|
    series_data << {
      name: "Cluster #{ index + 1 }",
      data: question_means_for_cluster
    }
  end

Ruby frees you from the bother of creating the array and adding elements to it. map can also be used in #diana_clusterer.

self

There are a few uses of self that you don't need. For example, this:

self.members.each do |member|

could be:

members.each do |member|

See this SO question for more.