Wednesday, November 17, 2010

C#: Opening Up For Mocking Unmockable Classes With The dynamic Keyword

I recently found myself in need of mocking the HttpContext class in an ASP.NET web application. The challenge is that HttpContext is sealed (can't inherit from it), doesn't inherit from anything that defines its interesting methods and is owned by the .NET API, so it can't be changed directly.

There is of course a traditional method of handling this situation, which is to write a very thin wrapper class that calls on the HttpContext methods and inherits from an interface for its own methods. While this class can't be tested directly, if every method is a simple passthrough method or property, just relaying parameters and results, it is a fairly safe class to have untested. And since it inherits from an interface, it's easy to make a mock of it for getting anything that is dependent on HttpContext under test.

Below is an example of such a wrapper class, making the SkipAuthorization property available for mocking.
public interface IHttpContextWrapper
{
    bool SkipAuthorization { get; set; }
}

public class HttpContextWrapper : IHttpContextWrapper
{
    public bool SkipAuthorization
    {
        get { return HttpContext.Current.SkipAuthorization; }
        set { HttpContext.Current.SkipAuthorization = value; }
    }
}
This is simple enough, but does add a layer to your project that doesn't have a natural fit, is there purely to facilitate testing, and contains more code to maintain.

However, with .NET 4.0 comes the dynamic keyword, which allows us to use some tricks that were earlier only available to dynamic languages, like Ruby or Python. So, if you're a programmer in a dynamic language, feel free to chuckle at us strongly typed language programmers finally getting it, and move on.

A common feature to dynamic languages is that you can match on behavior, rather than type. As such, if I have two classes with an .Execute() method, in a dynamic language they can be used interchangeably, so long as you're only using that method. This is true even if the two classes do not share a common parent of any sort.

The new 'dynamic' keyword in C# gives us this same ability, making available a new option for mocking away HttpContext or other difficult classes.

Take the following method. It's trivial and probably pointless since you can just check the HttpContext object directly, but it should be fine for illustrating the point.
public bool ShouldSkipAuthorization(HttpContext context)
{
    return context.SkipAuthorization;
}
Set up like this, all you can pass in is an HttpContext object. However, we can make the simple following change:
public bool ShouldSkipAuthorization(dynamic context)
{
    return context.SkipAuthorization;
}
Now I've replaced the HttpContext parameter type with the keyword dynamic. What this means is that instead of looking at the type of the object passed in, only the behavior will be considered. In other words, so long as the object passed in has a property called SkipAuthorization that returns a bool, it will run fine.

Changing the parameter to dynamic instead of HttpContext will work fine with the existing code. However, it also lets us do the following:
[TestFixture]
public class HttpContextHandlerTest
{
    [Test]
    public void WhenPassingFakeHttpContextResultIsValid()
    {
        var fakeContext = new FakeHttpContext { SkipAuthorization = true };
        var handler = new HttpContextHandler();
        var skip = handler.ShouldSkipAuthorization(fakeContext);
        Assert.IsTrue(skip);
    }
}

public class FakeHttpContext
{
    public bool SkipAuthorization { get; set; }
}
Here I've created a dummy class under my control that has a SkipAuthorization property. It is in no way related to HttpContext beyond having a property of the same name. Still, because of the dynamic keyword, I can pass this in and since everything matches, the code will execute, allowing me to mock out the HttpContext.

This is a very powerful tool, but there are some caveats.

As soon as you use the dynamic keyword, your method is no longer type safe. C# is a statically typed language, and the dynamic keyword gives you the option to bypass that completely. If you pass in an invalid object, you won't know that it crashes until it gets called at runtime. The loss of type safety is something that makes most strong-type programmers cower under their desks in fear.

However, with solid testing around it, and a sanity check in the method to check the incoming type and handle the failure gracefully if it doesn't match, the dynamic typing isn't so much a problem. Remember that people who work in dynamic languages write all their code that way, and it still somehow works.

Secondly, you lose all hints as to what types you actually should be passing in. You can no longer just look at the method signature to see what is expected. This can be sort of solved by adding a summary header to the method, but it feels hacky.

In conclusion, using the dynamic keyword lets you add testability without adding additional layers of abstraction that have no purpose in your code other than to add testability. It also means that you don't have untested wrapper code. What you lose is some code clarity and compile-time checking of type consistency.

Whether that's a fair tradeoff or not is up to you, but it is a fairly elegant way of sneaking in mock objects where there previously was a bunch of extra code required. In my own opinion, this can be a good viable middle stage while breaking up legacy code. However, I would want to get it back to static typing as soon as possible through redesigning the ugly code around it. I have nothing against dynamic coding in general, but I think it breaks a bit with the static typing paradigms that C# code is generally based on and may cause more confusion than it's worth for developers not familiar with the codebase.

Friday, June 25, 2010

When and Where to Fake (Mock/Isolate/Stub/etc.)

After learning how to make fakes (mocks, stubs, isolators, etc.) a part of our unit tests, it's easy to say, "Cool! I'm faking everything now!" I think that's especially true if we've just learned how to use a cool isolation framework at the same time. There is an elegance in ensuring that all we are testing in the code is the method we're looking at and that method only. However, this can be true even if there are dependencies.

At the right time and place, fakes are great tools for unit tests, or to serve as placeholders for unimplemented parts of production code. But I think it's important to think about why fakes are used to begin with.

When to Fake
So I ask myself a few things before I decide to fake a dependency in my system.

Does this dependency affect or rely on anything outside my system? In other words, do I read from or write to a file or database, call a web service, use an external framework and so on? Any external resource is considered unreliable and outside the scope of my tests. This is probably the most common reason to add a fake. In this case, writing fakes is almost without exception the right thing to do.

Is the code in this dependency fully tested? If there is code in my project that is not fully under test, I more or less expect it to have bugs. It may not, but I don't know that. The best solution is to get that code under test, but sometimes that isn't practical in the short term. So the next best thing is to fake the dependency. Then I at least know that errors are coming from the code I am testing. This incurs a bit of technical debt, however, since I have to remember to remove the fakes once the dependency is under test.

Does this dependency cause my tests to run significantly slower? While all of the dependencies may be fully tested code belonging to my project, there sometimes are processes that take a lot of time to complete. Those dependencies should be faked away so that I can run my tests frequently and maintain the TDD fail/fix/refactor cycle.

Why Not to Fake
There are also some good reasons to not use fakes, or at least use them as sparingly as possible.

Faking adds complexity to a test. Whether it is done manually by making a dummy class or by invoking some faking framework, the code becomes harder to read. Frameworks in particular have syntaxes that can seem cryptic to someone not familiar with them.

Also, faking will often cause more tests to be necessary, since a fake will break the data flow of a method, effectively short-circuiting the logic. Where I before just had to test a state or return value, I now also have to test an interaction to make sure that code behaves correctly before the insertion point of the fake. Most of the time, this means that I need an additional test to ensure that the fake is being called with the correct parameters.

Code Example
For this last point, I'm adding a code example here, since I'm not sure if I'm saying it clearly enough, and the code describes it better than I do.

Let's say I have a value generator class, which depends on a web service to deliver its values. By my guidelines above, this is an integration that definitely should be faked, but it's important to think about the effect that has on the tests.

First I establish a ValueGenerator class. It has a WebService dependency which has a method called GetAValue. The class can be instantiated without parameters, or by injecting a WebService object.
public class ValueGenerator
{
    private readonly WebService service;

    public ValueGenerator() : this(new WebService()) { }
    public ValueGenerator(WebService service)
    {
        this.service = service;
    }

    public int GetAValue()
    {
        return service.GetAValue();
    }
}
If I'm testing this without faking, which effectively makes it an integration test rather than unit test, then the test code is simple.
[TestClass]
public class ValueGeneratorTest
{
    // We happen to know that a call to GetAValue with
    // no parameters returns 10. Extremely bad form
    // however, since we don't control the external
    // resource.
    [TestMethod]
    public void GetAValue_CalledWithoutFake_10Received()
    {
        var generator = new ValueGenerator();
        var value = generator.GetAValue();
        Assert.AreEqual(10, value);
    }
}
However, we want to isolate the WebService and not be reliant on it being available, so we create a fake, called FakeWebService, which inherits from WebService. That results in the following tests.
[TestClass]
public class ValueGeneratorTest
{
    [TestMethod]
    public void GetAValue_CalledWithFake_ServiceIsCalled()
    {
        var service = new FakeWebService();
        var generator = new ValueGenerator(service);
        generator.GetAValue();
        Assert.IsTrue(service.ServiceCalled);
    }

    [TestMethod]
    public void GetAValue_CalledWithFake_5Received()
    {
        var service = new FakeWebService();
        var generator = new ValueGenerator(service);
        var value = generator.GetAValue();
        Assert.AreEqual(5, value);
    }

    private class FakeWebService : WebService
    {
        public bool ServiceCalled { get; private set; }

        public override int GetAValue()
        {
            ServiceCalled = true;
            return 5;
        }
    }
}

So in conclusion, faking is a great thing, but give it some thought before using it.

Sunday, May 30, 2010

The Bowling Kata in F# - Yup, More Bowling Katas

I actually have another non-bowling blog post almost ready to go (really!), but I had to present F# to my coworkers recently, and so I resorted to my favorite little code kata to use as an example. We didn't get much time for it in our meeting, so I said I would put it online in a blog post as well. So here it is.

I haven't gotten so far as to really get going on writing unit tests in F#, and from what I've read, it seems that there are still some issues using the MS test runner and F#, so my unit tests are written in C#. This does, however, show how the .NET languages tie neatly together for most intents and purposes.

Whether we will begin to use F# or not, I don't know. Functional programming is fun, but getting everyone proficient at using both C# and F# could prove a challenge. A decent piece on the benefits of F# was written by Kevin Hazzard, Microsoft MVP, which at least highlights some of the key things it provides.

If nothing else, functional programming is a fun and different way to think of programming compared to imperative languages like C#, C/C++, Java, etc. It's well worth the time to check it out.

So, without further ado, here is the bowling kata, written for F#.

The C# to F# Extension Method
I used my tests from my C# version of the scorer as my basis for the tests here. The content of each test is identical, but code had to change a little to accommodate F#'s immutability. That is, variables and collections in F# cannot be changed after they've been created, just as with other functional languages. However, there are ways to convert the lists, and what I finally did was to write an extension method in C# that converts a C# list into an F# list.
using System.Collections.Generic;
using Microsoft.FSharp.Collections;

namespace BowlingKataTest
{
    public static class FSharpInteropExtensions
    {
        public static FSharpList<t> ToFSharpList<t>(this IEnumerable<t> list)
        {
            return ListModule.OfSeq(list);
        }
    }
}
This allows me to just tack on .ToFSharpList() on any collection that implements IEnumerable and turn it into an immutable F# list.

Making My Rolling Easier
Also, to make my rolling of multiple balls easier, I made a short utility method as follows:
private static IEnumerable<int> RollManyBallsWithSameNumberOfPins(int numberOfBalls, int pins)
{
    for (var i = 0; i < numberOfBalls; i++)
        yield return pins;
}
The Bowling Scorer
As usual, I begin with a test, rolling all gutterballs and receiving a score of 0. Note the extension method.
[TestMethod]
public void CalculateScore_AllBallsAreGutterBalls_0()
{
    var rolls = RollManyBallsWithSameNumberOfPins(20, 0);
    var score = Scorer.CalculateScore(rolls.ToFSharpList());
    Assert.AreEqual(0, score);
}
This is solved with a simple function in F#. But first of all, let me explain the F# module header code.
#light

module Scorer
The #light keyword tells the compiler to compile this class using lightweight syntax. What this basically means is that you don't have to use as many keywords, and that whitespace formatting matters. While that sounds counter-intuitive to a c/c++/c#/java programmer, it's pretty typical in functional languages, and generally the code reads better that way. You get used to it.

"module Scorer" declares the module, which works in many ways like a C# static class. Since all methods and values in functional programming are immutable, having properties or class members doesn't make any sense. All are static. Writing a whole C# program where everything is static could make for an interesting exercise in itself.

The actual function is short and sweet.
let CalculateScore rolls = 0
I think this is pretty self-explanatory, but the most notable is that "rolls" is a parameter to the function. Note that no type is declared, and while it is possible to declare the type, the best is to let the compiler figure that out on its own.

This does not mean that F# is weakly typed, but rather that the type is determined at compile time. This is similar to using the "var" keyword in C#. If the type cannot be deduced by the compiler, you'll have to cast the value. If possible, your code will be more flexible if you can let the compiler figure it out on its own. However, you can cast it as follows.
let CalculateScore (rolls : List<int>) = 0
The test passes, so we move on testing for knocking down one pin for each roll.
[TestMethod]
public void CalculateScore_AllBallsKnockDownOnePin_20()
{
    var rolls = RollManyBallsWithSameNumberOfPins(20, 1);
    var score = Scorer.CalculateScore(rolls.ToFSharpList());
    Assert.AreEqual(20, score);
}
To solve this test, we just have to sum up all the rolls, so we do. Note that at this point it does become necessary to specify what type the "rolls" parameter is, so that the summation call understands how to use it.
let CalculateScore (rolls : List<int>) = Seq.sum rolls
Unlike languages based on C-syntax, there is no need for parentheses around the parameter when making a function call unless the call is ambiguous.

This passes both tests so far. So we move on to the next test, where the first roll is a strike, but all following rolls are ones.
[TestMethod]
public void CalculateScore_FirstFrameIsStrikeRestOnes_30()
{
    var rolls = new List<int> {10};
    rolls.AddRange(RollManyBallsWithSameNumberOfPins(18, 1));
    var score = Scorer.CalculateScore(rolls.ToFSharpList());
    Assert.AreEqual(30, score);
}
To solve this in F#, we need to make the function recursive, which allows us to score by frame. So first I rewrite the current functioning code and make sure the previous tests still pass.
let rec CalculateScore rolls = 
    match rolls with
        | x::y::rest -> x+y + CalculateScore rest
        | _          -> 0
This looks a bit more complex, but really isn't too bad. The match statement is a lot like a switch statement in imperative languages like C#, but with a wider range of match possibilities.

The rec keyword tells the compiler that this function is recursive. Then I tell the method to match the pattern x::y::rest from the rolls list, which means that x and y are split off the front of the list, and the remaining list is stored in rest. I then add together x and y and then add the result of passing rest to the recursive call.

Finally, I have to add a catchall condition, in case the x::y::rest pattern isn't matched. The underscore matches anything, like an asterisk (*) in file systems, or percent (%) in SQL. In this case I tell the call to return zero. With our current tests, this shouldn't be hit, but to compile, F# requires that you've covered all match possibilities.

I run the tests, and they pass, so then I add the strike functionality, which is a new pattern to match.
let rec CalculateScore rolls = 
    match rolls with
        | 10::y::z::rest -> 10+y+z + CalculateScore (y::z::rest)
        | x::y::rest     -> x+y    + CalculateScore rest
        | _              -> 0
The new pattern to match is 10::y::z::rest, which represents a ten, followed by two rolls y and z, and then the rest of the list. If so, it adds the ten and the next two rolls, and then calculates the score for the next frame, which includes y and z. Passing y::z::rest puts y, z and rest together into a new list that is passed to the recursive call.

Now that we've seen that we can handle one strike, we should check to see that we handle all strikes, most importantly the last one, with the two bonus balls at the end that should only be counted to score the strike.
[TestMethod]
public void CalculateScore_AllFramesAreStrikes_300()
{
    var rolls = RollManyBallsWithSameNumberOfPins(12, 10);
    var score = Scorer.CalculateScore(rolls.ToFSharpList());
    Assert.AreEqual(300, score);
}
The problem here is that the last two rolls get counted when they shouldn't, so the current code scores 320 instead of 300. The easiest way to solve that is with a frame counter. The client shouldn't have to worry about tracking frames, so I extract the frame rolling to its own function and have the original function call it with initializing values.
The first goal is to make the already passing tests pass with the new arrangement.
let rec CalculateFrame rolls frame  =
    match rolls with
        | 10::y::z::rest -> 10+y+z + CalculateFrame (y::z::rest) (frame+1)
        | x::y::rest     -> x+y    + CalculateFrame rest         (frame+1)
        | _              -> 0        

let CalculateScore rolls = 
    CalculateFrame rolls 0
Note that the order of functions is important. F# seems to evaluate from the bottom up, so CalculateFrame has to appear above CalculateScore in the source file. As you can see, CalculateScore simply calls CalculateFrame, initializing it with the starting frame, which is zero.

CalculateFrame recurses until it runs out of frames, then totals up the score as it resolves back to the calling function. Since, as I mentioned above, this causes it to total up the bonus frames for the last strike incorrectly, we'll put in a frame cap so the function knows when to stop. We do this in the form of another case, with a qualifier that once the frame count reaches 10, the recursion unfolds and the score is totaled.
let rec CalculateFrame rolls score frame  =
    match rolls with
        | _ when frame = 10 -> 0
        | 10::y::z::rest    -> 10+y+z + CalculateFrame (y::z::rest) (frame+1)
        | x::y::rest        -> x+y    + CalculateFrame rest         (frame+1)
        | _                 -> 0
Now the score for all strikes is 300, as it should be. All that remains is handling spares. So we test for a single spare followed by ones in each following roll.
[TestMethod]
public void CalculateScore_FirstFrameIsSpareRestOnes_29()
{
    var rolls = RollManyBallsWithSameNumberOfPins(2, 5).ToList();
    rolls.AddRange(RollManyBallsWithSameNumberOfPins(18, 1));
    var score = Scorer.CalculateScore(rolls.ToFSharpList());
    Assert.AreEqual(29, score);
}
To make the test pass, we just add another case in the match list. Simply, it matches the next three balls, if the first two of those add up to 10.
let rec CalculateFrame rolls score frame  =
    match rolls with
        | _ when frame = 10         -> 0
        | 10::y::z::rest            -> 10+y+z + CalculateFrame (y::z::rest) (frame+1)
        | x::y::z::rest when x+y=10 -> 10+z   + CalculateFrame (z::rest)    (frame+1)
        | x::y::rest                -> x+y    + CalculateFrame rest         (frame+1)
        | _                         -> 0
The test passes, so we try for all spares.
[TestMethod]
public void CalculateScore_AllFramesAreSpares_150()
{
    var rolls = RollManyBallsWithSameNumberOfPins(21, 5);
    var score = Scorer.CalculateScore(rolls.ToFSharpList());
    Assert.AreEqual(150, score);
}
This passes right away, since it really just tests that we stop at the tenth frame, which we already implemented for strikes. It's always nice to see that practice matches theory, however.

So anyway, there's another bowling kata, working in F#.

The Tests
using System.Collections.Generic;
using System.Linq;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace BowlingKataTest
{
    [TestClass]
    public class ScorerTest
    {
        private static IEnumerable<int> RollManyBallsWithSameNumberOfPins(int numberOfBalls, int pins)
        {
            for (var i = 0; i < numberOfBalls; i++)
                yield return pins;
        }

        [TestMethod]
        public void CalculateScore_AllBallsAreGutterBalls_0()
        {
            var rolls = RollManyBallsWithSameNumberOfPins(20, 0);
            var score = Scorer.CalculateScore(rolls.ToFSharpList());
            Assert.AreEqual(0, score);
        }

        [TestMethod]
        public void CalculateScore_AllBallsKnockDownOnePin_20()
        {
            var rolls = RollManyBallsWithSameNumberOfPins(20, 1);
            var score = Scorer.CalculateScore(rolls.ToFSharpList());
            Assert.AreEqual(20, score);
        }

        [TestMethod]
        public void CalculateScore_FirstFrameIsStrikeRestOnes_30()
        {
            var rolls = new List<int> {10};
            rolls.AddRange(RollManyBallsWithSameNumberOfPins(18, 1));
            var score = Scorer.CalculateScore(rolls.ToFSharpList());
            Assert.AreEqual(30, score);
        }

        [TestMethod]
        public void CalculateScore_AllFramesAreStrikes_300()
        {
            var rolls = RollManyBallsWithSameNumberOfPins(12, 10);
            var score = Scorer.CalculateScore(rolls.ToFSharpList());
            Assert.AreEqual(300, score);
        }

        [TestMethod]
        public void CalculateScore_FirstFrameIsSpareRestOnes_29()
        {
            var rolls = RollManyBallsWithSameNumberOfPins(2, 5).ToList();
            rolls.AddRange(RollManyBallsWithSameNumberOfPins(18, 1));
            var score = Scorer.CalculateScore(rolls.ToFSharpList());
            Assert.AreEqual(29, score);
        }

        [TestMethod]
        public void CalculateScore_AllFramesAreSpares_150()
        {
            var rolls = RollManyBallsWithSameNumberOfPins(21, 5);
            var score = Scorer.CalculateScore(rolls.ToFSharpList());
            Assert.AreEqual(150, score);
        }
    }
}
The Code
#light

module Scorer


let rec CalculateFrame rolls frame =
    match rolls with
        | _ when frame = 10         -> 0
        | 10::y::z::rest            -> 10+y+z + CalculateFrame (y::z::rest) (frame+1)
        | x::y::z::rest when x+y=10 -> 10+z   + CalculateFrame (z::rest)    (frame+1)
        | x::y::rest                -> x+y    + CalculateFrame rest         (frame+1)
        | _                         -> 0

let CalculateScore rolls =
    CalculateFrame rolls 0

Friday, April 30, 2010

Unit Testing Methods That Use Delayed Execution In C#

I've found the yield break/return construct and LINQ to Objects in C# to be really handy for shorter, more concise and more readable code in many cases. However, their lazy evaluation can also trip you up if you don't pay attention to when they are evaluated. I'll illustrate below.

The Same Method - Three Ways
Just as a reminder, if you don't use these constructs very often, here are three versions of a simple method that converts a collection of strings into integers.
public IEnumerable<int> ConvertStringToInt1(IEnumerable<string> strings)
{
    var returnCollection = new List<int>();

    foreach(var str in strings)
        returnCollection.Add(int.Parse(str));

    return returnCollection;
}

public IEnumerable<int> ConvertStringToInt2(IEnumerable<string> strings)
{
    foreach(var str in strings)
        yield return int.Parse(str);
}

public IEnumerable<int> ConvertStringToInt3(IEnumerable<string> strings)
{
    return strings.Select(str => int.Parse(str));
}
Late Evaluation
There is a significant difference between the first example and the others. The latter two, using yield return and LINQ to Objects respectively, aren't executed until the resulting collection is evaluated. This can happen much later in the program flow, or may not even happen at all, and can lead to some confusing results when the code is run, whether in tests or otherwise.

This generally isn't an issue if you're testing the result itself, since it will require you to evaluate the contents of the result, which then will trigger the method execution.

For example, the following test should pass without issues:
[TestMethod]
public void ConvertStringToInt2_PassIntsAsString_ValuesAsInt()
{
    var converter = new Converter();
    var strings = new List<string> { "1", "2", "3", "4", "5" };
    var collection = converter.ConvertStringToInt2(strings);
    Assert.IsTrue(collection.Any());
}
When Any() is called on the collection, evaluation starts and continues until it encounters an element, checking that the collection is not empty. If you step through the code, you should see that the ConvertStringToInt2 method is called once.

Side Effects
So far so good.

However, often we want to do more than just a simple call when iterating. Often we want other things to happen in addition, and this is where it becomes easy to trip up.

Consider the following variant of the string to integer converter, which has received the additional responsibility to update a flag if it encounters the value "4" in the conversion set.
public bool FourFound { get; private set; }

public IEnumerable<int> ConvertStringToInt4(IEnumerable<string> strings)
{
    foreach(var str in strings)
    {
        if (str == "4")
            FourFound = true;

        yield return int.Parse(str);
    }
}
Obviously, we'd want to test this behavior as well, and at first glance, the following test should do that.
[TestMethod]
public void GetCollection_Call_CollectionReturned()
{
    var converter = new Converter();
    var strings = new List<string> { "1", "2", "3", "4", "5" };
    converter.ConvertStringToInt4(strings);
    Assert.IsTrue(converter.FourFound);
}
However, this test fails.

The reason is that the result of the ConvertStringsToInt4 method is never evaluated since the result is never used anywhere. As such, the method is never actually called, and FourFound is never set to true.

The simple, short-term solution to this is to force an evaluation. A simple way is to tack on a .ToList() conversion to the method call as below.
[TestMethod]
public void GetCollection_Call_CollectionReturned()
{
    var converter = new Converter();
    var strings = new List<string> { "1", "2", "3", "4", "5" };
    converter.ConvertStringToInt4(strings).ToList();
    Assert.IsTrue(converter.FourFound);
}
As soon as .ToList() is added, the program must iterate through the conversion to create the list, and FourFound is set as expected. This can be an OK way to handle these situations in tests, in my opinion, but it is a warning sign.

Also note that it is necessary to iterate through the whole collection to ensure that all side effects are triggered. Using .Any(), for example, would not make this test pass, since the fourth element would never be evaluated. Likewise, assigning the method call to a variable will not iterate, since all that is assigned is a reference.

The Bigger Picture
While the .ToList() trick can make your tests pass or even make code function as expected, having something that volatile in your code can be dangerous. If I had production code checking the value of FourFound before it would be natural to evaluate the method result, there would be a bug there that could be pretty tricky to hunt down. That kind of code would force the programmer to remember to force an evaluation first, which defeats the point of having automatically delayed evaluation in the first place.

The Single Responsibility Principle (SRP) becomes key here. If your method has no other responsibilities than to generate the new collection, there is no way that other code will rely on anything in the collection before it's evaluated. Ideally, all code that doesn't directly lead to building the new collection is refactored out.

Sometimes, this can be difficult, and can result in having to iterate through the same collection multiple times. I might be processing a list that has come from the URL query string and I want to validate each element before I convert it. For collections known to be small, this may not be a big deal.

If the collections are large, however, or a lot of processing is necessary, this can have direct performance implications. If it is necessary to process everything with one iteration through the collection, my recommendation is to save yourself some trouble and use a traditional loop, like in ConvertStringToInt1, where there is no delayed evaluation.

Saturday, March 13, 2010

Bowling Kata as a Haskell Exercise (First Attempt)

At this point I wonder if I should consider renaming this blog to be the Bowling Kata blog. The reason I keep using it is that I find the complexity of the kata is just about right for a simple but non-trivial task, and so it will probably show up again; most likely in x86 assembly, but that's for another time. There is some other content on the way as well, I promise.

I'm currently trying to learn Haskell, a purely functional language, so with that in mind, I figured I'd try to implement the bowling kata as a first exercise, using the HUnit test framework to test it. I'm sure that this implementation will broadcast clearly to anyone in the know that I come from an OO imperative programming world, so I'll welcome any constructive criticism as usual.

For those interested in looking into Haskell, both of these tutorials have been of great help so far.
For HUnit, Getting Started With HUnit has been great to quickly get the hang of it.

Anyway, without further ado, the Bowling Kata, Haskell style.

Bowling Kata in Haskell
I'm following the same test order as I did with the C# implementation, so my first test is for a game of all gutterballs. Just to show how the test module is set up, this first code snippet also includes the header info.
module ScorerTest where

import HUnit
import Scorer

main = runTestTT $ TestList [allGuttersReturnZero]

allGuttersReturnZero = TestCase $ assertEqual
    "All gutterballs" 0 (calculateScore $ replicate 20 0)
And the following implementation of calculateScore looks like this. In short, define a function that takes a list of type number, and returns a number. To make the first test pass, we accept the list xs, but return zero no matter what.
module Scorer (calculateScore) where

calculateScore :: (Num a) => [a] -> a
calculateScore xs = 0
The next test is to roll all ones, avoiding the more complicated spare and strike cases.
allBallsKnockDownOnePin = TestCase $ assertEqual
    "All balls roll one" 20 (calculateScore $ replicate 20 1)
To make this pass for both test cases, we simply sum up all of the rolls passed in.
calculateScore :: (Num a) => [a] -> a
calculateScore xs = sum xs
Then we begin to get to the meat of the problem, this time rolling a strike on the first frame and following with ones for the rest of the rolls.
firstFrameStrikeRestOnes = TestCase $ assertEqual
    "First frame is strike, rest ones" 30 (calculateScore $ [10] ++ replicate 18 1)
To make this test pass, a bit more work is needed.

First of all, since we're introducing recursion at this stage, the edge case needs to be covered, which is calculateScore being called with an empty list. We set it to return zero if that is the case.

Secondly, we split up the list coming in into head and tails (x:xs), and then check the head (x) to see if it's equal to 10 - a strike. If it is, we sum it up along with the next two values in the list (take 2 xs) and then run the calculation again on the rest of the list. If the value is not 10, we just take off the first element, and add it to the score calculation of the rest of the list.

More succinctly, it looks like the following.
calculateScore :: (Num a) => [a] -> a
calculateScore [] = 0
calculateScore (x:xs)
    | x == 10   = x + sum (take 2 xs) + calculateScore xs
    | otherwise = x + calculateScore xs
Moving on with the tests, we devise one where all of the rolls are strikes to see how the code performs. As expected, we get a score of 330 instead of 300, since the algorithm doesn't know to not count the last two balls as fully scoring strikes. So we consider frames instead of individual balls.
allFramesAreStrikes = TestCase $ assertEqual
    "All frames are strikes" 300 (calculateScore $ replicate 12 10)
I have to admit that I got a little stuck here, since I'm still trying to get the hang of the functional way of thinking, so thanks to Tom Moertel, Isaac Gouy and Phillip Schwarz, whose solutions I found online to help me adjust my plan of attack.

Switching over to using frames as a basis instead of balls required a bit more extensive code rewriting, including breaking frame scoring out into its own function, which takes the list of rolls, the current total and the frame as arguments.

The function itself is split into three cases. First a case to make sure the calculation stops after the 10th frame, then a case to handle the last frame if there are no special rolls. These first two are our edge cases.

Lastly, the general case for the remaining rolls scores the list recursively. It splits the list of rolls into four components - the first three balls and the remaining list. If the first roll is 10 (a strike), it then scores the next frame along with the remaining list, at the same time adding the first three balls to the score. Otherwise it scores the same way, but adding only the first two balls to the score.

This is how it looked when I wrote it.
calculateScore :: (Num a) => [a] -> a
calculateScore rolls = scorePerFrame rolls 0 0
    
scorePerFrame :: (Num a) => [a] -> a -> a -> a
scorePerFrame rolls score 10 = score
scorePerFrame [x,y] score frame = score+x+y
scorePerFrame (x:y:z:rest) score frame 
    | x == 10   = scorePerFrame (y:z:rest) (score+x+y+z) (frame+1)
    | otherwise = scorePerFrame (z:rest)   (score+x+y)   (frame+1)
Once this framework is in place, adding the case for spares is pretty simple. First the test case.
firstFrameSpareRestOnes = TestCase $ assertEqual
    "First frame is spare, rest ones" 29 (calculateScore $ [5,5] ++ replicate 18 1)
And then we just add a line that matches a spare.
scorePerFrame (x:y:z:rest) score frame 
    | x == 10     = scorePerFrame (y:z:rest) (score+x+y+z) (frame+1)
    | x + y == 10 = scorePerFrame (z:rest)   (score+x+y+z) (frame+1)
    | otherwise   = scorePerFrame (z:rest)   (score+x+y)   (frame+1)
In theory, this solution should work fine if I roll all spares as well, but for my own sense of security, I'll write that test anyway.
allFramesAreSpares = TestCase $ assertEqual
    "All frames are spares" 150 (calculateScore $ replicate 21 5)
It passes right away, so no code changes needed. So there you have it, the bowling scorer in Haskell.

The final code follows below. I tried to do a little refactoring, but in the end I found the current terse style was more legible. Part of that I think is a nature of the Haskell syntax. x, y and z could perhaps be replaced with "first, second and third," but I thought it just made it look bulky.

The Tests
module ScorerTest where

import HUnit
import Scorer

main = runTestTT $ TestList [
    allGuttersReturnZero, 
    allBallsKnockDownOnePin, 
    firstFrameStrikeRestOnes, 
    allFramesAreStrikes,
    firstFrameSpareRestOnes,
    allFramesAreSpares
    ]

allGuttersReturnZero = TestCase $ assertEqual
    "All gutterballs" 0 (calculateScore $ replicate 20 0)
    
allBallsKnockDownOnePin = TestCase $ assertEqual
    "All balls roll one" 20 (calculateScore $ replicate 20 1)
    
firstFrameStrikeRestOnes = TestCase $ assertEqual
    "First frame is strike, rest ones" 30 (calculateScore $ [10] ++ replicate 18 1)

allFramesAreStrikes = TestCase $ assertEqual
    "All frames are strikes" 300 (calculateScore $ replicate 12 10)
    
firstFrameSpareRestOnes = TestCase $ assertEqual
    "First frame is spare, rest ones" 29 (calculateScore $ [5,5] ++ replicate 18 1)
    
allFramesAreSpares = TestCase $ assertEqual
    "All frames are spares" 150 (calculateScore $ replicate 21 5)
The Code
module Scorer (calculateScore) where

calculateScore :: (Num a) => [a] -> a
calculateScore rolls = scorePerFrame rolls 0 0
    
scorePerFrame :: (Num a) => [a] -> a -> a -> a
scorePerFrame rolls score 10 = score
scorePerFrame [x,y] score frame = score+x+y
scorePerFrame (x:y:z:rest) score frame 
    | x == 10     = scorePerFrame (y:z:rest) (score+x+y+z) (frame+1)
    | x + y == 10 = scorePerFrame (z:rest)   (score+x+y+z) (frame+1)
    | otherwise   = scorePerFrame (z:rest)   (score+x+y)   (frame+1)

Thursday, February 18, 2010

We're All Connected: A Passion For Your Craft

This little musical gem has been stuck in my mind and behind my eyeballs for over a day now. In itself, it doesn't relate directly to coding, but is a tribute to some of the greatest minds of our time, coupled with some fun with auto-tune. I'll explain why I'm posting this afterwards, but watch it first, if you haven't. And if you like it, you'll find more like it at Symphony of Science.


While this clip is neat in its own right, it's not the technical or musical aspects of it that gripped me. Instead, it's the men who are starring in it. It is the enthusiasm that they obviously feel for their fields of study, how they seem to be bubbling inside in near euphoria just to be able to speak about this stuff.

This relates in many ways to any craft, science, job or hobby, including software development. If you love what you do, learning how to do it better becomes fun and interesting. It develops into a passion.

I saw Theo Härén, a creativity consultant, speak recently, and his passion is obviously creativity. It was a fascinating talk, but the question that stuck with me was one that our stand-up comic host posed to Mr. Härén after the talk, which was whether a passion for creativity could be learned. And the question seemed to stump our guest speaker.

However, I think passion is something that does develop. That can be learned. Often a passion for something doesn't arise until you learn more about it. As you practice software development and learn to see the intricacies, patterns and structures within software, it is only then you can really begin to appreciate the beauty of a well put-together program in all its ingenuity.

The same could be said for astronomy, gaming, painting, love, parenting or most anything else.

And I don't think you're a success within your craft until you've discovered that passion, because until then, you're only doing a job. In a lifetime, that is bound to get old someday.

So, you may want to ask yourself, does what you do make you all excited and bubbly on the inside? Awesome! And if not, what would?

Friday, January 8, 2010

C# Bowling Kata as a TDD Exercise (Revisited)

After discussing my first implementation of the Bowling Kata as a TDD exercise with a few different people, and using the Bowling Kata as an example while trying to teach some of my coworkers how TDD works, I found that there were several points I could improve on. Also, I realized that my previous post wasn't as clear as it could have been about which steps I took and when.

So, here's a new variant of the Bowling Kata in C# as a TDD Exercise, which ends up with more or less the same end result, but is more explicit in how it gets there and uncovers some jumps I made in the previous attempt. The changes mainly lie in the process and description.

To start, I realized that I put too much into getting my first test up, so to stay truer to the TDD principles, I've simplified. Here comes take two of my TDD excercise.

Bowling Kata in C# as a TDD Exercise
My first test is as follows. The Scorer class isn't implemented yet.
[Test]
public void CalculateScore_AllBallsAreGutterBalls_0()
{
    var scorer = new Scorer();
    var score = scorer.CalculateScore();
    Assert.AreEqual(0, score);
}
Both the class creation and the CalculateScore methods show up red, as expected, and my first test doesn't compile. I solve that by creating the class and adding the following method.
public int CalculateScore()
{
    return 0;
}
It compiles, and causes my test to pass. This test basically tells me that I have a class that is live and initialized, and a CalculateScore method that works for all gutterballs.

For the next test I need to show that my CalculateScore method can actually calculate the score for more than one defined game, so I set up a second test for rolling all ones, instead of zeros.

Since I'm declaring the scorer object in two places, I refactor it out and initialize it in the setup.
private Scorer scorer;

[SetUp]
public void SetUp()
{
    scorer = new Scorer();    
}
The test looks like the following:
[Test]
public void CalculateScore_AllBallsKnockDownOnePin_20()
{
    for (var i=0; i < 20; i++)
        scorer.Roll(1);

    var score = scorer.CalculateScore();
    Assert.AreEqual(20, score);
}
This forces me to add some logic and a Roll method to allow me to update the number of pins knocked down per roll. I don't need to add any tests that test the Roll method specifically, since that will be proven through getting the correct result from the CalculateScore method. Conversely, I can't get the correct score from the CalculateScore method without the Roll method, which will show that they're both necessary and functioning. The code that makes the new test function without breaking the old test is this:
private int score;

public int CalculateScore()
{
    return score;
}

public void Roll(int pins)
{
    score += pins;
}
Next I need to deal with special rolls, the strikes and spares. Rather than trying to solve both the case of extra frames and special score handling at once, I reduce the next test to represent a strike in the first frame, and the rest of the balls as ones. Again I have some code duplication, since I need another loop to roll the ones, so I separate the ball rolling into its own helper method. Its name is a bit longer than I like, but I don't have a more concise name while leaving its function clear.
private void RollManyBallsWithSameNumberOfPins(int numberOfBalls, int pins)
{
    for(var i=0; i < numberOfBalls; i++)
        scorer.Roll(pins);
}
And the test itself looks like:
[Test]
public void CalculateScore_FirstFrameIsStrikeRestOnes_30()
{
    scorer.Roll(10);
    RollManyBallsWithSameNumberOfPins(18, 1);
    var score = scorer.CalculateScore();
    Assert.AreEqual(30, score);
}
To handle a strike, it becomes necessary to remember each roll, so I add a list of integers to do that, and modify the Roll method to add to it. The score variable can be made local. The score calculation is then as follows (remembering that a bowling strike is 10 + the next two rolls).
private readonly IList<int> rolls = new List<int>();

public int CalculateScore()
{
    var score = 0;

    for (var i = 0; i < rolls.Count; i++)
    {
        if (rolls[i] == 10)
            score += rolls[i] + rolls[i + 1] + rolls[i + 2];
        else
            score += rolls[i];
    }

    return score;
}

public void Roll(int pins)
{
    rolls.Add(pins);
}
The test passes, but there is trouble lurking at the edge cases. So the next test is with all strikes to see how the code handles that.
[Test]
public void CalculateScore_AllFramesAreStrikes_300()
{
    RollManyBallsWithSameNumberOfPins(12, 10);
    var score = scorer.CalculateScore();
    Assert.AreEqual(300, score);
}
As expected, bowling a perfect game blows up at the end of the scoring list. The current algorithm tries to add two balls to the last two bonus balls, which are really just there to help score the last 'true' strike. So we fix the scorer to make the test pass. The easiest way to do this is to start scoring frames instead of individual rolls.
private readonly IList<int> rolls = new List<int>();
private int currentRoll;

public int CalculateScore()
{
    var score = 0;
    currentRoll = 0;

    for (var i = 0; i < 10; i++)
    {
        if (rolls[currentRoll] == 10)
        {
            score += rolls[currentRoll] + rolls[currentRoll + 1] + rolls[currentRoll + 2];
            currentRoll++;
        }
        else
        {
            score += rolls[currentRoll] + rolls[currentRoll + 1];
            currentRoll += 2;
        }
    }
    
    return score;                       
}
This makes my test pass, but wait! My first test suddenly implodes, failing with an out of range exception. While this test has been passing since the very beginning, I actually made a mistake there. It doesn't test that the scorer handles gutterballs, but just if the CalculateScore method exists.

And with the new implementation, it becomes obvious that the CalculateScore method has a temporal (time-based) dependency on the Roll method. In other words, it relies on the Roll method having been called first to initialize the list of rolls. In real life, this is fairly obvious, since it doesn't make sense to score a bowling game that hasn't been played yet, but this could create trouble down the road in code.

There are many ways to handle this, and that goes a bit outside the scope of this excercise. However, the point to notice here is that the unstable code was exposed by the tests. If I were just coding this directly, the instability might have gone into production and been triggered by other code. However, I will fix this the easy way, and correct the gutterballs test to actually roll an appropriate number of zeros.
[Test]
public void CalculateScore_AllBallsAreGutterBalls_0()
{
    RollManyBallsWithSameNumberOfPins(20, 0);
    var score = scorer.CalculateScore();
    Assert.AreEqual(0, score);
}
Now all tests pass again, and we can move on to scoring spares. Again I follow the pattern of just testing the first frame, so I'm not testing for more than one thing at a time. First I roll two fives for a spare and then fill in the rest of the rolls with ones.
[Test]
public void CalculateScore_FirstFrameIsSpareRestOnes_29()
{
    RollManyBallsWithSameNumberOfPins(2, 5);
    RollManyBallsWithSameNumberOfPins(18, 1);
    var score = scorer.CalculateScore();
    Assert.AreEqual(29, score);
}
The following code change makes the test pass, inserting a new conditional between the strike and the regular roll.
if (rolls[currentRoll] == 10)
{
    score += rolls[currentRoll] + rolls[currentRoll + 1] + rolls[currentRoll + 2];
    currentRoll++;
}
else if (rolls[currentRoll] + rolls[currentRoll + 1] == 10)
{
    score += rolls[currentRoll] + rolls[currentRoll + 1] + rolls[currentRoll + 2];
    currentRoll += 2;
}
else
{
    score += rolls[currentRoll] + rolls[currentRoll + 1];
    currentRoll += 2;
}
Next I write a test where all rolls are spares, which also passes, since the logic is already such that the spare frames are taken care of.
[Test]
public void CalculateScore_AllFramesAreSpares_150()
{
    RollManyBallsWithSameNumberOfPins(21, 5);
    var score = scorer.CalculateScore();
    Assert.AreEqual(150, score);
}
Conclusion
And with that, the bowling scorer is functionally complete. Some refactoring remains, which you can see the result of below. Other than that, I have a couple of finishing thoughts which I'm still trying to work out good answers for.

What to do with tests which are covered by later tests? For example, the tests that test the first strike or spare, are covered by the later tests that test all strikes or spares. Should I take those early tests out once the broader tests are written? While I want total test coverage, I at the same time want as few tests as possible to achieve that, so less changes are necessary if the specification should change later. Those tests seem superfluous now.

On the other hand, maybe my tests are bad. Perhaps there is a different test I could write that only tests for the edge case of strike on the last frame and as such keep the responsibilities separate. But even so, I wouldn't feel safe without a test that tested a perfect game, for example, to see that all the parts hang together as they should.

Edit: The original version of this post had the final code unrefactored, which was the result an incorrect copy and paste. I did the quick fix of calling it an exercise for the reader, but since I was called on it, I've gone back and pasted in the refactored code.

Final Code
Tests:
using NUnit.Framework;

namespace BowlingKata02
{
    [TestFixture]
    public class ScorerTest
    {
        private Scorer scorer;

        [SetUp]
        public void SetUp()
        {
            scorer = new Scorer();
        }

        private void RollManyBallsWithSameNumberOfPins(int numberOfBalls, int pins)
        {
            for (var i = 0; i < numberOfBalls; i++)
                scorer.Roll(pins);
        }

        [Test]
        public void CalculateScore_AllBallsAreGutterBalls_0()
        {
            RollManyBallsWithSameNumberOfPins(20, 0);
            var score = scorer.CalculateScore();
            Assert.AreEqual(0, score);
        }

        [Test]
        public void CalculateScore_AllBallsKnockDownOnePin_20()
        {
            RollManyBallsWithSameNumberOfPins(20, 1);
            var score = scorer.CalculateScore();
            Assert.AreEqual(20, score);
        }

        [Test]
        public void CalculateScore_FirstFrameIsStrikeRestOnes_30()
        {
            scorer.Roll(10);
            RollManyBallsWithSameNumberOfPins(18, 1);
            var score = scorer.CalculateScore();
            Assert.AreEqual(30, score);
        }

        [Test]
        public void CalculateScore_AllFramesAreStrikes_300()
        {
            RollManyBallsWithSameNumberOfPins(12, 10);
            var score = scorer.CalculateScore();
            Assert.AreEqual(300, score);
        }

        [Test]
        public void CalculateScore_FirstFrameIsSpareRestOnes_29()
        {
            RollManyBallsWithSameNumberOfPins(2, 5);
            RollManyBallsWithSameNumberOfPins(18, 1);
            var score = scorer.CalculateScore();
            Assert.AreEqual(29, score);
        }

        [Test]
        public void CalculateScore_AllFramesAreSpares_150()
        {
            RollManyBallsWithSameNumberOfPins(21, 5);
            var score = scorer.CalculateScore();
            Assert.AreEqual(150, score);
        }
    }
}
Code:
using System.Collections.Generic;

namespace BowlingKata02
{
    public class Scorer
    {
        private readonly IList<int> rolls = new List<int>();
        private int currentRoll;
        private int score;

        public int CalculateScore()
        {
            score = 0;
            currentRoll = 0;

            ScoreAllRolls();

            return score;
        }

        private void ScoreAllRolls()
        {
            for (var frame = 0; frame < 10; frame++)
            {
                if (rolls[currentRoll] == 10)
                    ScoreStrike();
                else if (SumUpRollsFromCurrent(2) == 10)
                    ScoreSpare();
                else
                    ScoreNormal();
            }
        }

        private void ScoreNormal()
        {
            score += SumUpRollsFromCurrent(2);
            currentRoll += 2;
        }

        private void ScoreSpare()
        {
            score += SumUpRollsFromCurrent(3);
            currentRoll += 2;
        }

        private void ScoreStrike()
        {
            score += SumUpRollsFromCurrent(3);
            currentRoll++;
        }

        private int SumUpRollsFromCurrent(int numberOfRolls)
        {
            var sum = 0;
            for (var i = 0; i < numberOfRolls; i++)
                sum += rolls[currentRoll + i];
            return sum;
        }

        public void Roll(int pins)
        {
            rolls.Add(pins);
        }
    }
}