An example that I spotted where we did this recently was while building a 'BusinessSummary' object whose state was based on the state of a collection of other objects.

The code was keeping a count of the various states of the business objects:

public BusinessSummary buildSummaryObject() {
	BusinessSummary businessSummary = new BusinessSummary();
 
	for(BusinessObject businessObject : businessObjects) {
		State state = businessObject.getState();
		if(State.STATE_1.equals(state)) {
			businessSummary.incrementState1();	
		} else if(State.STATE_2.equals(state)) {
			businessSummary.incrementState2();
		} else {
			// and so on
		}					
	}	
	return businessSummary;
}

In this example 'State' is an enum representing the state the 'BusinessObject' is currently in.

Our first change to the code was to push the logic around state back into the 'BusinessObject' which results in the following code:

public BusinessSummary buildSummaryObject() {
	BusinessSummary businessSummary = new BusinessSummary();
 
	for(BusinessObject businessObject : businessObjects) {
		if(businessObject.hasState1()) {
			businessSummary.incrementState1();	
		} else if(businessObject.hasState2()) {
			businessSummary.incrementState2();
		} else {
			// and so on
		}					
	}	
	return businessSummary;
}

public class BusinessObject {
	...
	public boolean hasState1() {
		return State.STATE_1.equals(state);
	}
	...
}

I think this is an improvement but it still isn't following the tell don't ask principle, we're just asking to be told about the data instead of asking for the data itself.

We didn't have the time to do the next change where we would have got rid of the 'hasState' methods and just got the 'BusinessObject' to update the 'BusinessSummary' itself:

public BusinessSummary buildSummaryObject() {
	BusinessSummary businessSummary = new BusinessSummary();
 
	for(BusinessObject businessObject : businessObjects) {
		businessObject.writeTo(businessSummary);					
	}	
	return businessSummary;
}

public class BusinessObject {
	private final State state;
	...
	public void writeTo(BusinessSummary businessSummary) {
		if(State.STATE_1.equals(state)) {
			businessSummary.incrementState1();	
		} else if(State.STATE_2.equals(state)) {
			businessSummary.incrementState2();
		} else {
			// and so on
		}
	}
	...
}

Looking back on this code having written this post I wonder whether we need to do the loop or whether it would be better to pass the collection of 'businessObjects' to the 'BusinessSummary' and let it take care of that logic instead.

If we did that then we would need to expose 'hasState1′ and 'hasState2′ methods on 'BusinessObject' like on the first refactoring so that we could work out how to populate 'BusinessSummary'.

I prefer the solution where 'BusinessObject' writes to the 'BusinessSummary' although it seems like 'BusinessObject' now has more than one reason to change – if it changes or if 'BusinessSummary' changes. This would violate the single responsibility principle

I discussed this with Dave and he pointed out that as long as the contract that 'BusinessSummary' and 'BusinessObject' have – i.e. the 'incrementState1′ and 'incrementState2′ methods – stays the same then it wouldn't really be a problem.

The former describes the need to get a piece of code into a test harness while the latter describes the need to assert whether that piece of code is doing what we want it to.

On the projects I've worked on we've tended to run into problems with the latter more frequently and Matt and I actually ran into this problem when we were refactoring some code into a role based interface approach.

We started with the following code:

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 IContent BuildContent()
	{
		if(BusinessType == "BusinessType1")
		{
			return CreateContentForBusinessType1();
		}
		else if(BusinessType == "BusinessType2")
		{
			return CreateContentForBusinessType2();
		}
		else
		{
			return CreateContentForEveryOtherBusinessType();
		}
	}
}

The 'BuildContent' method is the one that we're really interested in here but it's a private method on the controller so we currently don't have an easy way to assert that it's being created correctly.

In order to test that functionality more easily we decided to make the method public so we could just call it directly.

We wanted to do this so that we could write some tests around this bit of functionality before we refactored it so that we could be sure we hadn't broken the way it worked while doing so.

As Hamlet D'Arcy points out, if we're going to call it refactoring then we need to make sure that we're protected by tests. Otherwise we're just changing stuff.

