Monthly Archives: August 2009

Best and Worst Practises in Unit Testing

Ok, so I had been in the process of writing a blog to summarize my opinions on TDD etc, and this article popped up on my feed reader – it covers a good deal of the things I had wanted to discuss, and is summarized in a very well constructed way. So rather repeating everything, I’ll focus on a couple the issues discussed that I feel are important.

The point I’d like to emphasize in this article, that truly strikes a chord with me is:

 a suite of bad unit tests is immensely painful: it doesn’t prove anything clearly, and can severely inhibit your ability to refactor or alter your code in any way

 

In fact, I can’t emphasize this point enough. A suite of bad tests inhibit change. Brittle unit tests, like brittle code, need to be refactored/replaced. I think the points made under the section “Name your unit tests clearly and consistently” I think are crucial here. Just today for instance, I came across the following tests methods in a class named DutyAssignmentChecks:

SeverityLevels()

ViolationChecks()

WarningChecks()

TranslatedMessage()

 

I was only looking at the test code so get a feel for the current behavior of the system; after all, tests should be executable specifications that define the behavior of our code, what better way to document the system! Looking at this “unit test” code however did not help one bit. Maintenance is hard when you don’t know what you’re maintaining. If I break these tests when modifying the original code base, what value do these tests provide? If I’m changing the expected behavior of the system, then breaking a test may be valid, but how am I to know this is the case, if I can’t deduce what the expected behavior under test is! The article suggests using  the subject, the scenario, and the result in the test name to clarify the behavior under test. This pattern of specifying is the basis of what has evolved into the Behavior Driven Development technique coined by Dan North.  

 

The style of text naming I prefer is as follows:

     using Skynet.Core

      

     public class when_initializing_core_module : concerns

    {

       SkynetCoreModule _core;

       

       public void context()

       {

         //we’ll stub it…you know…just in case

        var skynetController = stub<ISkynetMasterController>();

        _core = new SkynetCoreModule(skynetController);

        _core.Initialize();

       }

      

       [Specification]

       public void it_should_not_become_self_aware()

       {

       _core.should_not_have_received_the_call(x => x.InitializeAutonomousExecutionMode());

       }

      

       [Specification]

       public void it_should_default_to_human_friendly_mode()

       {

         _core.AssessHumans().should_equal(RelationshipTypes.Friendly);

       }

      

       // more specifications under this same context

       // …

     }

I think this kind of style of testing greatly improves the clarity and purpose of both the test, and the therefore subject under test.

The only point the article suggests that I’d take with a small pinch of salt is in the section “Unit testing is not about finding bugs”. I think the author slightly underplays the effectiveness of unit tests detecting regression bugs when you’re making any changes to the existing code. He does mention that unit testing can be effective in finding bugs when refactoring,  which I completely agree with – it’s this factor that makes refactoring confidently possible. I’d also argue however, that it helps find bugs introduced when a developer is extending the code base without following the Open/Closed Principal. In this scenario, the developer is modifying and existing code base to add a new feature or function. Although this scenario is undesirable (see the link to O/C P for details), and I would rather see the developers trained in good OO practice, in my experience I would say this scenario is extremely common in teams without much experience in good OO development. Under these conditions, unit tests are still quiet valuable in finding regression bugs.

 

What do you think?

Advertisements

Why not Make Every Method Virtual?

In an effort to make my blog suck less, I figured I’d post my response to Ward Bells Do not Make Every Method Virtual. He carries on the discussion of whether or not .NET methods should be made virtual by default, as they are in Java. You should read his post before mine, as this will establish the context of everything that follows (and if you don’t already subscribe to his blog,  be sure to add it – you’ve been missing out on a lot of good stuff).

All done? Ok, I’ll continue.

Reviewing case against default virtual methods

Unwanted Extension Points

Wards "elevator" example demonstrates that we should not make all methods virtual, since it exposes extension points where we do not want them. Let’s have a look at the reason this particular extension point is not wanted.

The elevator.Up() method has responsibilities for moving the elevator up and  closing doors etc. These responsibilities need to be chained in the right order to function safely; we do not wish client code to break the desired behaviour by inadvertently calling Up() at the wrong time.

