Mark Needham

In our latest technical book club we discussed chapters 3,4 and 5 of Michael Feathers’ ‘Working Effectively With Legacy Code‘ – ‘Sensing and Separation’, ‘The Seam Model’ and ‘Tools’.

These are some of my thoughts from our discussion of these chapters:

• Feathers suggests two reasons why we break dependencies when trying to get tests in place – sensing and separation. The former involves the breaking of dependencies in order to get access to the values computed in our code and the latter is necessary so that we can get our code into a test harness to start with.

  In my time coding professionally I have experienced difficulties with the former than the latter. I have certainly written code which is bordering impossible to test but it seems like maybe writing code which is difficult to sense against is less problematic than code which we struggle to get into a test harness.

• I really like the idea of seams and enabling points to describe how we can alter the way that our code works by making changes in other places.

  Most of the enabling points in the code I’ve worked on are object seams but Halvard and I did make use of a link seam when we wanted to override the behaviour of a specific class in a 3rd party library. We were able to do this by including our own class with the same class signature earlier in the application’s class path.

  Ahrum described a time when he had to do something similar to get rid of some noisy warning messages which one of the dependencies was emitting. He was able to verify that nothing important was going on in that particular class before overriding the behaviour with the same trick.

• Dave pointed out that it’s useful to remember that a test is also a client of an object in particular reference to Feathers pointing out that software doesn’t seem to be designed to be easily testable. From what I’ve seen the best way to make code easily testable is to design in that testability when we’re writing it. It’s much more difficult and time consuming to try and do that later on.
• We had some discussion around big tests in terms of the size of test fixtures and individual tests. The consensus seemed to be that when we have an important class where we care a lot about the edge cases then we’ll probably write a large number of tests. On the other hand if our individual test methods are big – which usually means there’s lots of setup required – then it might indicate that a method or class is doing too much.
• The problems in code that these two chapters come generally happen because we are doing too much in single classes instead of separating the responsibilities. Ahrum pointed out that if we had lots of small classes we would have to solve the problem about how to organise these classes effectively instead.

  On the projects I’ve worked on we tend to end up with packages which contain a huge number of classes but I haven’t noticed it as being particularly painful so far. Ahrum suggested an alternative approach to grouping objects by the layer they reside in would be to group them by feature instead. I haven’t done this before but it’d be interesting to see if it would make it easier to manage our code.

I’ve read a bit about role based interfaces but I’ve never really quite understood how the idea could be applied into our code – this week my colleague Matt Dunn has been teaching me.

We had a requirement to show some content on every page of the website we’re working on. The content would be slightly different depending on which business process you’re doing.

Our first solution made use of an already defined ‘BusinessType’ property which allowed us to work out which content we needed to create.

public class ApplicationController : Controller
{
	public string BusinessType
	{
		get { return GetType().Replace("Controller", ""); }
	}
 
	public override void OnActionExecuting(ActionExecutingContext context) 
	{
    		base.OnActionExecuting(context);
		ViewData["SomeViewDataKey"] = CreateThatViewDataStuff();
	}
 
	private ViewDataStuff CreateThatViewDataStuff()
	{
		return new ViewDataStuff 
		{
			Content = BuildContent()
			// and so on
		};
	}
 
	private Content BuildContent()
	{
		if(BusinessType == "BusinessType1")
		{
			return CreateContentForBusinessType1();
		}
		else if(BusinessType == "BusinessType2")
		{
			return CreateContentForBusinessType2();
		}
		else
		{
			return CreateContentForEveryOtherBusinessType();
		}
	}
}

public class BusinessType1Controller : ApplicationController { }
public class BusinessType2Controller : ApplicationController { }

The problem is that it’s really hacky, way too much is going on in the ApplicationController and every time we have a business type which has different content we have to add to the mess.

We decided to make use of polymorphism to make the solution a bit cleaner and my initial thought was that we could just create a ‘CreateContent’ method on the ApplicationController and then override that if necessary in the other controllers.

Matt suggested that we should take this a step further and define that method as part of an interface called ‘IContentFactory’.

With this approach we would then be able to just pass a reference to that interface into the ‘ViewDataStuff’ class which can delegate to the ‘IContentFactory’ when the content is requested.

