Thursday, January 31, 2008

Which Style Is More Readable?

I've been writing ScalaTest tests for the ScalaTest TestNG integration (say that five times fast). I used a couple of different styles and I was hoping to get some input on which style people thought was more readable, more clear. Both styles are functional, one merely masks it a bit while the other flaunts it.

The first style I'll show is the discrete style. Here I call a test method which takes a function and executes it.


test( "Reporter Should Be Notified When Test Passes" ){

val testReporter = new TestReporter

// when
new SuccessTestNGSuite().runTestNG(testReporter)

// then
assert( testReporter.successCount === 1 )
}


test( "Reporter Should Be Notified When Test Fails" ){

val testReporter = new TestReporter

// when
new FailureTestNGSuite().runTestNG(testReporter)

// then
assert( testReporter.failureCount === 1 )
}


The I'm showing only two examples of calling the test function but in actuality I have many more tests. Notice that in each test the first line is always the same:


val testReporter = new TestReporter


There are benefits to this. All the code is right there and you can read the test method without looking anywhere else. There are also some negatives as well. I have the same line of code in several places.

Now I'll show the even more functional style. It takes a little more explaining. Here I declare a withFixture method that accepts a function that takes a TestReporter as its input. The withFixture method creates the TestReporter (so each test call doesn't have to) and calls the input function passing it the TestReporter. To create tests I call the testWithFixture function which takes a function and passes it to the withFixture function I just wrote.


override def withFixture(f: (TestReporter) => Unit): Unit = {
f(new TestReporter)
}


testWithFixture( "Reporter Should Be Notified When Test Passes" ){
t: TestReporter =>

// when
new SuccessTestNGSuite().runTestNG(t)
// then
assertThat( t.successCount, is(1) )
}


testWithFixture( "Reporter Should Be Notified When Test Fails" ){
t: TestReporter =>

// when
new FailureTestNGSuite().runTestNG(t)
// then
assert( t.failureCount === 1 )
}



I'm realizing as I'm writing this that all the extra explaining I had to do is because the code is more complicated. There are definite negatives here. Code is being passed around to other code, you might now be sure how things are actually running, you might not be sure where the TestReporter object is coming from. On the upside the methods are slightly shorter and I've removed the most obvious duplication by moving it to the withFixture function. There's still plenty of duplication (in each example) that could be removed but you can definitely push duplication removal too far. Especially in tests, you must balance duplication and indirection. They are in direct opposition.

Does this code push it too far?

Is this just a setup method in disguise? I'm of the opinion that setup methods are bad, but at least here everything is immutable.

If people were to get used to this style would it become more readable in the long run?

Please please let me know your thoughts.

No comments:

Post a Comment