Mark Needham

On a few projects that I've worked on one of the things that we've done is create a shared library of objects which can be used across several different projects and while at the time it seemed like a good idea, in hindsight I'm not sure if it's an entirely successful strategy.

I'm quite a fan of not recreating effort which is generally the goal when trying to pull out common code and within one team this seems to be a good approach the majority of the time.

When it comes to sharing across teams then I think we need to consider the perceived benefits a bit more because it doesn't come without costs.

These are some of the types of code that we've shared previously:

Domain objects

I think this is the most dangerous type of code to share because although we often do have the same domain concepts in different projects, it's quite rare that they mean exactly the same thing.

In addition there is an implicit coupling created with our database since we pretty much now have to make sure that our database schema matches up with the current version of that domain object.

Either that or we do have a shared database for all the applications which use that shared domain object in which case we have an even stronger coupling between applications.

We're assuming that the two application have exactly the same domain concept and from my experience quite often that isn't the case – even if there is a concept with the same name it may be used in different ways or mean something completely different in different applications.

This is quite similar to the problem with having a universal domain model which Dan points out in his classic SOA article.

In general I don't think it makes sense to share this type of code.

Test code

This one seems like it should fairly universally a good idea – after all we often import 3rd party testing libraries so it seems like just sharing some common testing code shouldn't be much different.

One piece of code that we shared was the Selenium bootstrapping code and this approach worked reasonably well until we wanted to adjust the amount of time between each command because commands were being sent to the browser before elements had the chance to load.

Apart from the fact that the other users of the library didn't want anything change with respect to how they used the code we had to go and make the change in another project, build that and then update the reference that we had to the library.

Certainly this process would have been made easier if we'd used something like Ivy but the amount of duplication of code that we were saving didn't seem worth the hassle it caused so we ended up inlining the code.

Infrastructure code

General infrastructure code e.g. code to handle NHibernate transactions which is quite unlikely to change seems one candidate which can work quite well in a shared library and so far I haven't seen many problems arise from doing this.

I think the key with these bits of reusable code is that we keep them quite small and ensure that they have only one responsibility which will be useful for all the applications.

We eventually ended up slimming down our shared library and the majority of the code that remains in there is solving specific infrastructure type problems which will be the same across any applications using the same technical stack.

Things to be careful about when sharing code

One reason that we may share code is so that if there is a change then it only needs to be done in one place.

We need to have a degree of confident that if we put code in a shared library that this is actually the case.

If it's likely that different applications might need shared code to change in different ways then we might not want to make that bit of code shared otherwise we'll just end up with application specific code in a shared library.

From what I've noticed it makes most sense to put code which is unlikely to change and is generic enough to be useful across several applications as is into shared libraries.

For any other code it might actually be beneficial to accept that there will be some duplication between applications in the same organisation and not try and pull out a common piece.

I wrote previously about how I've been using effect sketches to identify responsibilities in objects so that I can pull them out into other objects and once I've done this I often find that I can't see a small next step to take.

At this stage in the past I've often then stopped and left the refactoring until I have more time to complete it but this hasn't really worked and a lot of the time I end up only seeing the code change in my mind and not in the actual code.

I came across this problem again last week in an object which had 8 dependencies when I came across it.

Having drawn the effect sketch I realised that 3 of those could be pulled out into a new object which could then be injected into the original object and help to encapsulate those other 3 dependencies.

The code that I wanted to change was something like this:

public class TheObject 
{
	private readonly DependencyA;
	private readonly DependencyB;
	private readonly DependencyB;
	...	
 
	public Foo FooCreation()
	{
		var dependencyAValue = dependencyA.GetSomething();
		var dependencyBValue = dependencyB.GetSomething();
		var dependencyCValue = dependencyC.GetSomething();
 
		return new Foo(dependencyAValue, dependencyBValue, dependencyCValue);
	}
 
	...
}

I wanted to pull the 'FooCreation' method out into another object and then change all the places that were calling 'TheObject'FooCreation' to just call this new object directly.

The first step was to create a 'FooFactory' and just have that delegated to internally:

public class FooFactory 
{
	private readonly DependencyA;
	private readonly DependencyB;
	private readonly DependencyB;
	...	
 
	public Foo Create()
	{
		var dependencyAValue = dependencyA.GetSomething();
		var dependencyBValue = dependencyB.GetSomething();
		var dependencyCValue = dependencyC.GetSomething();
 
		return new Foo(dependencyAValue, dependencyBValue, dependencyCValue);
	}
 
}

public class TheObject 
{
	...	
 
	public Foo FooCreation()
	{
		return new FooFactory(dependencyA, dependencyB, dependencyC).Create();
	}
 
	...
}

I ran out of time at this stage to finish off the refactoring but it was obvious where the refactoring was going so the next time I got the chance I injected the 'FooFactory' into 'TheObject':

public interface IFooFactory 
{
	Foo Create();
}

public class TheObject 
{
	private readonly IFooFactory;
	...	
	public TheObject(IFooFactory fooFactory) 
	{
		this.fooFactory = fooFactory;
	}
 
	public Foo FooCreation()
	{
		return fooFactory.Create();
	}
 
	...
}

To do this I had to go and change the tests on 'TheObject' and move some of them to go directly against 'FooFactory'.

The third stage of the refactoring was to change all the places which called 'TheObject.FooCreation()' to just call 'FooFactory.Create()' directly.
Some of those places were also using other methods on 'TheObject' so those objects now have an extra dependency although I think at least the code is more intention revealing than it was previously.

I'm sure there are some other patterns for this type of small step refactoring but this is just one that I've noticed so far.

I've written previously about how useful I find effect sketches for helping me to understand how an object's methods and fields fit together and while drawing one a couple of weeks ago I noticed that it's actually quite useful for seeing which parts of the code will be the easiest to change.

I was fairly sure one of the object's in our code base was doing too many things due to the fact that it had a lot of dependencies.

However, it wasn't obvious to me from looking at the code which would be the easiest place to start in pulling out some of those responsibilities.

I therefore drew out an effect sketch which looked something like this:

From the diagram I could see more clearly that 'MethodC' is using 3 fields which are not used by any of the other methods in the object.

This therefore seemed like the perfect method to pull out since I could do so really easily and get rid of 3 of the object's fields since noone else used them anyway.

This reminded me a lot of the Mikado method for addressing technical debt which I've read about but haven't used yet.

As I understand it, the goal with the Mikado method is to locate areas of the code base that we can change easily because there are no dependencies on this piece of code.

When using effect sketches the goal is to try and use the sketch to work out how we can group functionality and I think the idea of making initial changes that have a low impact is a good one to follow.

I drew my initial effect sketch on paper but I noticed that drawing it up in graphviz actually makes it even more obvious which bits of functionality are related.

For example I hadn't realised that 'fieldB' was used by so many methods until I typed this up.

It's quite a neat tool and easy to pick up. This is my 'dot' file for the above sketch:

blog.dot

digraph effectgraph {
	size="8,8"; 
 
	"MethodA" -> "fieldA";
	"MethodA" -> "fieldB";
	"MethodA" -> "fieldC";
 
	"MethodB" -> "fieldB";
	"MethodB" -> "fieldE";
	"MethodB" -> "fieldF";
 
	"MethodC" -> "fieldG";
	"MethodC" -> "fieldH"
	"MethodC" -> "fieldD"
 
	"MethodD" -> "fieldB"
	"MethodD" -> "fieldI"
}

And to generate a png I ran the following command from the terminal:

dot -Tpng -blog.png blog.dot

A while ago I wrote about some rules of thumb that I'd been taught by my colleagues with respect to software development and I was reminded of one of them – don't put anything in the session – during a presentation my colleague Luca Grulla gave at our client on scaling applications by making use of the infrastructure of the web.

The problem with putting state in the session is that it means that requests from a specific user have to be tied to a specific server i.e. we have to use a sticky session/session affinity.

This reduces our ability to scale our system horizontally (scale out) i.e. by adding more servers to handle requests.

If, for example, we have a small amount of users (whose first request went to the same server) making a lot of requests (perhaps through AJAX calls) then we may quickly put one of our servers under load while the others are sitting there idle.