In this case in the ‘CreateContent’ methods we were making network calls to retrieve data so it allowed us to delay these calls until strictly necessary.

public class ApplicationController : Controller, IContentFactory
{
	public override void OnActionExecuting(ActionExecutingContext context) 
	{
    		base.OnActionExecuting(context);
		ViewData["SomeViewDataKey"] = CreateThatViewDataStuff();
	}
 
	private ViewDataStuff CreateThatViewDataStuff()
	{
		return new ViewDataStuff(this) 
		{
			// and so on
		};
	}
 
	public virtual Content CreateContent()
	{
		return new Content(...);
	}
}

public class BusinessType1Controller : ApplicationController
{
	public override Content CreateContent()
	{
		// creates the Content object slightly differently.
		return new Content(...); 
	}
}

public class ViewDataStuff
{
	private readonly IContentFactory content;
 
	public ViewDataStuff(IContentFactory content)
	{
		this.content = content;
	}
 
	public Content Content 
	{
		get { return content.CreateContent(); }
	}
}

We thought about using Udi Dahan’s naming notation for interfaces but the factory pattern is quite well known so it seemed like we might creation more confusion by doing that.

I think it’d be interesting to see what we’d have come up with if we’d test driven this code rather than refactored it into this pattern.

Overall though it seems quite neat as an approach and the next time we came across some similar code I was able to introduce a role based interface having been taught how to do it by Matt on this one.

A couple of weeks ago I wrote a post about my software development journey over the last year and towards the end I described the difficulties we were having in making changes to some C# code while being sure that we hadn’t broken javascript functionality that also relied on that code.

We typically have code which looks like this:

public class SomeController
{
	public ActionResult SomeControllerAction()
	{
		var someModel = new SomeModel { Property1 = "my Property" };
 
		return new JsonResult { Data = someModel };
	}
}
 
public class SomeModel 
{
	public string Property1 { get; set; }
}

We would make use of this type of object in javascript code like so:

$.getJSON("/SomeController/SomeControllerAction",
        function(data){
			var value = data.Property1;
			// do some cool stuff with that value
		}
);

My colleague Raymond Maung recently came up with the idea of writing tests to ensure that the properties we make use of in Javascript exist on the C# objects which we return in JSON calls.

We now have a testing extension method which uses reflection to check that the expected properties are set.

public static class TestingExtensions 
{
        public static void AssertHasProperty(this Type type, string propertyName)
        {
            var property = type.GetProperty(propertyName);
            Assert.IsNotNull(property, "Expected {0} to have property '{1}' but it didn't", type.Name, propertyName);
        }
}

[Test]
public void ShouldEnsurePropertiesRequiredInJavascriptAreSet()
{
	var type = typeof(SomeModel);
	type.AssertHasProperty("Property1");
	// and so on
}

It still requires the developer to remember to put a test in if they add a property to the C# model but I think it’s working better than having to switch between the javascript and C# files checking that you haven’t broken anything.

We’ve decided to go back to reading a book in our technical book club after a few months of discussing different papers and the chosen book is Michael Feathers’ ‘Working Effectively With Legacy Code‘.

We started off by reading the first two chapters titled ‘Changing Software’ and ‘Working with Feedback’ and these are some of my thoughts and our discussion of the chapters:

• Early on Feathers talks about the need to change software in order to add features and fix bugs and while it is certainly necessary to make some changes to code in order to do this we discussed whether there is ever a time that we might look to keep the number of changes we’re making to a minimum.

  Tom suggested that if we have good enough tests then we shouldn’t be fearful of making changes at any time. I think we’d look to be more careful about making changes around the time of a release because we don’t have a lot of time to recover if we make a mistake. Perhaps that only suggests that we don’t have good enough tests though!

• Something which I’ve noticed recently is that we often end up with transitionary refactorings in our code base which are attempts at refactorings which haven’t quite been completed yet. I think this is somewhat inevitable if we are making incremental changes to improve the quality of the code base.

  The problem with this is that it is sometimes it is not obvious where that refactoring is going and if someone other than the original authors has to work with the code then they can easily drive it in a different direction.

  While we were discussing this it reminded me of an idea we read in ‘An agile approach to a legacy system‘. The authors suggest that whenever there is a big refactoring to make the whole team only works on that until it is completed. It would be interesting to see how well this approach would work with a bigger team.

