Mark Needham

Thoughts on Software Development

Archive for September, 2009

TDD: Testing with generic abstract classes

with 8 comments

In a post I wrote earlier in the week I described a dilemma we were having testing some code which made use of abstract classes and Perryn Fowler, Liz Keogh and Pat Maddox pointed out that a useful approach for this problem would be to make use of an abstract test class.

The idea here is that we create an equivalent hierarchy to our production code for our tests which in the example that I provided would mean that we have roughly the following setup:

public abstract class ParentModelTest
{
}
 
public class BusinessProcess1ModelTest : ParentModelTest { }
public class BusinessProcess2ModelTest : ParentModelTest { }
public class BusinessProcess3ModelTest : ParentModelTest { }

We then want to create an abstract method on the ‘ParentModelTest’ class to create the object under test and we’ll implement this method in the sub classes.

We can then put the common tests into the abstract class and they will be run when we run the tests for each of the sub class tests.

My colleague Matt Dunn and I were looking at how you would implement this and he pointed out that it provided quite a nice opportunity to use generics to achieve our goal.

If we make the abstract test class take in generic parameters then it allows us to create our specific type in the ‘create’ method rather than having to create ‘ParentModel’ and then casting that for our specific sub class tests.

public abstract class ParentModelTest<T> where T : ParentModel
{
	protected abstract T CreateModel();	
 
	[Test]
	public void ShouldTestThoseCommonDependencies()
	{
		var model = CreateModel();
		// Do some awesome testing that's common across all 3 sub classes here
	}
}
[TestFixture]
public class BusinessProcessModel1Test : ParentModelTest<BusinessProcessModel1>
{
	protected override BusinessProcessModel1 CreateModel()
	{
		return new BusinessProcessModel1(...);
	}
 
	[Test]
	public void ShouldDoSomeEquallyOutstandingStuff() 
	{
		var model = CreateModel();
		// and test away
	}
}

It seems to work quite well and we realised that it’s actually a pattern that we had also used to test that our controllers would actually be injected with all their dependencies from our DI container or whether we have forgotten to register some of them.

We have an abstract class similar to this which all our controller tests extend:

public abstract class ContainerTest<T> where T : Controller
{
	[Test]
	public void ShouldHookUpAllDependencies()
	{
		var container = CreateTheContainer();
 
		var controller = container.Resolve<T>();
 
		Assert.IsNotNull(controller);
	}
}
public class AwesomeControllerTest : ContainerTest<AwesomeController> { }

When we haven’t got all the dependencies injected correctly the code will actually blow up on the resolve step so the last line is not strictly necessary but the test seems like it’s not testing anything at first glance without it.

Written by Mark Needham

September 18th, 2009 at 12:40 am

Posted in Testing

Tagged with

Coding: Watch out for mutable code

with 3 comments

I’ve been doing some more work recently on trying to reduce the number of fields in some of our classes and moving any logic related to calculations into the methods that use the logic but managed to break part of our application recently by doing that a bit too casually and not realising that the code I’d inlined was actually being mutated later on.

The code I’d refactored originally looked like this:

public class MutableFactory
{
	public MutableClass BuildMyMutableClass() 
	{
		var mutableClass = new MutableClass() 
		{
			MutableProperty = new MutableProperty() {
				RandomValue = true
			};
		}; 
 
 
		// later on in the code and hidden away in another method
		mutableClass.MutableProperty.RandomValue = false;
	}
}
 
public class MutableClass
{
	public MutableProperty MutableProperty { get; set; }
} 
 
public class MutableProperty
{
	public bool RandomValue { get; set; }
}

After I refactored it this is what it looked like:

public class MutableFactory
{
	public MutableClass BuildMyMutableClass() 
	{
		var mutableClass = new MutableClass();
 
		// later on in the code and hidden away in another method
		mutableClass.MutableProperty.RandomValue = false;
	}
}
 
public class MutableClass
{
	public MutableProperty MutableProperty 
	{ 
		get 
		{ 
			return new Mutable { RandomValue = true }; 
		}
	}
} 
 
public class MutableProperty
{
	public bool RandomValue { get; set; }
}

Originally when we retrieve ‘mutableClass.Mutable.RandomValue’ it has a value of ‘false’ but after the refactoring it always returns ‘true’

As a result of my inlining the logic, whenever that property was called the logic was recalculated and the mutation of the data which had happened earlier on was lost.

Luckily my colleague Matt Dunn spotted that this was what was happening while we were running the code through the debugger but we shouldn’t have even needed to resort to that in the first place!