In addition we have increased complexity around our deployment process.

If we want to do an incremental deployment of a new version of our website across some of our servers then we need to ensure that we create a copy of any sessions on those servers and copy them to the ones we're not updating so that any users still on the system don't experience loss of data.

There are no doubts products which can allow us to do this more easily but it seems to me to be an unnecessary product in the first place since we can just design our application to not rely on the session.

As I understand it the web was designed to be stateless i.e. each request is independent and all the information is contained within that request and the idea of the session was only something which was added in later on.

How does the way we code change if we don't use the session?

One thing we've often used the session for on projects that I've worked on is to store the current state of a form that the user is filling in.

When they've completed the form then we would probably store some representation of what they've entered in a database.

If we don't use the session then we need to store this intermediate data somewhere and include a key to load it in the request.

On the project I'm working on at the moment we're storing that data in a database but then clearing out that data every other day since it's not needed once the user has completed the form.

An alternative perhaps could be to store it in a cache since in reality all we have is a key/value pair which we need to keep for a relatively short amount of time.

Advantages/disadvantages of this approach

The disadvantage of this approach is that we have to make more reads and writes to the database to deal with this temporary data.

Apart from the advantages I outlined initially, we are also more protected if a server handling a user's request goes down.

If we were using the session to store intermediate state then that information would be lost and they would have to start over.

In the approach we've using this isn't a problem and when the request is sent to another server we can still query the database and get whatever data the user had already saved.

As with most things there's a trade off to be made but in this case it seems a fair one to me.

Alternative approaches

I've come across some alternative approaches where we avoid using the session but don't store intermediate state in a database.

One way is to store that state in hidden fields on the form and another is to send it in the request parameters.

Neither of these approaches seem particularly clean to me and they give the user an easier way to change the intermediate data in ways that the form might not allow them to do.

From my experience our server side code becomes more complicated since we're always writing all of the data entered so far back into the page.

In addition the url becomes a complete mess with the second approach.

I came across an interesting post that Roy Osherove wrote a few months ago where he talks about 'Willed vs Forced Designs' and some common arguments that people give for not using TypeMock on their projects.

I'm not really a fan of the TypeMock approach to dealing with dependencies in tests because it seems to avoid the fact that the code is probably bad in the first place if we have to resort to using some of the approaches it encourages.

Having said that Roy makes the following point which I think is quite accurate:

You let an automated tool (rhino mocks, Moq etc..) tell you when your design is OK or not. That point alone should go against anything ALT.NET has ever stood for, doesn’t it? If you need a tool to tell you what is good or bad design, then you are doing it wrong.

While it is true that it's useful to be able to know for ourselves whether our code is drifting into territory where it's become way too complicated, I think it is useful to have the tests as a reminder that this is becoming the case.

It's quite easy when you have a delivery deadline and are under pressure to stop being as observant about the quality of what you're coding and to rush to complete our particular task.

In these situations it can be useful to be restricted by our framework to the extent that the pain we'll feel in trying to test our code will act as an indicator that we're doing something wrong.

What I found interesting when reading Roy's post is that the arguments sound sounds quite similar to the discussion a couple of years ago with respect to whether using Mockito instead of jMock was bad because it hides design problems that you have with dependencies. Steve Freeman wrote the following comment on Dan's post:

But, it also became clear that he wrote Mockito to address some weak design and coding habits in his project and that the existing mocking frameworks were doing exactly their job of forcing them into the open. How a team should respond to that feedback is an interesting question.

In the meantime, I’ve found that I /can/ teach using the existing frameworks if I concentrate on what they were intended for: focusing on the relationships between collaborating objects. I’ve seen quite a few students light up when they get the point. In fact, the syntactic noise in jMock really helps to bring this out, whereas it’s easy for it to get lost with easymock and mockito.

In this case I definitely prefer the style of mocking that we get with Mockito over jMock even though I've worked on code bases where we've created objects with way too many dependencies and haven't felt the pain as much because the framework is so easy to use.