This seems to highlight a smell that the single responsibility principle is not being adhered to in the elevator class. Since this issue is cause by incorrect ordering of multiple responsibilities. By adhering to SRP, separating out the different behaviours from the responsibility of coordinating them, we can avoid this conflict. We can then keep this method as virtual, and benefit from an additional extension hook safely.

Explicit Contracts

Paraphrasing another point made (I hope Ward will correct me if this is a misrepresentation) is that we may want to be very explicit about the points in which an application can be extended. Marking a method as virtual is a way of explicitly showing your "approved" extension hook.

Arguably, I prefer to see my "extension hooks" more explicitly than a virtual method. As an application developer looking for a "seam" to extend, I would favour seeing an interface that I can implement, over inheriting from a class and overriding it’s virtual methods. This is mainly because to interfaces tend to be more discoverable than virtual methods.

Open/Closed Principle

Lastly, I have a different perspective on the OCP. I believe "closed for modification" to be in reference to the actual LOC contained in the class rather than "closing off" modifications to behaviour. I agree however, that making all methods virtual does open up a greater susceptibility to the violation of LSP.

Summary

The arguments made against methods being virtual as default mostly seem to fit under the umbrella of protecting yourself against unwitting developers. I think there are safer ways to do this whilst still obtaining the flexibility provided when all methods are virtual by default.

Refactoring a static method (on a generic class) to an extension method‏

The Goal

I needed to perform this refactoring today, and it took me a few mins to figure out how to do this in a safe way using resharper. My goal was to refactor usages like:

Mapper<DestinationType>.Map(Source)

to use an extension method so I can use the syntax similar to:

Source.MapTo<DestinationType>();

which I find to be less verbose, more discoverable, and more readable. This however was slightly more difficult than I hoped; resharper didn’t let me do this in a single step (how lazy of me).

The Problem

The original code* looked similar in structure to this:

public abstract class Mapper<TDestination>

{

   public static TDestination Map(object source)

   {

      //do mapping here

      return destination;

   }

}

Since Mapper is not a static class, we cannot use resharpers built in “convert static to extension method” refactoring straight up; extension methods can only live in a static class. We could move this method to a static class, say MapperExtensions, such that we can perform “convert static to extension method”, but to do this we also require the generic type parameter TDestination that lives on the Mapper class. There isn’t a refactoring (that I am aware of/could find with a quick google) to add a generic type parameter to a method signature and adding this in by hand would break all my code that invokes this method. We cannot make the Mapper class static either, since it is an abstract class. This was looking like it could be a bit of a pain.

*The example (mapping) is actually contrived – I’ve made up a example here that is representative of the structure of the problem, rather than use the actual code. This was to avoid breaching the IP rules of the company I work for. In an actual mapper, the Map method is more likely to be an instance method rather than static, and since Mapping it is the primary responsibility of the class, it is not advisable for the Map method to be implemented as an extension method.

The Solution

Step 1: Extract Method

First of all, we can use the extract method refactoring to extract the contents of the original method into another method. We can then use this newly created method as a base to make a any further changes (like adding the generic type parameter) without breaking anything that invokes our original code. After this step, we have:

public abstract class Mapper<TDestination>

{

   public static TDestination Map(object source)

   {

      return MapTo<TDestination>(source);

   }

   public static TDestination MapTo<TDestination>(object source)

   {

      //do mapping here

      return destination;

   }

}

Step 2: Move Method

Since we now have a static method with the required generic type parameter, we can now use the “Move Method” refactoring to move MapTo to its new home, the static class MapperExtensions.

Step 3: Convert static to extension method

We’re now all set to use this refactoring – and we’re nearly there!

Step 4: inline method

We can now make the original call to Map an inline method. This will essentially replace all existing calls from using Map to use the MapTo extension method.

The End Result

The finished article now looks like this:

public static class MapperExtensions

{

   public static TDestination MapTo<TDestination>(this object source)

   {

      //do mapping here

      return destination;

   }

}

Now we’re talking!