The mistake I made here was not to check the test coverage around this bit of code before I made the change, something which Michael Feathers emphasises constantly in Working with Legacy Code.

I was under the assumption that the changes I was making were very minimal and wouldn’t actually break anything! In hindsight I should have put a test around this code first and then done the refactoring.

If I’d done that then the mutation would have quickly become obvious to me i.e. the feedback loop would have been significantly quicker than it was.

We do now have tests around the offending code and an additional benefit of working out how to test this functionality was that we noticed how many dependencies the object was actually relying on and have started taking steps to try and solve that problem.

Taking a step back from this situation I’m still of the belief that having setters all over the place is really not a good idea – we lose trust in the code since we don’t know when an object might be mutated and that mutation may happen in ways that we don’t expect.

Maybe we don’t need every object in our system to be immutable but I think we could get away with having much more immutability than we do now, certainly in the code base I’m working on.

Written by Mark Needham

September 16th, 2009 at 11:31 pm

Posted in Coding

Tagged with

Book Club: SOLID Principles (Uncle Bob Martin)

with 3 comments

In our latest technical book club we discussed Uncle Bob Martin’s presentation to the Norwegian Developers Conference on ‘SOLID Design‘.

These principles of object oriented design are also written up on Uncle Bob’s website and are also in his book ‘Agile Principles, Patterns and Practices‘.

I read most of the book a couple of years ago but I don’t always remember all of the principles when I’m coding so it was good to revisit them again.

These are some of my thoughts and our discussion:

  • Something that we’ve noticed on the project I’m working on at the moment with respect to the single responsibility principle is that often classes start off adhering to this principle but as soon as changes come along that single responsibility is pretty much ruined.

    It often seems like a new piece of functionality fits quite nicely onto an existing object but then when we look back on it in retrospect it didn’t really fit that well and then another bit of code that fits even less well is added and an object is carrying out 3 responsibilities before we know it.

    One way to potentially get around this is to write the responsibility of a class at the top of the file so that hopefully people will read that before adding anything new.

    In a recent coding dojo we were reading through some of the Jetlang code and that approach is followed on a lot of the classes which has the added benefit of making the code easier to follow if you’re new to the code base.

  • Uncle Bob talks quite a lot about ensuring that we design flexibility into the areas of the code that are likely to change so that if we do have to change the code then we can do so easily.

    I’ve read this type of advice before but I’m never sure how exactly you know where those areas of change are likely to be unless you’ve worked on a similar system before.

    From what I’ve seen any code which relies on another system’s data/representations needs to be designed in this way as we don’t know when it is going to change and it might even change without much warning so we need to adapt to that situation fairly quickly.

  • He didn’t mention the Interface Segregation Principle in this presentation but I find this one the most interesting, probably because I haven’t seen it followed on any projects I’ve worked on yet and I’m intrigued as what a code base that followed it would be like to work on.

    I like the idea of having role based interfaces although I imagine it would probably be quite easy to abuse this principle and end up with interfaces that are so finely grained that we lose the domain’s meaning.

  • While I have no doubt that if we followed these principles all the time when coding our code base would probably be much cleaner, it feels to me that these principles are quite difficult to remember when you’re coding.

    From what I’ve noticed we find it much easier to follow ideas like Don’t Repeat Yourself, perhaps because it’s easier to see when we are breaking principles like this.

    In addition, most people tend to agree about what makes repetition of code but when it comes to something like the Single Responsibility Principle, for example, people seem to have different opinions of what a responsibility is which makes it difficult to get consensus.

    I quite like the newspaper metaphor to writing code which Uncle Bob describes in Clean Code and he elabroates on this further in a recent post about extracting method until you drop. I find ideas like that easier to follow when coding.

    My current thinking is that the principles are really good for when we’re analysing code having taken a step back but when we’re actually coding they’re not necessarily always the first thing to come to mind, at least for me!

    Perhaps that’s normal but I’m intrigued as to whether more experienced developers than me are able to keep these ideas in mind all the time?

Written by Mark Needham

September 16th, 2009 at 1:11 am

Posted in Book Club

Tagged with

Scala: The ‘_=’ mixed identifier

without comments

I’ve been playing around with Scala a bit and in particular following some of the code examples from Daniel Spiewak’s ‘Scala for Java Refugees’ article on Traits and Types.

One thing that I got a bit confused about in one of the examples was the use of the ‘_’ at the end of one of the function definitions:

class MyContainer[T] {
  private var obj:T = null
 
  def value = obj
  def value_=(v:T) = obj = v
}
 
val cont = new MyContainer[String]
cont.value = "Daniel"
 
println(cont.value)