I can't think of a compelling argument for why this is different to the TypeMock vs other mocking frameworks argument. It seems to be a similar argument around dependencies in our code.

The other thing I'm intrigued about is whether the choice of framework should be in some way linked to the level of skill of the people who are going to use it.

If someone is a Dreyfus Model novice with respect to object oriented design then it would make much more sense to use a tool which makes it really obvious that they're doing something wrong. In that case using a perhaps more limited tool would just be a quick feedback mechanism.

Once we have a bit more skill then it would seem more appropriate to use the more powerful tool which we have the ability to abuse but hopefully now have the experience to know when we can and cannot get away with doing so.

In the end the argument seems quite similar to ones I've often heard about programming in Ruby and whether or not we should give programmers powerful language features because they're liable to hang themselves.

In conclusion I'm thinking that perhaps TypeMock in experienced hands isn't such a bad thing and could actually be useful in some select situations but would probably be quite a dangerous tool for someone new to the whole unit testing game.

One of the things which Nat Pryce and Steve Freeman suggest in their book Growing Object Oriented Software guided by tests is the idea of wrapping any third party libraries that we use in our own code.

We came across a situation where we did this and then later on I made the mistake of not following this advice.

To start with my colleague David had created a DSL which kept all the calls to Selenium nicely wrapped inside one class.

The problem we were experiencing was that we hadn't evolved the DSL with the webpage evolution to the point where we weren't taking into account that some fields on the page weren't visible until ones before them had been filled in.

We needed to change the DSL slightly and it seemed like an interesting opportunity to try and convert them to use Webdriver as it seems more suited for heavy filling in of forms which is our use case.

My first thought was that it should just be possible to change those Selenium calls to call the equivalent Webdriver methods instead but having done that we realised that the way the two tools interact with the page is slightly different so the direct replacement approach wasn't really working.

We decided to adopt a different approach whereby we would just try and change individual tests to use Webdriver instead and leave all the other tests as they are using Selenium.

I thought about creating another version of the DSL to encapsulate the Webdriver interaction with the page but decided against the idea as it didn't seem like it would add much value and the only way I thought of at the time was to create a clone of the original DSL.

We managed to get one of our tests working more effectively using Webdriver having sorted out the problems with the different interactions between fields but unfortunately the current C# API doesn't seem that stable and seems to fail somewhat randomly for reasons we haven't been able to work out yet.

As a result we now want to convert those tests I'd rewritten to take advantage of the new way they're written but to use Selenium instead!

Sadly the approach I took has made this really difficult and it's now a very frustrating journey to get the tests back into shape.

It's quite frustrating to make this type of mistake especially when I read about a solution so recently.

In hindsight I think a better approach may have been to pull our an interface to represent our DSL – currently it's a series of method calls on a few classes – and then create Webdriver and Selenium specific versions of that.

A few of things stand out for me from this experience:

• I talked myself out of wrapping Webdriver because I saw the main reason for doing so being to shield us in case we chose to change the library and I didn't anticipate having to do that. As it is I was wrong but I didn't totally appreciate how we can benefit from defining an API that defines how we want to interact with the web page rather than how the library wants us to.
• We need to evolve our DSLs with the application and not be afraid to change them if the application changes.
• It probably wasn't a good idea to try and fix the DSL and change the underlying library at the same time. Small steps!

The collecting parameter pattern is one of my favourite ones when used well but I've noticed recently that it can lead to quit misleading APIs as well.

One way that we used it quite effectively was when getting objects to render themselves to a ViewData container which was then used to populate the view.

public class Micro {
	private string micro;
 
	public Micro(string micro) {
		this.micro = micro;
	}
 
	public void renderTo(ViewData viewData) {
		viewData.add(micro);
	}
}

I think it's quite obvious from the method name what it does and we can easily test this method by checking on the contents of the value passed in after the method's been executed.

The problem I've noticed with this pattern is when the method isn't so explicit about what it's doing or where a method is using a collecting parameter as well as returning a value.

For example:

public SomeModel GetModel(ViewData viewData) 
{
	viewdata["someKey"] = "someValue;
	// do some other stuff
 
	return new SomeModel(...); 
}