• I really like the definition of unit tests that Feathers uses – tests that give us ‘localised feedback and ‘run fast‘. I wrote a post last year where I tried to break this down further but I think Feathers’ guideline is useful to keep in mind when writing these tests.

  It’s easy to end up relying on functional tests which undoubtably have their place but don’t provide the rapid feedback that we need to work effectively.

• We also discussed the need to think twice before creating a technical debt card or adding a ‘TODO’ comment to a piece of code. More often than not these just end up being ignored so it makes sense to check whether you can make the change when you see the problem where possible.

  Evan Bottcher wrote a cool comment in a previous post I wrote where he describes his experience with TODO comments:

  I find TODO comments a symptom of this – a colleague of mine once said that he was annoyed by TODOs, that they’re a statement of ‘I can’t be bothered doing this, I want YOU TODO this”.

  It’s certainly not always possible to fix things immediately but my current thinking is it probably makes sense to note that down somewhere that’s not in the code and get back to it as soon as possible. Having said that I was reading Mario Fusco’s entry in ‘97 things every programmer should know‘ earlier and he recommends putting ‘TODO’ comments into the code base to identify areas of code to come back to later. Perhaps it just depends on the team.

• I think the following observation is quite astute:
  

  Breaking down a big class into pieces can be pretty involved work unless you do it a couple of times a week. When you do, it becomes routine. You get better at figuring out what can break and what can’t, and it is much easier to do.

  In addition if we don’t do this type of refactoring then those classes increase in size and it becomes even more difficult to do in future.

  Tom also pointed out that the more we practice the better we become at identifying good and bad code which allows us to better focus our attention on where we need to make improvements.

In my continued attempts to learn a bit of Scala I’ve been trying out the 2nd of Dave Thomas’ code katas – Karate Chop – while using an array slicing approach.

I’ve tried out the iterative approach to this problem in Java about a year ago and it ends up being quite verbose so I thought the array slicing one would be much more concise.

I didn’t drive any of the solutions I worked on from the tests – in fact I only got all the tests provided by Dave Thomas running right at the end which was probably a mistake in retrospect.

My first solution didn’t really work at all – it resulted in an ‘ArrayIndexOutOfBoundsException’ for pretty much any input where the ‘needle’ wasn’t the middle value:

def chop(needle : Int, haystack : Array[Int]) : Int  = {
	val middleIndex = haystack.length / 2
	if(haystack(middleIndex) == needle)
		return middleIndex
	else if(haystack(middleIndex) < needle) 
		return chop(needle, haystack.slice(0, middleIndex))
	else
		return chop(needle, haystack.slice(middleIndex + 1, haystack.length))
}

It turns out that the logic is actually the wrong way around – if we find that ‘haystack(middleIndex)’ is less than ‘needle’ then we want to be looking at the 2nd half of the array and not the first half!

Attempt number two addressed this problem:

def chop(needle : Int, haystack : Array[Int]) : Int = {
	val midpoint = haystack.length/2
	val valueAtMidpoint = haystack(midpoint)
 
	if(valueAtMidpoint == needle) return midpoint
	else if(valueAtMidpoint > needle) return chop(needle, haystack.slice(0, midpoint))
	else return chop(needle, haystack.slice(midpoint, haystack.length))
}

The problem with this solution is that if the ‘needle’ is in the second half of the array then it will end up reporting us the wrong position with respect to the whole array – we will just get the position of the ‘needle’ in the remaining array.

This suggested to me that I would probably need to make use of an inner function to keep track of how many items had been discarded whenever we realised that the item was in the second half of the array.

We could then add that value to the final index position if the ‘needle’ was found in the array:

def chop(needle : Int, haystack : Array[Int]) : Int = {	
	def chopInner(discardedItems : Int, innerHay : Array[Int]) : Int = {	
		val midpoint = innerHay.length/2
		val originalMidpoint = midpoint + discardedItems
		val valueAtMidpoint = innerHay(midpoint)
 
		if(valueAtMidpoint == needle)  
			originalMidpoint
		else if(valueAtMidpoint > needle)  
			chopInner(0, innerHay.slice(0, midpoint))
		else  
			chopInner(originalMidpoint, innerHay.slice(midpoint, innerHay.length))
	}
	chopInner(0, haystack)
}

