Extreme Enthusiasm

We have some files containing sequences of integers, one number per line. We want to compute the sum of the numbers. The catch is that the numbers are written as roman numerals.

This code works, but there is a problem: every time we run the tests, a nasty error message appears on standard error. We’d like to be able to turn off the error message when we run the tests. The error message should still appear when the code is run in production.

Your job: make it so that the error message does not appear when the tests are run.

See here the starting code. Copy it in a file pesky.rb and run it with the commmand ruby pesky.rb.

class RomanNumeral
  def initialize(string)
    @value = string
  end

  def to_i
    case @value
    when "I"; 1
    when "II"; 2
    when "III"; 3
    when "IV"; 4
    else
      STDERR.puts("Invalid numeral '#{@value}'")
      0
    end
  end
end

class RomanSequence
  def initialize(input)
    @input = input
  end

  def sum
    result = 0
    @input.lines.each do |line|
      roman = RomanNumeral.new(line.chomp)
      result += roman.to_i
    end
    return result
  end  
end

require "minitest/autorun"
require "stringio"

class RomanSequenceTest < MiniTest::Unit::TestCase
  def test_adds_roman_numerals
    input = StringIO.new("I\nII\nIII\n")
    assert_equal 6, RomanSequence.new(input).sum
  end

  def test_skips_invalid_numerals
    input = StringIO.new("I\nY\nIII\n")
    assert_equal 4, RomanSequence.new(input).sum
  end  
end

PLEASE

Solve the exercise before continuing.

Bad solution: going global

We’d like to stop RomanNumeral from producing error messages, but we have no way to reach the RomanNumeral from the unit test. What to do? One possible solution is to add a class variable to RomanNumeral that controls the verbosity.

def test_skips_invalid_numerals
  RomanNumeral.we_are_in_test_mode = true
  input = StringIO.new("I\nY\nIII\n")
  assert_equal 4, RomanSequence.new(input).sum
end  

1.0

Yuck! The name of this variable is terrible. Our RomanNumeral should not care about running in test or running in production. It may care about running in “verbose” or “quiet” mode. The name “we_are_in_test_mode” makes the code dependent on the context it runs in. This is bad; we want our code to be independent on context, so that we can use it in different contexts. The names “verbose” and “quiet” refer to what the object actually wants to do, not to where it is used.

So we’ve decided that we want to use “quiet” and “verbose”. Which one will be the default? Since we’d prefer not to change all production code that calls RomanNumeral, we prefer to make “verbose” the default, and change our test to request “quiet” mode.

def test_skips_invalid_numerals
  RomanNumeral.quiet = true
  input = StringIO.new("I\nY\nIII\n")
  assert_equal 4, RomanSequence.new(input).sum
end  

1.1

This is slightly better, but still it relies on what is, in the end, a global variable. All class variables (called “static” variables in other languages) are global variables: accessible from everywhere, modifiable by anybody.

The variable would be used in RomanNumeral like this:

class RomanNumeral  
  def to_i
    case @value
      ...
    else
      STDERR.puts("Invalid numeral '#{@value}'") unless @quiet
      return 0
    end
  end
end

A variant of this theme would be to define a globally accessible logger facility, like

def test_skips_invalid_numerals
  Logger.level = Logger::FATAL # silence warnings and errors
  input = StringIO.new("I\nY\nIII\n")
  assert_equal 4, RomanSequence.new(input).sum
end  

1.2

The logger would be used in RomanNumeral like this:

class RomanNumeral  
  def to_i
    case @value
      ...
    else
      Logger.warn("Invalid numeral '#{@value}'")
      return 0
    end
  end
end

This relies on a global variable (that is, Logger.level) and a class method (that is, Logger.warn). Class methods are, in effect, not really methods but global procedures. So what, I hear you say? What’s the problem?