From my limited understanding of the language the ‘_’ (or placeholder syntax) is often passed to functions when we only want to partially apply the function to its arguments but when you use it in that context there’s usually a space between the function name and the ‘_’ so it wasn’t being used like that.

From Programming in Scala:

Remember that you need to leave a space between the function name and the underscore, because otherwise the compiler will think you are referring to a different symbol, such as for example, a method named println_, which likely does not exist.

In this example we were able to make use of ‘value’ without using the ‘_’ though so clearly this was some other syntax I was unaware of.

I came across the idea that it might be linked to ‘DynamicVariable‘ but I think that it’s just a coincidence that ‘DynamicVariable’ happens to define a function called ‘value_’ since the code described above doesn’t mention ‘DynamicVariable’ anywhere.

Eventually Lars Westergren set me back on track by pointing out that what’s being described is a mixed identifier which is a much simpler explanation than what I’d been thinking!

In this case the mixed identifier is ‘value_=’ which defines an assignment operator which takes in a value ‘v’ of type ‘T’ and then assigns it to ‘obj’.

It seems quite similar to the way we would use properties in C#.

I’m still finding having so many ‘=’ on the same line to be a bit confusing at the moment so I’d probably rewrite that function like this so it’s easier for me to understand:

  def value_=(v:T) = { obj = v }

Channing Walton also linked me to a post which explains how Scala properties work.

Written by Mark Needham

September 14th, 2009 at 11:49 pm

Posted in Scala

Tagged with

TDD: Testing sub classes

with 11 comments

We ran into another interesting testing dilemma while refactoring the view model code which I described in an earlier post to the point where we have an abstract class and three sub classes which means that we now have 3 classes which did the same thing 80% of the time.

As I mentioned in a post a couple of weeks ago one of the main refactorings that we did was to move some calls to dependency methods from the constructor and into properties so that those calls would only be made if necessary.

After we’d done this the code looked a bit like this:

public abstract class ParentModel
{
	private readonly Dependency1 dependency1;
 
	...
 
	public decimal Field1 { get { return dependency1.Calculation1(); } }
	public decimal Field2 { get { return dependency1.Calculation2(); } }
}
 
public class BusinessProcess1Model : ParentModel { }
public class BusinessProcess2Model : ParentModel { }
public class BusinessProcess3Model : ParentModel { }

We wanted to ensure that the tests we had around this code made sure that the correct calls were made to ‘depedency1′ but because ParentModel is an abstract class the only way that we can do this is by testing one of its sub classes.

The question is should we test this behaviour in each of the sub classes and therefore effectively test the same thing three times or do we just test it via one of the sub classes and assume that’s enough?

Neither of the options seems really great although if we cared only about behaviour then we would test each of the sub classes independently and forget that the abstract class even exists for testing purposes.

While the logic behind this argument is quite solid we would end up breaking 3 tests if we needed to refactor our code to call another method on that dependency for example.

I suppose that makes sense in a way since we have actually changed the behaviour of all those classes but it seems to me that we only really need to know from one failing test that we’ve broken something and anything beyond that is a bit wasteful.

In C# it’s not actually possible for ‘Field1′ or ‘Field2′ to be overriden with an alternate implementation unless we defined those properties as ‘virtual’ on the ‘ParentModel’ which we haven’t done.

We could however use the ‘new’ keyword to redefine what those properties do if the callee had a reference directly to the sub class instead of to the abstract class which means it is possible for a call to ‘Field1′ to not call ‘dependency1′ which means that maybe we do need to test each of them individually.

I’m not sure which approach I prefer, neither seems better than the other in my mind.

Written by Mark Needham

September 13th, 2009 at 10:21 pm

Posted in Testing

Tagged with

Coding: An abstract class/ASP.NET MVC dilemma

with 9 comments

I previously described a refactoring that we have been working on to reduce the number of fields and delay calculations and the actual goal behind this refactoring was to get the code into shape so that we could add in the logic for a new business process that our application needed to handle.

The code in question defines view models being used by different partial views which are rendered depending on the business process that the user is currently executing.

We decided that the best way to add in this new code was drive our code towards a stage where we could have an abstract class and then sub classes for each of the specific business processes around this area.

Before this we had been using the same class everywhere but it was becoming a bit too complicated to understand as we attempted to add in this new functionality – there was already scattered if/else logic for the two business processes that were already being handled in the same place.

The views were defined like this, where the generic type on ‘ViewModel’ is the name of the specific business process model class:

ParentPage.aspx : ViewModel
 
BusinessProcess1Partial.aspx : ViewModel
BusinessProcess2Partial.aspx : ViewModel
BusinessProcess3Partial.aspx : ViewModel