This solution will find the position of ‘needle’ if that value exists in the ‘haystack’ but if the value isn’t in the list we end up in an infinite recursion and if the list is empty then we end up with an ‘ArrayIndexOutOfBoundsException’ when we try to get the value of the midpoint.

My truly hacky solution which solves these two problems looks like this:

def chop(needle : Int, haystack : Array[Int]) : Int = {	
	def chopInner(discardedItems : Int, innerHay : Array[Int]) : Int = {	
		if(innerHay.length == 0) 	
			return -1
 
		val midpoint = innerHay.length/2
		val originalMidpoint = midpoint + discardedItems
		val valueAtMidpoint = innerHay(midpoint)
 
		if(valueAtMidpoint == needle)  
			originalMidpoint
		else if(valueAtMidpoint > needle)  
			chopInner(0, innerHay.slice(0, midpoint))
		else if(innerHay.length == 1)  
			-1
		else  
			chopInner(originalMidpoint, innerHay.slice(midpoint, innerHay.length))
	}
	chopInner(0, haystack)
}

I converted Dave Thomas’ tests into ‘specs‘ tests using the following regular expression:

Find:

assert_equal\((.*),[ ]+chop\((.),[ ]+\[(.*)\]\)\)

Replace:

chop($2, Array($3)) mustBe $1

I then adapted my colleague Sam Newman’s bigvisiblewall project so that I could make use of the ant script he’s setup which makes specs and ant play nicely together.

All the tests pass but there must be a cleaner solution than what I’ve ended up with – I think another attempt will be required!

I recently came across an interesting post by Dave Thomas where he discussed several domain specific languages (DSLs) he’s come across and suggests that a lot of them seem to be trying too hard to read like the english language instead of focusing on describing a vocabulary for their specific domain

Reading this post reminded me that I fell into this trap earlier in the year while doing some work to create a builder pattern in our code which didn’t need to make use of a ‘Build’ method but instead would make use of C#’s implicit operator to automatically convert the builder to an object at the appropriate moment.

My motivation was to make the code more readable but the code actually ended up being quite misleading for anyone else who came across it.

When someone sees the builder pattern they have an expectation that the method chaining sequence should end with ‘Build()’ so by using the implicit operator I was essentially breaking a well defined DSL for creating new objects.

To add to our problems, we nearly always use ‘var’ to describe local variables in our code base so there we times when you couldn’t tell if what you had was a ‘Foo’ or a ‘FooBuilder’.

Quite frequently the implicit conversion only happens when the variable is passed into another method or construct which explicitly defines which value is needed.

[Test]
public void SomeRandomTest()
{	
	var foo = new FooBuilder().Foo("someValue");
 
	var someFoos = new Foo[] { foo }; 
}

In this example ‘foo’ is only implicitly converted to the type ‘Foo’ when it is put into the array and its actual type is ‘FooBuilder’.

In terms of what I was trying to achieve I created a piece of code that was easier to read but more difficult to understand which means that it’s not intention revealing as my colleague Sai Venkatakrishnan pointed out.

It was an interesting experiment but I think in the future I’ll stick with the more obvious approach which is a bit more verbose but a bit more intention revealing as well.

A colleague and I were working on some code a couple of weeks ago which mostly revolved around investigating the C# reflection API to work out which methods we needed to use.

My colleague was driving while we were doing this and our progress seemed very much based on intuition about the API rather than being gradual.

In fact it was quite similar to one of the situations in which Uncle Bob suggests TDD doesn’t work so well:

…
I need the freedom to fiddle around with the formatting and the structure until everything is just the way I want it. Trying to do that fiddling with TDD is futile. Once I have the page the way I like it, then I’ll write some tests that make sure the templates work as written.

My colleague was fiddling around with the code trying to work out what combination of methods we needed to call and I was struggling a bit to get involved through no fault of my colleague.

He was describing what he was doing but for a lot of the time what we tried didn’t work and then suddenly it worked.

It all seemed a bit magical to me!

At this stage we decided to refactor the code and thought that it would might be useful for me to do that refactoring so that I could explain my understanding of the code my colleague had just written and see if I had followed what he’d been doing.

