Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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 reducereduce is idiomatic Ruby:

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

See this SO questionthis SO question for more.

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:

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

See this SO question for more.

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:

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

See this SO question for more.

Source Link
Wayne Conrad
  • 3.3k
  • 1
  • 17
  • 38

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.