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?

Something which happens quite often is that you'll come across code which makes use of defensive programming in one of its constructors like so:

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

This is fine in itself but you'll often find another constructor defined close by which looks like this:

public class SomeObject 
{
	public SomeObject(Dependency1 valueOne, Dependency2 valueTwo)
	{
		this.valueOne = null;
		this.valueTwo = null;
	}
}

Depending which constructor you use the outcome is quite different and you probably wouldn't expect the default constructor to allow nulls seeing as the other one prevents you from doing this.

This inconsistency can happen for a variety of reasons.

Test only constructor

It might be a test only constructor which is not intended to be called in production case. It's much easier to get an object under test if we don't have to supply any parameters.

In that case though we're testing the object constructed in a much different state than it will be when we really use it so the value of this type of testing is questionable.

It's probably best to look why we need that test only constructor and see if we can change the code so that we can avoid that problem.

An implicit domain concept

Another reason is that the original pre condition of the object might have changed and now it isn't always the case that we need to provide 'Dependency1′ and 'Dependency2′.

In this case I think it would make more sense to make that more explicit, perhaps by making use of the null object pattern or by finding another abstraction which doesn't require nulls to be passed around.

Either way we should probably look to ensure that both constructors either ignore nulls or both stop them from being set.

Didn't realise pre conditions were being checked

Although this one might seem a bit far fetched it's pretty easy for this to be the case if there are different people working on the code base.

Someone might glance at the class, see that it doesn't quite allow them to construct the object the way they want and create another constructor without realising the way the original one was working.

In this case talking more about the way that we're solving this type of problem across the code base should be enough to ensure that we get some kind of consistency.

Connascence describes the relation between two different bits of code and two bits of code are said to be connascent if a change to one bit of code would require a change to the other bit of the code or if some change to another piece of code would require both bits of code to change for our program to still be correct.

I think this principal is quite similar to the idea of coupling which we seem to use more frequently these days but I found it quite compelling that as I was reading through the different types of connascence that Page-Jones describes I was easily able to identify mistakes I've made and seen made in code.

There are many different types of connascence and Jim Weirich goes through many of them in his presentation from MountainWest RubyConf 2009 titled 'The building blocks of modularity'.

I'll just cover a couple of the ones that seem to cause the most pain from my experience.

Connascence of execution

This describes the situation where two different lines of code have to be executed in a certain order for the program to be executed correctly.

A typical example of this type of connascence occurs when we make use of setter methods to construct objects:

1
2
3
4
5
6
7
8
9
10
11
12
13
14

public void SomeMethod()
{
	var someObject = new SomeObject();
	someObject.Property1 = new Property1();
 
	// some where else far far away
 
	SomeOtherMethod(someObject);
}
 
private void SomeOtherMethod(SomeObject someObject)
{
	someObject.Property1.AnotherProperty = new AnotherProperty();
}

In this example we need line 2 to be executed before line 13 otherwise we'll get a null pointer exception. These two lines therefore have connascence of execution.

Quite often line 13 of this example would be hidden away in a chain of method calls and it wouldn't be completely obvious that it relies on a line further up being executed first.

We eventually end up making what we think is an innocuous reordering of the method calls and suddenly our code doesn't work any more.

In this case to reduce this type of connascence we might look at using a constructor to create our objects, use one of the builder/factory patterns or at least try and capture related code in the same method so that the potential for confusion is reduced.

Connascence of algorithm

This typically describes the situation where we are making use of a data structure in a specific way such that all the pieces of code which interact with that data structure need to know exactly how that data structure works in order to make use of it.

This problem seems to happen particularly when we're over using lists when perhaps another level of abstraction is required.

One example of this might be where a piece of code assumes that another piece of code inserts a certain value into a list.

public class AClassWhichPutsASpecificItemInAList
{
	public List<SomeObject> CreateTheEvil()
	{
		var myEvilList = new List<SomeObject>();
 
		myEvilList.Add(new SomeObject("SomeSpecialName"));
		// and so on
 
		return myEvilList;
	}
}

public class SomeOtherClassFarFarAway
{
	public void SomeMethod(List<SomeObject> someObjects)
	{
		var speciallyNamedObject = someObjects.Where(s.Name == "SomeSpecialName").First();
	}
}

Despite the fact that these two classes never refer to each other they have connascence of algorithm because if 'AClassWhichPutsASpecificItemInAList' decides not to put that value into the list then 'SomeOtherClassFarFarAway' may stop working so we will need to change that as well.

This type of connascence is much more implicit than some of the other types and it may not be immediately obvious that two pieces of code are related.

We could get around this problem by encapsulating this type of logic in its own type so that at least we only have to deal with it once.

The goal is to try and make an implicit piece of code more explicit.

Connascence of convention

This describes the situation where there is an implicit convention for the meaning behind the value of a certain piece of code such that every other bit of code that touches it needs to know this convention. This is quite similar to connascence of algorithm.

An example that I came across recently was around how we passed around the value of an option selected on a form by the user.

The user could either select 'Yes', 'No' or they could choose not to answer the question. If they entered 'Yes' then they would need to answer another question immediately after this.

Later on we needed to pass this data to the service layer. A value of 'No' if they selected that, the answer to the next question if they answered 'Yes' and 'None' if they didn't enter anything.

We ended up having code similar to this in the service call and then again in the binder:

public class TheServiceThatUsesThatValue
{
	public void CallTheService(string theAnswer)
	{
		var theRequest = new ServiceRequest();
 
		if(theAnswer == "None")
		{
			theRequest.TheAnswer = "None";
		}
		else if(theAnswer == "No")
		{
			theRequest.TheAnswer = "No";
		}
		else
		{
			theRequest.TheAnswer = LookUpTheCodeFor(theAnswer);
		}
 
	}	
}

We eventually gave up on the idea of passing 'None' around because that was the bit causing the most confusion. Instead we stored the answer in a nullable data type and then did the conversion to 'None' when necessary in the Service class.

In summary

There are just a couple of the examples that Page-Jones outlines but the general idea is that we want to try and minimise the connascence in our system by creating well encapsulated objects.

Within those objects it makes sense to keep connascence high and in fact if it's not then it might suggest that we have another object waiting to get out.