[Test]
public void ShouldCreateContent()
{
	var someRepository = MockRepository.CreateMock<ISomeRepository>();
 
	var controller = new BusinessType1Controller(someRepository);
 
	someRepository.Expect(s => s.GetBusinessType1Message()).Return("someValue");
 
	var content = controller.BuildContent();
 
	// and so on	
}

We did the same for the other business types as well.

I normally don't like making private methods public but we didn't intend to checkin our code until the refactoring was complete at which point the method would be made private again.

The code ended up something like this:

public class BusinessType1Controller : ApplicationController
{
	public override IContent CreateContent()
	{
		return new BusinessType1Content(someRepository);
	}
}

public class BusinessType1Content : IContent
{
	private readonly ISomeRepository someRepository;
 
	public BusinessType1Content(ISomeRepository someRepository)
	{
		this.someRepository = someRepository;
	}
 
	public string GetMessage()
	{
		// this would be a different repository call for other business types
		return someRepository. GetBusinessType1Message();
	}
}

We were able to create a test against the business types directly instead of having to write a test against the controller:

[Test]
public void ShouldCreateContent()
{
	var someRepository = MockRepository.CreateMock<ISomeRepository>();
 
	var content = new Content(someRepository);
 
	someRepository.Expect(s => s.GetBusinessType1Message()).Return("someValue");
 
	var message = controller.GetMessage();
 
	// and so on	
}

Having got these new tests passing the ones we'd written against 'BuildContent' now also worked and since they were effectively testing the same thing we were able to get rid of them and make 'BuildContent' private again.

Despite the fact that I was initially reluctant to expose the 'BuildContent' method this approach seemed to work out reasonably well.

An alternative approach would have been to create a test only ApplicationController and then create a method on that which called the 'OnActionExecuting' method.

We've done that to test some other things and it would have allowed us to test the creation of 'Content' in a more roundabout way without needing to make 'BuildContent' public.

The additional effort involved in doing that for not much gain means that given a similar situation in the future I'd probably use just make the method public again!

An agent noun is defined by Wikipedia as:

In linguistics, an agent noun (or nomen agentis) is a word that is derived from another word denoting an action, and that identifies an entity that does that action.

Some typical examples of this are classes which end in the name 'Manager', 'Retriever', 'Helper' or even 'Controller' as Carlos points out.

It's often the case that these classes better describe a method which belongs on another object and I quite like Peter's idea that agent nouns are useful for describing the roles that our objects carry out.

We came across an interesting problem related to this a while ago where we wanted to calculate the age of a given customer and then send that data to another web page where it would be displayed.

The date of birth was an attribute on the 'Customer' object and we didn't need any of the other attributes of a customer on this web page so my first thought was that we needed an 'AgeCalculator' class.

public class AgeCalculator 
{
	private readonly IClock clock;
 
	public AgeCalculator(IClock clock)
	{
		this.clock = clock;
	}
 
	public int CalculateAge(DateTime dataOfBirth)
	{
		// do some calculation with clock & date of birth here
	}
}

public void SomeMethod()
{
	var age = new AgeCalculator(new Clock()).CalculateAge(customer.DateOfBirth);
	// and so on 
}

This approach worked quite well as it allowed us to easily test the age calculation logic in isolation and we could then pass on the age returned to the web page.

The thing I didn't like about this solution is that it seems a lot like an agent noun class.

The giveaway in this case is that the method name is just a variation on the class name. This suggests that this behaviour belongs on another object instead.

It's not as object oriented as it could be but it doesn't seem as bad as some of the other examples such as classes ending in 'Manager' since this object is only solving one problem and we do have calculators in real life.

Some colleagues suggested that perhaps age should be an attribute on the customer which does seem to be a better place for this logic to reside.

On the other hand age isn't used anywhere else in the application and we don't need any of the other attributes of customer as I mentioned earlier.

I'm not sure what the 'right' solution is but there seem to be a few different potential approaches:

• Should age be an attribute of the customer? There could be a tiny type 'Age' which does the above logic.
• Should we have a role/interface 'ICalculateAges' which the customer implements?
• Should we just keep the 'AgeCalculator'?
• Is there some other solution that I'm not seeing?!