Well… it might not be a problem; it could work. But consider this: relying on a global variable increases complexity in your tests, because you have to change the variable before the test, and then you must remember to reset it to its previous value after the test. Thus, the global also increases the risk of interaction between tests. (We want our tests to be independent!)

The Logger variant is slightly better because we are moving the decision to write to STDERR in a separate procedure. If STDERR were used in many places in our code, it would be difficult to change the output file to something else should we want to. Moving this decision to the procedure Logger.warn makes it easier to change.

Still, we’re relying on a global procedure. This means that if we change that procedure, all calling code is affected. This makes it more difficult to have different logging behaviour in different places. Changing the global verbosity level will affect all logging behaviour of the application.

If you want flexible code, class (static) variables and methods ought to be avoided if at all possible.

Scores:

• Award yourself -1 points if your solution uses a global Logger facility, like snippet (1.2), for using global procedures instead of objects.
• Award -1 points if your solution uses a global variable, like snippet (1.1)
• Award -3 points if your solution uses a global variable that has a name like that of snippet (1.1), for using a context-dependent name.

Bad solution: passing information up the call stack

If we want to avoid global state, one idea would be to equip each single instance of RomanNumeral with its own verbosity setting. The problem here is that the Roman Numeral object is created in a place that is not accessible from the test:

def test_skips_invalid_numerals
  input = StringIO.new("I\nY\nIII\n")
  assert_equal 4, RomanSequence.new(input).sum
end  

What can we do? We cannot pass the RomanNumeral to RomanSequence as a collaborator, because in fact we need not one but many RomanNumbers. In these cases, one thing you can do is to pass a factory that creates RomanNumbers. The factory that is used in the tests will create “quiet” RomanNumbers, the factory that will be used in production will create “verbose” ones.

def test_skips_invalid_numerals
  input = StringIO.new("I\nY\nIII\n")
  factory = RomanNumberFactory.new(false)
  assert_equal 4, RomanSequence.new(input, factory).sum
end  

2.0

This is ugly. What does “false” mean? A better way is to pass an explicit hash of options. The hash in Ruby look like this:

def test_skips_invalid_numerals
  input = StringIO.new("I\nY\nIII\n")
  factory = RomanNumberFactory.new(:quiet => true)
  assert_equal 4, RomanSequence.new(input, factory).sum
end  

2.1

Now at least when we read the code we understand the intent.

An alternative to the factory is to pass the desired verbosity option to the RomanSequence, and have the sequence pass it up to every RomanNumeral it creates:

  
def test_skips_invalid_numerals
  input = StringIO.new("I\nY\nIII\n")
  assert_equal 4, RomanSequence.new(input, :quiet => true).sum
end  

class RomanSequence
  def initialize(input, options)
    @input = input
    @options = options
  end
  
  def sum
    result = 0
    @input.lines.each do |line|
      roman = RomanNumeral.new(line.chomp, options)
      result += roman.to_i
    end
    return result
  end  
end

2.2

All of the solutions in this section work, and are even reasonable when you look at them from here. They are not right, however, because they don’t go in the direction of simplicity. In order to avoid a nasty error message in the tests, we are complicating both RomanNumeral and RomanSequence. In the case of the factory, we are even introducing a new class.

Scores:

• -1 for passing options (2.2) for increased complexity
• -2 for passing a factory (2.1) for even more complexity
• -3 for passing “false” (2.0) for uncommunicative code, and increased complexity

Good solution: handle exceptions at the proper level

The occurrence of an invalid RomanNumeral is an exceptional circumstance; we expect that the numerals will be correct most of the time. What policy should we apply for the case when we encounter an incorrect numeral? One plausible policy is to stop the execution of the whole program. Another plausible policy is to ignore the invalid numeral and carry on. Yet another policy is to log an error message and then ignore the invalid numeral.

Which one should we choose? It depends! We might want to choose one or the other route in different programs, or in different parts of the same program. The point I want to make is that it’s not RomanNumeral’s place to decide which policy to adopt. All that RomanNumeral should do is to signal that it can’t continue by raising an exception. Returning a conventional number like “0” is also an option, but is much worse. When “0” is used in the context of a sum, it will be ignored. But what if we were using the number in a multiplication? The end result would be changed to zero.

