In the spirit of the code review, I tried to provide constructive feedback to all participants. Following is a summary of some common suggestions that I had to offer...
Bang Methods
Method parameters are mutable in Ruby. If you modify the value of the parameter inside a method, the original value changes. This can lead to all sorts of unexpected side-effects. There are some exceptions to this, but it is best to take no chances and treat it as a rule.This can be a bit frightening at first, but Ruby has a convention of adding a bang (!) to the end of a method name when side-effects are expected. Many of the core methods follow this convention. For instance consider the difference between
map and map!:a = [1, 2, 3]The problem requested a solution with a method signature of
# Double every value in the array
>> a.map{|x| x * 2}
=> [2, 4, 6]
# The original value has not changed
>> a
=> [1, 2, 3]
# Double every value in the array with a bang method
>> a.map!{|x| x * 2}
=> [2, 4, 6]
# The original value is changed
>> a
=> [2, 4, 6]
average_time_of_day(times), where the times parameter was list of strings (e.g. %w{6:41am 6:51am 7:01am}). Since the method does not end with a bang, it should not modify the input (e.g. by calling a bang method like map!). A third of the submissions modified the times array inside that method—or worse, in other methods that were called by average_time_of_day.Since
average_time_of_day was the entirety of the challenge, modifying the input was not a huge problem. Still, not a good habit to start. Enumerable
Since the challenge involved averaging times, most solutions involved summing up these times. A typical solution looked something like this:sum = 0It is nice that Ruby provides syntax like
i = 0
while i < times.length
time = times[i]
# parse / manipulate the time
sum = sum + time
i = i + 1
end
for and while, making the transition from other languages so easy. There is a cleaner way to accomplish this in Ruby:sum = 0Eschewing
times.each do |time|
# parse / manipulate the time
parsed_time = #....
sum = sum + parsed_time
end
while in favor of an each block eliminates the need for the tracking variable, i. With the assignment and computation of the tracking variable eliminated, there is less to distract from the actual intent of the code. Less code makes intent clearer.The only thing still getting in the way of intent is the accumulator variable,
sum. It is being initialized outside of the each block and manipulated inside the block. If you think of a block like a function, then this is akin to modifying a global variable inside a function.Ruby has an
inject method (also known as reduce and fold in Ruby 1.8.7+) to handle this:sum = times.inject(0) do |accumulator, time|An
# Manipulate the time somehow
accumulator + time
end
inject starts with a seed value for an accumulator (sometimes called a memo). As inject iterates over each value, the last value in the block is used to seed the accumulator the next iteration. The end result of the iteration is the accumulation of each of these values. In the case of the code snippet above, it is the summation of the times.But wait, there's more! It is possible to compact this even further...
Chaining
If you are not using bang methods, then you can chain iterators together. Let's say your solution involved mapping time strings to time objects and then sorting them, this can be accomplished via:def average_time_of_day(times)The
sorted_times = times.
map { |t| Time.parse(t) }.
sort
# Do more stuff ...
end
sorted_times local variable now contains, well... sorted times. The time strings were mapped to actual time objects, which could then be sorted. That's just lovely!Going back to the accumulator example, it is possible to chain together iterator to make intent crystal clear:
sum = times.It is possible to achieve even greater clarity through symbol to proc shorthand:
map { |str| Time.parse(str) }.
inject { |sum, t| sum + t }
class StringThe theory behind symbol-to-proc is probably a bit beyond "newbie" scope, but still worth seeing to give an idea how close to the domain Ruby allows you to get. In that last code snippet, I am taking the times strings, mapping them to Time instances (with a monkey patch to String) and adding them all together. Intent does not get much clearer than that.
def to_time
Time.parse(self)
end
end
def average_time_of_day(times)
sum = times.map(&:to_time).inject(&:+)
# Calculate the average ...
end
Read the documentation on Ruby's Enumerable module. Even seasoned Rubyists can benefit from re-reading it regularly.
Test Harness
Some will claim that the Ruby community is too "test obsessed". There are very good reasons to embrace TATFT / BDD, etc. I could write for months on why this is a good idea. In fact I did. But this is not one of those times.Using a test harness for an algorithm like
average_time_of_day makes too much sense not to use it. Many of the submissions were clearly iterating toward a solution. The author got it working for the simple case, then tried to get it working crossing midnight—only to break the simple case. Many authors never noticed that the simple case was no longer working and submitted half working solutions.As soon as you have a simple solution working, write a test (if not sooner!):
def average_time_of_day(times)If you had saved this file as
# incredibly simplistic solution
end
require 'test/unit'
class TestAverageTimeOfDay < Test::Unit::TestCase
def test_simple_times
assert_equal('6:51am', average_time_of_day(%w{6:41am 6:51am 7:01am}))
end
end
average_time_of_day.rb, you could execute this test after each change to the average_time_of_day method thusly:jaynestown% ruby average_time_of_day.rbRuthlessly running this command as the
Loaded suite average_time_of_day
Started
.
1 test, 1 assertion, 0 failures, 0 errors
average_time_of_day method evolves will tell you immediately when something is broken. The sooner you know something is wrong, the sooner you can address it.Don't test because a segment of the Ruby community forces you to, do it because it is so damn easy there is no reason not do.