I was able to extract out methods correctly most of the time but there were a couple of places where I hadn’t quite understood what was happening so this approach allowed my colleague to explain it to me.

My initial thinking when we started doing this coding was that perhaps it wasn’t suitable for pairing because it’s quite exploratory but it seemed to work out reasonably well and my understanding of reflection is now slightly better thanks to my colleague’s explanations.

Something which I noticed was a problem with the first example test that I provided in my post about API readability and testability is that the assertion we are making is not that great.

[Test]
public void ShouldConstructModelForSomeSituation()
{
	Assert.AreEqual(DateTime.Today.ToDisplayFormat(), model.SomeDate()); 
}

It’s not really obvious what the expected result is supposed to be except that it should be the ‘DisplayFormat’. If that fails then we’ll need to navigate to the ‘ToDisplayFormat’ method to work out what that method does.

I think it should be possible to immediately know why a test failed so that we can address the problem straight away without too much investigation.

In this example we changed the way the code was working which coincidentally allowed us to make the assertion more obvious.

[Test]
public void ShouldConstructModelForSomeSituation()
{
	Assert.AreEqual("10 Oct 2009", model.SomeDate()); 
}

Erik pointed out another example of this a few weeks ago while we were working on some HTML helper code.

The tests in question looked roughly like this:

[Test]
public void ShouldConstructReadOnlyValue()
{
	var readOnlyValue = new HtmlHelper().ReadOnlyValue("someId", "someValue", 'someTextValue');
 
	...
	var expectedValue = new TestHtmlBuilder().AddLabel("someId", "someTextValue").AddHiddenField("someId", "someValue").Build();
 
	Assert.AreEqual(expectedValue, readOnlyValue);
}

The test reads reasonably nicely and it’s fairly obvious what it is we’re testing for.

The problem here is that we’ve hidden away our expectation and in this case we actually found out that the ‘TestHtmlBuilder’ had extra spaces in some places so all our assertions were incorrect and we didn’t even know!

In addition we end up duplicating the logic that the ‘HtmlHelper’ is doing if we create test assertion helpers like these.

[Test]
public void ShouldConstructReadOnlyValue()
{
	var readOnlyValue = new HtmlHelper().ReadOnlyValue("someId", "someValue", 'someTextValue');
 
	...
	var expectedValue = @"<input type=""hidden"" id=""someId"" value=""someId"" /><label for=""someId"">someValue</label>";
 
	Assert.AreEqual(expectedValue, readOnlyValue);
}

The new test doesn’t look as clean as the old one but the assertion is much more obvious so if it fails then we can quickly work out why.

About a month ago or so I described how we did some work to ensure that we were calling a class the same way in our tests as in our production code and while I think that was a good choice in that situation we came across a similar problem this week where we weren’t so sure.

The piece of code in question was being used to create the view model for a page and one of the pieces of data that we wanted to show on this page was the date on which something would be valid which is currently today’s date.

We were working with some code that looked a bit like this:

public class SomeRandomModel
{
	...
 
	public static SomeRandomModel CreateForSomeSituation(string value1, string value2)
	{
		return new SomeRandomModel(value1, value2);
	}
 
	public string SomeDate()
	{
		return DateTime.Today.ToDisplayFormat();
	}
}

public static class DateTimeExtensions 
{
	public static string ToDisplayFormat(this DateTime aDateTime)
	{
		return String.Format("{0:MMM d, yyyy}", aDateTime)
	}
}

Initially we put the following test around the code:

[Test]
public void ShouldConstructModelForSomeSituation()
{
	var model = SomeRandomModel.CreateForSomeSituation("", "");
 
	...
 
	Assert.AreEqual(DateTime.Today.ToDisplayFormat(), model.SomeDate()); 
}

The problem with this test is that it could theoretically fail if the ‘DateTime.Today’ in the test and the ‘DateTime.Today’ in the code were fired near enough to midnight that they returned different days.

It’s not quite as big a problem as we would have if we were actually comparing the equality of the two dates but it’s still not great to write a test which might fail sporadically.

We therefore need to find a way to control the input to the code so that we can test the output.

The problem is that the date is always going to be today at the moment so it seems to unnecessarily complicate the ‘CreateForSomeSituation’ method if we had to pass in ‘DateTime.Today’ as a parameter.