–

In reality after we'd discussed all these alternatives it turned out that the requirement was slightly wrong and that we actually needed to send down the date of birth and not the age.

I still think it's an interesting problem though and one that I'm bound to come across again!

This situation doesn't always happen because if we have two people available and realise before we start on the task that there is some doubt as to which solution is the most appropriate then we can adopt a set based approach whereby we try out multiple potential solutions in parallel.

However I've found that there are times when we might think that one solution is much better than the alternatives and we will start working on that and discard the alternatives for the moment.

Frequently the approach we choose will solve our problem but on other occasions we can end up getting a bit bogged down when it doesn't work out as expected.

At this stage I often find that my instinct is to try and solve these problems and while I think this is certainly a valid approach it's also very easy to end up shaving the yak.

This seems to happen much more frequently when working alone and even when we're getting feedback which suggests that what we're trying isn't working we can be reluctant to back track.

I think there is some pride involved as I frequently find myself really wanting to make the current approach work as I believe that a better developer than myself would be able to do so.

Sometimes that is the case but more often than not when I work through the problem with a colleague their suggestion is to try another approach rather than continuing to struggle with the current one.

It doesn't make a lot of sense to spend all our time going down one avenue when our goal is solve a particular problem and the solution is a means to that end.

When discussing this with Nick he pointed out that we also need to recognise when we should persevere with the approach.

One way to avoid too much yak shaving is to set a timebox by when we should either have a better idea of whether we are going to be able to solve the problem with this solution or if we need to consider an alternative.

If there's some light at the end of the tunnel with an approach after this period then I would be inclined to keep on going but it's sometimes difficult to tell how close we actually are and how much is wishful thinking!

I still sometimes find myself struggling to decide whether to keep going or change direction so it'd be interesting to know if anyone has ideas around doing so more effectively.

I thought that it was probably because I'm more used to that approach than any other but in Michael Feathers' description of TDD in 'Working Effectively With Legacy Code' he points out the following:

One of the most valuable things about TDD is that it lets us concentrate on one thing at a time. We are either writing code or refactoring, we are never doing both at once.

It also temporarily removes us from the feeling of drowning in all the new information which is typical at the start of projects. In a way it reminds me of the 'Retreat Into Competence' pattern from 'Apprenticeship Patterns' as it gives us a chance to regain composure and take in all the new information.

We can probably make a much more useful contribution if we only need to understand one small piece of the system rather than having to understand everything which can often be the case with what Feathers coins the edit & pray approach to development.

It's still necessary to spend some time trawling around the code base working out how everything fits together but I'm now more aware that it's also really useful to take some time to just focus on one much smaller class or piece of functionality.

Perhaps also something to keep in mind the next time I'm pairing with someone who's new to a project that I've been working on for a while.

From what I've noticed there seem to be two reasons that we end up with inconsistent code on projects:

1. People's personal preferences for how certain bits of code should be written vary.
2. The team is trying to incrementally move towards an improved solution to a problem encountered but isn't completely there yet.

I think the first of these is the area which Frank and Pat are addressing in their posts.

However I think there is some overlap between the two. For example someone may rewrite a bit of code in their own style and while they would suggest that what they are doing is improving the code base their team mates may think that what they are doing is merely creating more inconsistency.

Potential coding inconsistency falls into three categories as I see it:

Personal Preferences

A simple example of this that we had on the last project I worked on was around how we should write simple conditional statements when used as guard clauses.

These were the 3 different ways that people preferred and we had all the different styles across the code base!

public String SomeMethod()
{
	if(SomeCondition()) return "hello";
}

public String SomeMethod()
{
	if(SomeCondition()) 
		return "hello";
}

public String SomeMethod()
{
	if(SomeCondition()) 
	{
		return "hello";
	}
}

We eventually agreed to use the last one because it was the least controversial choice and although people who preferred the other two approaches thought it was too verbose they were fine with using that as a compromise.