We want to learn two principles here:

• The Policy-Mechanism Separation Principle. When a computation can’t continue, it should raise an exception, not return a conventional value. Let someone else decide what to do. Don’t embed the policy in the object; this would make the object context-dependent.
• The Centralized Exception Handling Principle.
  Handling exceptions shouls be centralized, in some place low in the call stack.

In general, the policy of how to deal with exceptions should be centralized in one place. The best place, in general, is close to the main loop of the application.

class RomanSequence
  def initialize(input, options={})
    @input = input
    @options = options
  end

  def sum
    result = 0
    @input.lines.each do |line|
      begin
        roman = RomanNumeral.new(line.chomp)
        result += roman.to_i
      rescue ArgumentError => e
        STDERR.puts(e) unless @options[:quiet]
      end
    end
    return result
  end  
end

Moving exception control to RomanSequence has the benefit of making it easy to control the degree of verbosity. We no longer need a global setting; we can decide verbosity separately for every instance of RomanSequence. Our test becomes:

def test_skips_invalid_numerals
  input = StringIO.new("I\nY\nIII\n")
  assert_equal 4, RomanSequence.new(input, :quiet => true).sum
end  

and RomanNumeral becomes

class RomanNumeral  
  def to_i
    case @value
    # ...
    else 
      raise ArgumentError, "Invalid numeral '#{@value}'"
    end
  end
end

Scores:

• +1 for solving the problem cleanly.

Good solution: move logging to specialized logger object

The previous solution is acceptable, but it raises a question: if our requirement is to log an error message in production, why are we not testing that the message is in fact generated? This solution was suggested by my friend Uberto Barbini.

In order to test that a message is actually generated, we pass the RomanSequence a logger object. In production we will use a real logger that writes on a file somewhere. In test, we will pass a fake logger that keeps its messages in an array in memory. So it goes:

def test_skips_invalid_numerals
  input = StringIO.new("I\nY\nIII\n")
  logger = FakeLogger.new
  assert_equal 4, RomanSequence.new(input, logger).sum
  assert_equal "Invalid numeral 'Y'", logger.last_message
end  

And the fake logger is something as simple as

class FakeLogger
  def error(message)
    @last_message = message
  end
  
  def last_message
    return @last_message
  end
end

The fake logger idea solves both problems: it prevents the ugly error message to appear on the console when we run the tests, and it allows us to check that the proper error message was sent. Note that this logger is much different than the one in snippet 1.2: there we had a global logging procedure, while here we have a regular object.

Still, the main idea about this exercise is to remember to handle the exception at the right level: at the bottom of the call stack.

Scores:

• +2 for solving the problem cleanly, *and* testing for all relevant behaviour.

Conclusions

In this exercise I assign scores to various possible solutions. This implies that I regard some solutions as good, and some as poor. My judgments are based on the premise that there is nothing more to this problem than what I show here. In reality, however, everything depends on context, so it might be that in a broader context, some solution that I judged poor might become reasonable. Always design for the whole, not the particular! But in general, it’s a safe bet that exception handling should be done as near as possible to the outermost application loop.

Further reading

Steve Freeman and Nat Pryce have a lot to say on the subject of logging. See their book Growing Object-Oriented Software (GOOS) and this presentation by Steve. Freeman and Pryce also make a point that code should be context-independent; see GOOS for that.

General wisdom on exceptions:

• Code Complete by Steve McConnell, section 2.8
• Clean Code, chapter 7 by Michael Feathers.
• Effective Java by Joshua Bloch, chapter 9.

This entry was posted on 28 August 2012 at 21:51 and is filed under Agile, Design Exercise. You can follow any responses to this entry through the RSS 2.0 feed. You can leave a response, or trackback from your own site.