It's not entirely obvious from reading the method signature that it would be mutating the 'ViewData' that's been passed in. I would actually expect that 'SomeModel' is being constructed from 'ViewData' if I just read the method signature in isolation. It's almost like it's following the collecting parameter pattern by accident.

In a way I find that API as misleading as ones which make use of an 'out' parameter to allow multiple values to be returned – it doesn't do what you expect, its intent is not clear.

Since I've started playing around with functional programming languages I've become more keen on the idea that functions take in inputs and return outputs and this pattern goes against that idea by encouraging mutation of state.

It seems to me that for the most part writing functions which return a value and don't do anything else reveal their intent more and for the less frequent occasions where we want to keep encapsulation but still find out some data about an object the collecting parameter pattern works pretty well. It should be used sparingly though.

Something which I've noticed quite a lot on the projects that I've worked on since C# 3.0 was released is that lists seem to be passed around code much more and have LINQ style filters and transformations performed on them while failing to describe the underlying abstraction explcitly in the code.

As a result of this we quite frequently we end up with this code being in multiple places and since it's usually not very much code the repetition goes unnoticed more than other types of duplication might do.

A typical example of this might be the following:

public class SomeFooHolder
{
	public List<Foo> Foos { get; set }
}

An example of how this might be used is like so:

var someFooHolder = new FooHolder(...);
someFooHolder.Foos.Select(f => f.Completed);

That code would typically be repeated in other places in the code where we want to get all the completed foos.

Although it's a simple change, as a first step I prefer to make that concept more explicit by putting 'CompletedFoos' on 'SomeFooHolder':

public class SomeFooHolder
{
	public List<Foo> Foos { get; set; }
	public List<Foo> CompletedFoos 
	{ 
		get { return Foos.Select(f => f.Completed); }
	}
}

Perhaps an even better solution would be to create an object 'Foos' to encapsulate that logic further:

public class Foos
{
	private readonly List<Foo> foos;
 
	public Foos(List<Foo> foos)
	{
		this.foos = new List<Foo>(foos.AsReadOnly());
	}
 
	public Foos Completed
	{
		get { return new Foos(foos.Select(f => f.Completed)); }
	}
}

As I've written about previously I prefer to wrap the list rather than extend it as the API of 'Foos' is more expressive since we don't have all the list operations available to any potential users of the class.

We were working on some refactoring today where we pushed some logic back from a service and onto a domain object and I noticed that we were able to use functions quite effectively to reduce the amount of code we had to write while still describing differences in behaviour.

The class we want to write needs to take in two integers which represent two different situations related to Foo. Depending upon whether we have 'Situation 1′, 'Situation 2′ or both situations we will display the results slightly differently.

    public class Foo
    {
        private readonly int situationOneValue;
        private readonly int situationTwoValue;
        private readonly Func<int, string> situationOneDisplayFunc;
        private readonly Func<int, string> situationTwoDisplayFunc;
 
        private Foo(int situationOneValue, int situationTwoValue, Func<int, string> situationOneDisplayFunc, Func<int, string> situationTwoDisplayFunc)
        {
            this.situationOneValue = situationOneValue;
            this.situationTwoValue = situationTwoValue;
            this.situationOneDisplayFunc = situationOneDisplayFunc;
            this.situationTwoDisplayFunc = situationTwoDisplayFunc;
        }
 
        public static Foo ForSituation1(int situationOneValue, int situationTwoValue)
        {
            return new Foo(situationOneValue, situationTwoValue, Format, _ => "Irrelevant value");
        }
 
        public static Foo ForSituation2(int situationOneValue, int situationTwoValue)
        {
            return new Foo(situationOneValue, situationTwoValue, _ => "Irrelevant value", Format);
        }
 
        public static Foo ForSituation1And2(int situationOneValue, int situationTwoValue)
        {
            return new Foo(situationOneValue, situationTwoValue, Format, Format);
        }
 
        public string Situation1()
        {
            return situationOneDisplayFunc(situationOneValue);
        }
 
        public string Situation2()
        {
            return situationTwoDisplayFunc(situationTwoValue);
        }
 
        private string Format(int value)
        {
            return string.Format("Formatted Value {0}");
        }
    }

