0
\$\begingroup\$

I was attempting to build a program using Ruby that asks the user to input at least three numbers, and it returns the mean, median, and mode of those numbers.

A developer friend of mine glanced over it and said that the program was wrong, but didn't state specifically what was wrong with it. I have looked over it and tested it repeatedly and can't figure out what's wrong with it.

puts "Please input three or more numbers with spaces inbetween them:"
numbers = gets.chomp
numbers = numbers.split(" ").map(&:to_i) 
length = numbers.length
y = 0.000
numbers.each do |x|
    y = x + y
end
mean = y / length
print "Mean: #{mean}"
print "\n"
a_order = numbers.sort
length1 = (length - 1) / 2
if length%2 == 1
    median = a_order[length1]
else
    length2 = length1 + 1
    median = (a_order[length1] + a_order[length2]) / 2.000
end
print "Median: #{median}"
print "\n"
frequencies = Hash.new(0)
numbers.each do |number|
    frequencies[number] = frequencies[number] + 1
end
frequencies = frequencies.sort_by { |a, b| b }
frequencies.reverse!
frequencies = frequencies.to_a
if frequencies[0][1] == frequencies[1][1]
    print "Mode: invalid"
else
    print "Mode: #{frequencies[0][0]}"
end
\$\endgroup\$
  • \$\begingroup\$ Please note that in this case, the mode is a single mode only. If there are multiple numbers that frequent the same number of times, the mode does not exist. \$\endgroup\$ – Andrew Lim Jul 28 '13 at 11:20
1
\$\begingroup\$

It looks valid to me. Except you are not enforcing the '3 number minimum'.

  • You can use x.odd? to check odd or even.
  • You can total numbers with [1,2,3].inject(&:+).
  • The frequencies.to_a is not needed, as it's already an array after the sort_by.
  • You can sort_by -b so you don't have to reverse.

These are all just refactorings. No change in behaviour.

\$\endgroup\$
1
\$\begingroup\$

The program is mostly correct. I don't see any reason why this problem requires at least three numbers, and in any case you don't bother enforcing it.

It's a wall of text, though, and rather hard to read as a result. You should definitely organize the code into three functions.

Try to avoid defining a lot of intermediate variables, which introduce clutter. For example,

length = numbers.length
y = 0.000
numbers.each do |x|
    y = x + y
end
mean = y / length

… should be a single expression in a function:

def mean(numbers)
  numbers.inject(:+).to_f / numbers.length
end

Similarly,

a_order = numbers.sort
length1 = (length - 1) / 2
if length%2 == 1
    median = a_order[length1]
else
    length2 = length1 + 1
    median = (a_order[length1] + a_order[length2]) / 2.000
end

… should be packed as:

def median(numbers)
  sorted = numbers.sort
  if numbers.length % 2
    sorted[numbers.length / 2]
  else
    0.5 * (sorted[numbers.length / 2 - 1] + sorted[numbers.length / 2])
  end
end

In the mode calculation, the counting operation…

frequencies = Hash.new(0)
numbers.each do |number|
    frequencies[number] = frequencies[number] + 1
end
frequencies = frequencies.sort_by { |a, b| b }
frequencies.reverse!
frequencies = frequencies.to_a

… could be done fluently as a single expression.

You have a bug here: if all of the numbers are the same, then frequencies[1][1] will crash.

if frequencies[0][1] == frequencies[1][1]
    print "Mode: invalid"
else
    print "Mode: #{frequencies[0][0]}"
end
def mode(numbers)
  frequencies = numbers.inject(Hash.new(0)) { |h, n| h[n] += 1; h }
                       .sort_by { |n, count| -count }
                       .to_a
  if frequencies.length == 1 || (frequencies[0][1] != frequencies[1][1])
    frequencies[0][0]
  else
    nil
  end
end

Once those functions have been defined, the "main" program can be very tidy:

puts "Please input some numbers with spaces in between them:"
numbers = gets.split.map(&:to_i)
puts "Mean: #{mean(numbers)}"
puts "Median: #{median(numbers)}"
puts "Mode: #{mode(numbers) || "invalid"}"

Note that there is no need to chomp if you're just going to split on whitespace anyway.

Printing a string with a newline at the end is better done using puts.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.