Our current solution was therefore to create a test only method which takes in a date and then get the original method to delegate to the test only one while passing in ‘DateTime.Today’ as the extra parameter.

public class SomeRandomModel
{
	private readonly DateTime aDateTime;
	...
 
	public static SomeRandomModel CreateForSomeSituation(string value1, string value2)
	{
		return CreateForSomeSituation(value1, value2, DateTime.Today);
	}
 
	public static SomeRandomModel CreateForSomeSituationForTest(string value1, string value2, DateTime aDateTime)
	{
		return new SomeRandomModel(value1, value2, aDateTime);
	}
 
	public string SomeDate()
	{
		return aDateTime.ToDisplayFormat();
	}
}

Our test now becomes:

[Test]
public void ShouldConstructModelForSomeSituation()
{
	var model = SomeRandomModel.CreateForSomeSituationForTest("", "", new DateTime(2009, 10, 10));
 
	...
 
	Assert.AreEqual("10 Oct 2009", model.SomeDate()); 
}

We can now specify our expectation a bit more cleanly since we aren’t tied to ‘DateTime.Today’.

It helped solve our problem reasonably well but it still seems a bit weird to me to have ‘test only’ code but I’m not sure what a better solution would be.

I recently came across a interview with Dave Hoover where he talks through the idea of working as an apprentice software developer and suggests some ways to do this more effectively.

I think the easiest thing to get wrong in software development is to over estimate our ability and there is even a study that proves that theory. Hoover refers to this as ‘having an accurate self assessment‘.

If we work on the same project for a while then we’re going to get pretty good at navigating that code base and we’ll probably be able to solve any problem and add any piece of functionality fairly easily which only helps fuel the belief.

Although it’s very difficult to avoid I don’t know if there is actually much value in comparing ourselves to others in terms of skill level. All that tells us is whether in our opinion at a given point in time we are better or someone else is better.

I think the most valuable thing that we can gain from working with people who totally outclass us or mentors ahead of us in the field is that we can see the areas in which they are much more skilled and then we can start asking questions to work out what steps they have taken to become that good.

Guiding the conversation so that we can learn how others first became knowledgeable in a certain area, what books they would suggest as a starting point and so on is much more useful in helping us to improve.

It seems like everyone you speak to will have a different opinion on what are the best things to learn, so I find that it’s useful to remember that we aren’t going to be outstandingly good at every single area of software development. There are just too many different areas for that to be possible.

There will also likely be suggestions to study the classics and while I think it is useful advice I do think that sometimes it’s not immediately applicable and there may well be more recent books on a topic which are more accessible to us when we are first learning.

We need to reach the stage of learning a subject where our curiosity guides us towards the classics at which stage we can ask for advice on which one might be good to read.

Ravi Mohan has a cool blog post where he defines the ‘book chain‘ pattern which suggests that getting suggestions on a useful order in which we could read books from more experienced practitioners would also work well.

I’m lucky enough to work with quite a few people who are very motivated by becoming really good at software development so there’s no shortage of peers to network with.

On a one to one basis we talk through the things we’re doing on our projects which has the added benefit of forcing you to know what you’re talking about because the other person doesn’t necessarily have the context to fill in gaps you miss in your explanation.

My colleagues Dave and Liz have been organising ‘project flashlights’ in the ThoughtWorks Sydney office every week where people can present a problem they’re trying to solve on their project or something interesting that they’ve been doing which works well for encouraging people to bounce ideas off each other.

I tend to code with colleagues outside of work in coding dojos or just randomly at least once a week and I think taking some time to practice with other people can be quite useful.

I’m not sure if it works so well when we’re totally novice at a skill but certainly even with just a few weeks experience using a new language I’ve found that working through problems with others helps me learn much more than if I work alone.

Beyond that there are code katas to practice on and another bit of advice that I still haven’t taken is to work on open source projects to embrace a different type of coding than we might get on our normal projects.

In summary

I really like the ideas behind software craftsmanship and I think it’s great that there are people in the industry always looking to improve themselves better and willing to help others do that as well.

Dave Hoover and Ade Oshineye’s ‘Apprenticeship Patterns‘ is currently available on Safari and will be out in shops by the end of the month.

I reviewed an early version and it’s certainly an interesting read for anyone attempting the road to mastery.