Pat covers a lot of the other types of code that I would classify as being linked to personal preference and I think that it would be beneficial to have coding standards/agreement in the team for the patterns we favour/names we're going to use in this area.

It's way easier to just go and write code in your own personal style and I do this way too frequently but it's much better for the team if we follow a common style.

Improving the code but perhaps not significantly

I've written previously about the way that we've been making use of a 'GetBuilderFor' class to hang our test builders off and while this worked quite well there is a better way to do this which requires less code being written overall.

We can make use of C#'s object initializer syntax to setup fields with values instead of having to create methods to do that and we don't need to write the code to make the builder hang off 'GetBuilderFor'.

As long as we put all the builders in the same namespace we can just type 'new BuilderNameSpace.' and then the IDE will show us the choices of builder that we have.

The problem we experienced was that that we already had 30-40 builders written using the 'GetBuilderFor' approach and there wasn't a significant advantage to be gained by going and rewriting all that code to use the new approach.

In the interests of consistency we therefore decided to keep using the 'GetBuilderFor' approach even though the next time I come across this problem I'd definitely favour the other approach.

Reg Braithwaite has a really interesting post in which he talks about 'the narcissism of small code differences' and how different authors 'improve' the code by adding their own personal touch.

I think he describes the type of code changes that I would classify in this and the previous category although it does make me wonder where the line between refactoring and adding your own personal touch to a piece of code lies.

Improving the code significantly

These are the code changes that may create inconsistency but help us to drive a design improvement which will have a big impact on the way we work.

An example of this that we had on my last project was moving our 'domain' model from being defined in WCF messages to being defined independently of other layers in the system.

We wanted to make this change to make the dependency between our code base and the service layer more loosely coupled and we started making that change around about February.

It took maybe a month to move the most awkward parts across since we did it bit by bit in an incremental way rather than stopping everything and making that change.

Along the way the code in these areas was in an inconsistent state and anyone looking at the code base who didn't know the reason why it was like that would think it was truly insane!

Even now there are still bits of code which are more reminiscent of the old approach than the new and these tend to get changed when people are working in that area.

I think this type of inconsistency is somewhat inevitable if we're incrementally making improvements to our code base or following the boy scout rule as Uncle Bob puts it.

In Summary

These are my current observations on coding inconsistency but I'm sure there are other factors which can affect this as well.

I think we should look to reduce the inconsistencies in the first two categories as much as possible and ensure that everyone is driving towards the same outcome with respect to the last category.

'Effect Sketching' is a technique Feathers describes in 'Working Effectively With Legacy Code' and the idea is that we sketch a diagram showing the interactions between the fields and methods in a specific class while browsing through the code.

Dave suggested doing a similar thing but with methods and classes instead while stepping through the code with the debugger.

I set up this code to step my way through:

var container = new UnityContainer();
 
container.AddNewExtension<Interception>();
container.RegisterType(typeof (IIDProvider), typeof (HttpContextIDProvider));
container.Configure<Interception>().SetDefaultInterceptorFor(typeof (GetPaymentBreakdownsService), new VirtualMethodInterceptor());
 
object resolve = container.Resolve(typeof (GetPaymentBreakdownsService));

These are some of my observations from this exercise:

• I found it was much easier for me to remember where I was in the call stack then normal.

  I often get quite engrossed in the individual method calls on objects that I forget where the code actually started before it ended up in the current location. Whenever this happened I was able to refer to my sketch to remind myself of where the code had started from.

