CODEWITHSUNDEEP
ruby
whatoncewaslost
whatoncewaslost

Reputation: 2256

Finding the mode of an array

So I'm working through the Mean Median and Mode chapter of the Ruby Cookbook, and I'm trying to figure out why the code is doing what it is doing. I get everything else, but what's with adding to the variable modes and then (from the looks of it) reassigning it? I know what all the operators in the code blocks mean but I can't grasp the logic behind it. 

def modes(array, find_all  = true)
  histogram = array.inject(Hash.new(0)) {|h,n| h[n] += 1; h  }
  modes = nil 

  histogram.each_pair do |item, times| 
    modes << item if modes && times == modes[0] and find_all
    modes = [times, item] if (!modes && times >1) or (modes && times > modes[0])
  end

  return modes ? modes[1...modes.size] : modes
end

Upvotes: 1

Views: 362

Answers (1)

Zach Kemp
Zach Kemp

Reputation: 11904

First of all, do not look at this code as something to emulate. It has so many problems and is so brittle that I frankly cannot believe it was published.

Here are the major problems:

  1. modes is the name of the method and the name of a temporary variable inside the method
  2. The internal modes is sometimes nil and sometimes an array, and the behavior of each line in the each_pair block changes depending on which is which.
  3. The first value of the modes array is used to store another temporary variable
  4. The rest of the values of the modes array is used as a return value.
  5. The final return statement checks again is modes is nil, returns the tail if not, and returns nil if so. Returning nil from a method is, as a rule, not a good idea. It's liable to break your other code in so many other places and will make you have to perform all of these nil checks everywhere— in short, it will make the rest of the program look like this wretched method.
  6. It uses the control flow operators and and or in place of the boolean operators && and || for boolean conditionals

As for your question, I believe the confusion arises from these four lines:

histogram.each_pair do |item, times| 
  modes << item if modes && times == modes[0] and find_all
  modes = [times, item] if (!modes && times >1) or (modes && times > modes[0])
end

This loops through each pair of the histogram hash as follows:

  1. Does modes exist? And does times equal the first element? And should I find_all?
    • if so, tack the current item onto the end of the array.
    • otherwise, do nothing
  2. Is one of the following true?
    • modes is nil and the item was found more than once
    • modes is NOT nil and the current value of times is larger than its first element
    • if so, replace the current value of modes with [times, item].
    • otherwise, do nothing.

Upvotes: 2

Related Questions