"Newbie" Feedback

I recently had the privilege to supply the challenge for the second ever Ruby Programming Challenge For Newbies. I thought it pretty cool that the challenge provoked 40+ "newbies" to submit responses. As one might expect, there was some rough Ruby, but very few butcherings of the language.

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]

# 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]

The problem requested a solution with a method signature of 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 = 0
i = 0
while i < times.length
  time = times[i]
  # parse / manipulate the time
  sum = sum + time
  i = i + 1
end

It is nice that Ruby provides syntax like for and while, making the transition from other languages so easy. There is a cleaner way to accomplish this in Ruby:

sum = 0
times.each do |time|
  # parse / manipulate the time
  parsed_time = #....
  sum = sum + parsed_time
end

Eschewing 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|
  # Manipulate the time somehow
  accumulator + time
end

An 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)
  sorted_times = times.
    map { |t| Time.parse(t) }.
    sort
  # Do more stuff ...
end

The 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.
  map { |str| Time.parse(str) }.
  inject { |sum, t| sum + t }

It is possible to achieve even greater clarity through symbol to proc shorthand:

class String
  def to_time
    Time.parse(self)
  end
end

def average_time_of_day(times)
  sum = times.map(&:to_time).inject(&:+)
  # Calculate the average ...
end

The 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.

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)
# 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

If you had saved this file as 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.rb
Loaded suite average_time_of_day
Started
.

1 test, 1 assertion, 0 failures, 0 errors

Ruthlessly running this command as the 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.

Conclusion

That about sums up my feedback to the "newbies". I really did enjoy providing the challenge and very much appreciated so many quality solutions. I hope most of you continue your learning and I look forward to seeing some of your names driving the future of Ruby!