• Despite having the drawing I still got a bit lost when the PolicyInjectionBehaviourDescriptor made a call back to the container's Resolve method which meant that the code went through the same path that I'd just followed:

  32 33 34 35 36 37 38

  public IInterceptionBehavior GetInterceptionBehavior( IInterceptor interceptor, Type interceptedType, Type implementationType, IUnityContainer container) { InjectionPolicy[] policies = container.Resolve<InjectionPolicy[]>();

  I was getting really confused watching various different injection policies being resolved instead of the type I was trying to resolve!

• I've previously tried drawing diagrams which just contained the classes in a code base but I've found that including the methods that connect them makes it easier for me to understand what's going on.

  I've been drawing these diagrams out by hand but I thought I'd translate some of it into dot notation so that I could create a version using graphviz to show on here.

  

  The nodes in orange represent classes and the dotted line represents where an event was fired.

• I realised that I still need to spend more time reading code so that I know when to dive into an object and when the details are probably not important. At the moment I'm too prone to getting distracted by wanting to see how a specific method works instead of looking at those details later on when I actually need to know.
• I felt as I was reading the code that in a lot of places the option type from functional programming would have come in quite useful. There is often code similar to this bit from LifeTimeStrategy:

  object existing = lifetimePolicy.GetValue()
  if (existing != null)
  {
      context.Existing = existing
      context.BuildComplete = true
  }

  Instead of existing returning a null it could return 'None' to indicate it hasn't been resolved yet.

• I've read about the 'yield' construct before but I've never seen a need to use it yet in any code I've written so it was interesting to see it being used quite frequently inside PolicySet:

  public IEnumerable<InjectionPolicy> GetPoliciesFor(MethodImplementationInfo member)
  {
      foreach (InjectionPolicy policy in this)
      {
          if (policy.Matches(member))
          {
              yield return policy;
          }
      }
  }

  From my understanding of this construct it seems like it acts in a similar way to a stream i.e. it's only evaluated when it's actually needed.

• In the 'Fundamentals of Object Oriented Design in Uml' Meilir Page Jones suggests that we might want to avoid replicated behaviour in our public APIs since it leads to greater complexity.

  This would therefore seem to suggest that having overloads of methods on an object is something to be avoided. Interestingly in this code base the overloads for 'UnityContainer' are actually defined as extension methods which then delegate back to 'UnityContainer' and pass in default values for unspecified parameters.

  This seems like quite a neat way of getting around the problem since we keep the API clean while also providing users of the code an easy way to do so.

On the subject of reading code I recently came across an interesting post by designbygravity which describes some approaches for reading code more effectively.

In particular I really liked the section about not hating the code :

You can get sucked into hating the code, merely because it is not yours. Software people tend to be equipped with ample egos, and other people’s code can offend. But realize, their working code is better than your imagined code, because their working code exists right now. So put your ego aside and learn the code in front of you.

It's so easy to drift into this mindset but it's rarely helpful so if we can stop ourselves it's almost certainly beneficial.

This chapter covers Test Driven Development and a technique I hadn't come across before called Programming By Difference.

These are some of my thoughts and our discussion of the chapter:

• In the section on TDD Feathers mentions the copy/paste/refactor pattern which I wrote about a few days ago. Cam pointed out that this technique allows us to get to the green bar more quickly after which we can decide how to clean up the code. It seems like perhaps this could be added to Kent Beck's 'Green Bar Patterns' that he describes in 'Test Driven Development by Example'.

  I've noticed a few times recently that delaying our desire to remove duplication until we can properly see where the duplication lies might be a better strategy than prematurely removing duplication and creating a meaningless abstraction which can be difficult to remove.

• From the examples in the book, programming by difference is used to describe an approach where we create a sub class and override some aspect of the super class. It allows us to quickly add new features although it does increase the complexity of the system since we have to add a new class to achieve this.

  The next step after this is to refactor the code into a state where the new class' behaviour is moved elsewhere or the new class is shaped into something that makes more sense.

  Feathers also points out that while programming by difference may initially lead to our code being in worse shape it can also allow us to see new abstractions which we can then work towards.

• I like the term 'normalised hierarchy' with respect to the way that we create inheritance hierarchies. Feathers suggests that we should look to create a hierarchy where no class has more than one implementation of a method. i.e. if a class has a concrete implementation of a method then that shouldn't be overriden.
• Towards the end of the chapter Feathers asks whether it is overkill to create a particular class which is just acting like a properties collection at the time. We discussed this and Cam pointed out that we seem to either end up with 'complex interactions and simple objects' or 'more complicated objects and simple interactions'.

  From my observations it seems that we err on the side of not creating enough objects but as Naresh Jain points out it is possible to go too far the other way and end up creating objects just for the sake of it.

Michael Feathers recently linked to a paper titled 'Testable Java' that he's currently working on which covers some of the ideas from the book but with examples in Java. Worth a look.

I can't remember coming across this approach previously but I found myself using it to solve a Scala problem last week.

I wanted to find the smallest number in a list having previously written the code to find the largest number in a list.

This was the code I already had:

1
2
3
4
5
6
7
8

  def max(values : List[Int]) : Int = {
    def inner(remainingValues : List[Int], currentMax : Int) : Int = remainingValues match {
      case Nil => currentMax
      case head::tail if head > currentMax => inner(tail, head)
      case _::tail => inner(tail, currentMax)
    }
    inner(values, Math.MIN_INT)
  }

I figured finding the smallest value would be quite similar to that but I couldn't quite see where the duplication between the two functions was going to be.

I somewhat reluctantly found myself copying and pasting my way to the following code:

1
2
3
4
5
6
7
8

  def min(values : List[Int]) : Int = {
     def inner(remainingValues : List[Int], currentMin : Int) : Int = remainingValues match {
      case Nil => currentMin
      case head::tail if head < currentMin => inner(tail, head)
      case _::tail => inner(tail, currentMin)
    }
    inner(values, Math.MAX_INT)
  }

Having done that it became clearer that the duplication between the two functions was on line 4 where we do the comparison of values and on line 7 where we choose the initial max/min value.

My first attempt at refactoring was to try and pull out the comparator function on line 4:

def comparator(x : Int, y : Int, f : Function2[Int, Int, Boolean]) : Boolean = f(x,y)

Which resulted in the 'max' function looking like this:

  def max(values : List[Int]) : Int = {
    def inner(remainingValues : List[Int], currentMax : Int) : Int = remainingValues match {
      case Nil => currentMax
      case head::tail if comparator(head, currentMax, (x,y) => x > y) => inner(tail, head)
      case _::tail => inner(tail, currentMax)
    }
    inner(values, Math.MIN_INT)
  }

At this point I realised that I was doing the refactoring the wrong way around and that I should be trying to make 'max' and 'min' call another function while passing in the differences as parameters to that function.

I ended up with this common function:

   def findValue(values : List[Int], comparisonFunction : Function2[Int, Int, Boolean], startingValue : Int) : Int = {
    def inner(remainingValues : List[Int], current : Int) : Int = remainingValues match {
      case Nil => current
      case head::tail if comparisonFunction(head, current) => inner(tail, head)
      case _::tail => inner(tail, current)
    }
    inner(values, startingValue)
  }

  def max(values : List[Int]) : Int = findValue(values, (x,y) => x>y, Math.MIN_INT)
  def min(values : List[Int]) : Int = findValue(values, (x,y) => x<y, Math.MAX_INT)

I'm not sure whether the name 'findValue' is a very good one for the common function but I can't think of a better one at the moment.

The neat thing about this approach is that it made it much easier to see where the duplication was and I was able to have both the functions working before trying to do that refactoring.

I'm not sure if there are built in functions to do these calculations. I looked quickly and couldn't find any and thought it would be an interesting exercise to write the code anyway.

It worked out much better than I thought it would.

Quite often we would see code similar to this in a controller:

public class SomeController 
{
	public SomeController(Dependency1 valueOne, Dependency2 valueTwo)
	{
		AssertThat.isNotNull(valueOne);
		AssertThat.isNotNull(valueTwo);
		// and so on
	}
}

Where 'SomeController' would have 'Dependency1′ and 'Dependency2′ set up in a Spring configuration file in this example.

I can't really see much benefit in this type of pre condition checking although Raph pointed out that if we got the configuration for this bean wrong then having these assertions would allow us to get quicker feedback.

While that is true it seems to me that we would maybe achieve quicker feedback by a few seconds in return for the clutter that we end up creating in our code. We have to read those assertions every time we come to this class.

I would prefer to have some specific tests for the dependency injection container if we're interested in getting quick feedback in that area.

That seems to be a cleaner solution than having these types of checks in production code or am I missing something?