Gain Confidence Before You Refactor

Every code base as its rough spots. Its just a fact of life. Even on a team of rock stars, there still exists hairy parts of the code that nobody wants to touch. When it does come time for this code to change, there are some great techniques that can reduce the pain.

The crux of the problem with changing nasty code is that there may be insufficient or no code coverage around the code you want to change. The technique that I describe below will help you gain the confidence you need to renovate the code to a squeaky clean state.

Step 1. Start Clean

Its important to start with a clean state. If you have done some exploratory spiking, make sure that you revert those changes or diff them off to a file. If there are tests around the method run them all and make sure they are green. If there are no tests, create one that will cover the success path of the function. This should be at a fairly high level and should have as little mocks as possible. We really want to understand the behavior here, not necessarily the interactions. If you are finding this test difficult to write, get a copy of Michael Feather's Working Effectively with Legacy Code. Its a great resource.

Step 2. Probe for Weakness

It may seem counter-intuitive, but the next step is to break the code in every way imaginable. Delete if statements, swap parameters used to call other methods, negate boolean expressions, anything you can think of. Each time you make a change, re-run the tests and make sure that at least one fails. If there are no failures, create a failing test. After each change has caused a failing test, revert your changes to the method and ensure that the tests once again pass. Repeat this until you feel comfortable that you have good coverage. This is usually a good point to check in.

Step 3. Make Room for the new feature

Now that we have sufficient coverage around the code, we can now begin changing the method to make room for our new feature. The key here is not to introduce any new functionality. We will lean on the tests that we just wrote to ensure that we have not broken anything. Once we have "extract method"-ed and "extract class"-ed enough to fit our new feature in, its time to check-in again.

Step 4. TDD the new feature

We have now gone to all the trouble of building a solid test suite around our code, it would be a shame to screw it up now by introducing untested changes. Therefore, we will TDD the new feature in by writing new tests or changing existing tests that fail and add new production code to make them pass. This may span multiple check ins depending on the complexity of the feature.

Step 5. Applaud yourself for being a mature developer and good teammate

Nobody likes cowboys and cowgirls in their code base. You should feel proud that you have just completed a feature like a professional. It may of taken more time that it would have to "just put it in", but now you are less likely to have an expensive defect in production. Give yourself a pat on the back.

Showing your longest running tests

Slow builds got you down? Have no idea where that slow test is? Get the latest in monkey-patched hotness:

require 'test/unit/ui/console/testrunner'
Test::Unit::UI::Console::TestRunner.class_eval do
 @@test_times = []

 old_attach_to_mediator_method = instance_method(:attach_to_mediator)

 define_method :attach_to_mediator do
   old_attach_to_mediator_method.bind(self).call
   @mediator.add_listener(Test::Unit::TestCase::STARTED, &method(:record_start_time))
   @mediator.add_listener(Test::Unit::TestCase::FINISHED, &method(:record_elapsed_time))
   @mediator.add_listener(Test::Unit::UI::TestRunnerMediator::FINISHED, &method(:print_times))
 end

 def record_start_time(name)
   @start_time = Time.now
 end

 def record_elapsed_time(name)
   @@test_times << {:name => name, :time => (Time.now - @start_time)}
 end

 def print_times(suite_elapsed_time)
   unless @@test_times.empty?
     puts ''
     puts "Displaying 10 longest running tests:"
     @@test_times.sort_by { |t| t[:time] }.reverse[0...10].each do |test_timing|
       puts "#{test_timing[:time]} seconds for #{test_timing[:name]}"
     end
   end
 end

end

And you get some output like this:

Displaying 10 longest running tests:
0.159756 seconds for test_awarding_0.04_points_per_passing_yard(Units::PlayerStatsTest)
0.000432 seconds for test_awarding_6_points_per_return_touchdown(Units::PlayerStatsTest)
0.000367 seconds for test_give_1_point_per_point_after_touchdown_made(Units::PlayerStatsTest)
0.000316 seconds for test_awarding_6_points_per_receiving_touchdown(Units::PlayerStatsTest)
0.000305 seconds for test_awarding_0.1_points_per_receiving_yard(Units::PlayerStatsTest)
0.000297 seconds for test_copping_out_and_giving_3_points_per_field_goal(Units::PlayerStatsTest)
0.00029 seconds for test_awarding_6_points_per_rushing_touchdowns(Units::PlayerStatsTest)
0.00029 seconds for test_penalizing_-2_points_for_fumbles_lost(Units::PlayerStatsTest)
0.000288 seconds for test_penalizing_-1_points_for_interceptions(Units::PlayerStatsTest)
0.000282 seconds for test_awarding_0.1_points_per_rushing_yard(Units::PlayerStatsTest)

Suite setup and teardown in Test::Unit

Test::Unit does not provide an easy way for suite setup and suite tear down methods to be executed. However, like most things in ruby, if there is a will there is a way. Today we managed to implement them using exit hooks.

Before I show you that code, it is important that we understand exactly how Test::Unit works. Here is a simple test:

require 'test/unit'

class ExampleTest < Test::Unit::TestCase
  def test_truth
    puts 'testing truth'
    assert true
  end
end

The part that I am most interested in is the require 'test/unit'. Hidden in the bottom of the code for this class is this code:

at_exit do
  unless $! || Test::Unit.run?
    exit Test::Unit::AutoRunner.run
  end
end

In short, this is the code that allows you to execute your test case rb file and have it actually run the test. This code will be called just before the interpreter exits. To prove to yourself that this is true (don't take my word for it) add this line to your test case anywhere after the require.

at_exit { exit! }

exit! will cause the interpreter to quit while ignoring any exit hooks. Since exit hooks are executed in reverse order of registration, this will effectively short circuit the execution of Test::Unit's exit hook.

Now we are getting somewhere. Using what we have just learned, we can now implement suite setup and teardown. Check it out:

require 'test/unit'

class ExampleTest < Test::Unit::TestCase
  def test_one; puts 'one'; end
  def test_two; puts 'two'; end
end

successful = false
at_exit { exit! successful }
at_exit { puts 'suite tear down' }
at_exit do
  unless $! || Test::Unit.run?
    successful = Test::Unit::AutoRunner.run
  end
end
at_exit { puts 'suite setup' }

When you run this, it will print:

suite setup
Loaded suite *the file*
Started
one
.two
.
Finished in 0.000564 seconds.

2 tests, 0 assertions, 0 failures, 0 errors
suite tear down

Enjoy!