The way that it's used is that we get a value coming in from another system as a string which represents which situation we have to deal with.

As a result in the code which processes this data we have a dictionary which maps from these strings to the corresponding static method defined above:

private static Dictionary<string, Func<int, int, Foo>> Foos = 
    new Dictionary<string, Func<int, int, Foo>> 
    { { "Situation 1", Foo.ForSituation1 }, 
      { "Situation 2", Foo.ForSituation2 }.
      { "Situation 1 and 2", Foo.ForSituation1And2 };

This is a neat little idea which I learnt from some Scala code that my colleague John Hume posted on an internal mailing list a few weeks ago where he'd done something similar.

In our code which parses the incoming data we have something like the following:

var foo = Foos[incomingMessage.Situation](incomingMessage.Situation1Value, incomingMessage.Situation2Value);
// and so on

If we wanted to solve this without using functions then for the first part I think we would probably need to use inheritance to create three different Foos, probably with a common interface, and then define the different behaviour on those classes.

The second part of the solution might end up being solved with some if statements or something similar.

I was a bit unsure of how easy it was to understand this code when we first came up with it but it's growing on me.

I've written previously about the test builder pattern in C# and having noticed some different implementations of this pattern I thought it'd be interesting to post my current thinking on how to use it.

One thing I've noticed is that we often end up just creating methods which effectively act as setters rather than easing the construction of an object.

This seems to happen most commonly when the value we want to set is a boolean value. The following is quite typical:

public CarBuilder WithSomething(boolean value)
{
	this.something = value;
	return this;
}

The usage would be like so:

new CarBuilder().WithSomething(true).Build();

It doesn't read too badly but it seems to be unnecessary typing for the user of the API. If we're going to use the fluent interface approach then I would prefer to have two methods, defined like so:

public CarBuilder WithSomething()
{
	this.something = true;
	return this;
}
 
public CarBuilder WithoutSomething()
{
	this.something = false;
	return this;
}

We could then use this like so:

new CarBuilder().WithSomething().Build();
...
new CarBuilder().WithoutSomething().Build();

That requires more code but I think it's a bit more expressive and makes life easier for the user of the API.

An alternative approach which my colleague Lu Ning showed me and which I think is actually better is to make use of the object initializer syntax if all we have are setter methods on a builder.

We might therefore end up with something like this:

public class FooBuilder 
{
	public string Something = "DefaultSomething";
	public boolean SomethingElse = false;
 
	public Foo Build()
	{
		return new Foo(Something, SomethingElse);
	}
}

new FooBuilder { SomethingElse = true }.Build();

With this approach we end up writing less code and although we use public fields on the builder I don't think it's a big deal since it allows us to achieve our goal more quickly. If we need other methods that take out the complexity of construction then we can easily just add those as well.

Another thing to note when using this pattern is that we don't need to override all the object's attributes on every single test. We only need to override those ones which we are using in our test. The rest of the values can just be defaulted.

[Test]
public void ShouldDoSomething()
{
	var foo = new FooBuilder { Bar = "myBar", Baz = "myBaz", SomethingElse = "mySE", AndAgain = "..." }.Build();
 
 
	// and then further on we only check the value of Bar for example
 
	Assert.That(someValue, Is.EqualTo("myBar"); 
}

We don't need to specifically set any of the values except 'Bar' because they are irrelevant and create a clutter in the test which means it takes longer to understand what's going on.

This would be preferable:

[Test]
public void ShouldDoSomething()
{
	var foo = new FooBuilder { Bar = "myBar" }.Build();
 
 
	// and then further on we only check the value of Bar for example
 
	Assert.That(someValue, Is.EqualTo("myBar"); 
}

Something which I've been wondering about recently is understanding the best way to describe the case where we don't want to define a specific value i.e. we want it as null.

I'd normally just set it to null like so:

new FooBuilder { Bar = null }.Build();

But we could make the API have a specific method and I haven't really decided whether there's much value to doing so yet:

new FooBuilder().WithNoBar().Build();