The classes in question are roughly like this:

public abstract class ParentModel
{
	private readonly Dependency dependency;
 
	public ParentModel(Dependency depedency)
	{
		this.dependency = dependency;
	}	
 
	public string GetValueOne()
	{
		return dependency1.GetValueOne();
	}
}
 
public class BusinessProcessModel1 : ParentModel
{
	public BusinessProcessModel1(Dependency dependency) : base(dependency) { }
 
	public int SomeBusinessProcess1SpecificThing()
	{
		// do some calculation
	}
}
 
public class BusinessProcessModel2 : ParentModel
{
	public BusinessProcessModel1(Dependency dependency) : base(dependency) { }
 
	public int SomeBusinessProcess2SpecificThing()
	{
		// do some calculation
	}
}

It worked quite well for the majority of what we wanted to do but we eventually got to the stage where there was a property which we needed on the ‘ParentModel’ for the ‘ParentPage’ which was also needed by BusinessProcessModel1 and BusinessProcessModel2 but not by BusinessProcessModel3.

We actually had the data required for that property at the time of construction of all three of the business processes so we decided to add it to the ‘ParentModel’ and even though it seems quite evil to do this we couldn’t see an immediately obvious alternative.

Our initial thought was that perhaps we could make use of some role based interfaces and then define those interfaces as part of the ‘ViewModel’ generic that each page extends. As far as I understand it’s not possible to do this though because any types we use with ‘ViewModel’ need to be classes.

An approach which we considered but didn’t try out is to have a ‘ParentModel’ representing the top level page and then have each of the business proceess models as propertys on that.

By doing that we could reduce the need to have the class hierarchy although we would increase the complexity of the code for rendering the business process specific partials since we’d need to introduce if statements into the parent view to ensure that the correct property from that was selected.

Neither option seems that great. Has anyone found a better solution?

Written by Mark Needham

September 13th, 2009 at 12:19 am

Posted in .NET,Coding

Tagged with

TDD: Test only constructors

with 11 comments

I wrote previously how we’d been doing some work to change the way that we get a ‘User’ object into our system and one mistake that we made intially was to have another constructor on the ‘User’ object which was being used in all our unit tests which involved the user in some way.

The original reason that this ‘test constructor’ was created was to make it easier to construct a ‘fake user’ which we were using in some of our functional tests but had ended up being used in unit tests as well.

The constructor being exposed for testing was pretty much the same as the private constructor that we have now.

public class User
{
	...
	public User(string userName, string customerId)
	{
		this.userName = userName;
		this.customerId = customerId;
	}
}

The problem with this approach to testing is that we aren’t actually testing the code which we’re using in production so even if the tests pass it doesn’t actually tell us very much about our code.

We could be doing everything right in this test constructor and doing something crazy in the static factory method and we wouldn’t find out until much later on.

The interesting thing is that the method that we call from the production code isn’t as testing friendly as the one we had made public just for testing:

	public static User CreateUserFrom(NameValueCollection headers)
	{
		var userName = headers["user-name-header-tag"];
		var customerId = headers["customer-id-header-tag"];
 
		// and so on
 
		return new User(userName, customerId);
	}

In order to make our test setup code use this method we had to create a NameValueCollection containing key/value pairs with the appropriate keys that reside in the headers of requests coming into our application.

We therefore end up with code similar to this in the test data builder:

public class UserBuilder
{
	...
 
	public User Build()
	{
		var headers = new NameValueCollection();
		headers.Add("user-name-header-tag", "randomName");
		headers.Add("customer-id-header-tag", "customerId");
		User.CreateUserFor(headers);
	}
}

This leaks a bit of the implementation of ‘CreateUserFrom’ into the tests but I prefer this to testing something which is never actually used.

Written by Mark Needham

September 12th, 2009 at 12:35 am

Posted in Testing

Tagged with

Impersonators: Using them in showcases

with one comment

Towards the end of my colleague Julio Maia’s blog post about the impersonator pattern he suggests that the standalone environment that we can create through the use of impersonators can be quite useful for showcases and we actually had a recent occasion where we had to switch mid-showcase from using the integration environment to make use of an impersonator.

In this case part of the environment went down in the middle of the showcase so if we wanted to keep on going then that was our only option but in general the expectation of the business is that our showcases show them the functionality of the application end to end.

We need to be careful when using a standalone environment that the business are aware of this because it’s quite likely that the application is going to respond much more quickly than it would if it was fully integrated.

If people aren’t aware of this then they will be very surprised when the application isn’t quite as performant when we put it into a real environment and it has to retrieve and store real data instead of just getting pre-stored values from an in memory impersonator.

