## Exercise: a Pesky Error Message

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 ```

Solve the exercise before continuing.

*            *
*

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.

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:

### One Response to “Exercise: a Pesky Error Message”

1. Giorgio Sironi Says:

My green field solution is in line with the last one presented here. However, I picked up the broader context obsession too and in case we’de dealing with legacy code I would:
– make logger an optional parameter that defaults to the real implementation.
– in tests, pass in the Fake logger
– in production, not touch the calling code
just because I won’t have access to all the places where a RomanSequence is created.
In some languages this add a compile-time dependency from RomanSequence to RealLogger, but it’s slightly better than a dependency to STDERR.