Another disadvantage of using a standalone environment is that we must ensure that we only enter data which we have previously entered otherwise we’ll either go against the real environment if we’re using a self initializing fake or (I think) just get no response if we’re using a record/replay type impersonator.

If we’re letting a business user drive the application during a showcase then we’ll need to ensure that they know which data they can enter.

As long as we’re careful in those regards I think it can be quite a useful approach since we can make our showcase independent of factors that our outside of our control – for example in our most recent showcase we had to reschedule the time because one part of the server was going to be restarted at our initial showcase time.

I definitely like the idea of using the impersonator for the safety it brings although my thinking at the moment is that maybe it’s best to use it as a backup option if the real environments aren’t working.

I’d be interested to hear if others have experiences around this area.

Written by Mark Needham

September 10th, 2009 at 12:23 am

Posted in Testing

Tagged with ,

A reminder that sometimes it’s best just to ask

with one comment

Recently my pair and I were trying to merge some changes into our code that we had just picked up fron updating from the trunk and realised that we weren’t actually sure how to resolve that merge since it seemed to conflict with what we’d been working on.

We hadn’t checked in for longer than we would have liked to due to a bit of a checkin pile up which had happened because the build on the CI server had been failing for a few hours due to a temporary problem we were having with an external dependency.

Another pair were working on some code which was around the same area of the code base as us but for a different feature so we hadn’t been working together too closely.

Usually when faced with this type of problem my default approach would be to go to the Subversion logs and go through each of the changes in their checkin until I could work out what their change was all about and how it fitted in with what we were doing.

On this occasion we didn’t do that but instead called over one of the guys in the pair to ask if they could explain what changes that made in their latest checkin and how we would need to change the code on our machine to work with those changes.

My colleague took us to a whiteboard and sketched out what they were working on and the approach they were taking to achieve that.

It took less than a minute to do this and after that it was really obvious what changes we need to make and in another couple of minutes we’d made those changes and had our code functioning again.

It’s certainly a useful skill to be able to merge code by scanning through the change logs but it’s also useful to remember that software development is generally a team game and that we can get things done so much quicker if we work closely with our team mates.

Written by Mark Needham

September 7th, 2009 at 10:27 pm

Fiddler: Trying to work out how it all hooks together

with 4 comments

I mentioned previously that we’re making use of Fiddler quite a lot on my current project, mainly to check the traffic going to and from the service layer, and I’m quite curious how it actually works.

In particular I wanted to know:

  • How we’re able to route requests through Fiddler and then through the corporate proxy
  • How proxy settings work differently for Firefox and Internet Explorer

As far as I’m aware the source code for Fiddler isn’t available so a colleague and I tracked the various proxy settings when Fiddler was turned on and off and also had a look at some registry settings.

fiddler.png

As Internet Explorer is a WinInet based application it looks at the Local Area Network settings to check whether there are any proxies that it needs to route requests through. I believe any proxy defined in here would be known as the ‘system proxy’.

We noticed that our application also routed its requests via the proxy defined in the LAN settings as well – it is a .NET application but I guess that even applications written in other languages but running on windows would make use of the system proxy too?

Firefox has its own proxy settings which are accessible via the preferences menu of the browser and are then configurable from the ‘Advanced -> Network’ tab.

If another proxy is defined in the LAN settings when Fiddler starts up it takes this value and stores it somewhere (in memory presumably) as the ‘Upstream gateway proxy’ (Tools -> Fiddler Options -> Connections) which it directs requests through before they go out to the internet.

It then changes the proxy defined in the LAN settings to point to itself – typically port 8888 locally.

The ‘Upstream gateway proxy’ always seems to be the same as the ‘system proxy’ so if a different proxy is defined on FireFox than Internet Explorer this won’t be used even though on the latest version of Fiddler when it starts up the FireFox proxy will be changed to point to Fiddler.

When Fiddler is shut down it reverts the proxy settings to how they were previously although (unsurprisingly) if it crashes then we are left in a state where requests are being sent to ‘http://localhost:8888′ and there is no application listening on that port anymore meaning that we end up with a ‘Cannot find server or DNS Error’.

Restarting Fiddler gets rid of that problem although the ‘Upstream gateway proxy’ usually seems to get lost so if we want to make requests out to the internet then we need to go and reset that in our LAN settings before restarting Fiddler.

If there’s anything inaccurate in what I’ve written or if you know of any resources that would help me understand this better if you let me know in the comments that’d be cool!

Written by Mark Needham

September 6th, 2009 at 11:25 pm

Posted in Software Development

Tagged with