Mark Needham

Thoughts on Software Development

Coding Dojo #17: Refactoring Cruise Control .NET

with 5 comments

After a couple of weeks of more experimental coding dojos this week we decided to get back to some pure coding with the session being focused around doing some refactoring of the continuous integration server Cruise Control .NET.

The overall intention of the refactoring we worked on is to try and introduce the concept of a ‘ChangeSet’ into the code base to represent the revisions that come in from source control systems that CC.NET integrates with.

The Format

We had 6 people for the majority of the dojo so we resorted to the Randori style with each pair at the keyboard for 10 minutes before rotating.

Dave and I have been reading/trying out the Pomodoro technique in our spare time recently so we decided to use Pomodoro’s idea of reflecting on our work by stopping every 3 pairing sessions and discussing what we’d done and whether we wanted to vary the approach or keep going the same way.

What We Learnt

  • The most obvious place in the code where the ‘ChangeSet’ concept made sense was in the RssFeedPublisher which was taking a collection of modifications and then converting them back into the same revision format that the data was in when it came from the source control system.

    The ISourceControl.GetModifications() method was the one that we needed to change if we wanted to introduce the changes which we started doing by changing the return type from being a modification array to an IEnumerable<Modificatiion>.

    The goal was to drive towards a place where we could create a ChangeSet and have that extend an IEnumerable<Modificatiion> so that we could easily get that into the code before working out how to remove the concept of a Modification.

    We wanted it to extend a more generic type than array since we didn’t want to tie the ChangeSet class to something as concrete as an array.

    Even with seemingly minor change we still ended up taking around 40 minutes to get the code compiling again – we were very much leaning on the compiler to guide us on what to fix, a technique Michael Feathers talks about in Working Effectively with Legacy Code.

    It would be interesting to see how a refactoring like this would work in a dynamic language like Ruby where you would lose the compiler support but still have the ability to run your tests to help guide you to the places that need fixing.

  • There were 963 usages of Modification in the code so we didn’t have the option of just deleting it straight away! I’ve not yet worked on a code base this size so it was interesting for me to see how we were forced into a smaller steps approach by the code.
  • After 3 pairing sessions we discussed the approach that we were taking which had led us to a situation where the code still wasn’t compiling.

    The alternative approach was to go in even smaller steps and make another method on ISourceControl for ‘GetModfications’ with a different signature and then delegate from the new method to the existing one.

    The problem with this was that there were 20 classes which implemented ISourceControl so we would have had to implement the delegation in all of these or create an abstract base class which did the delegation and then get all the existing implementors to extend the case class instead.

    We decided to keep going with our original approach for 3 more pairs as it seems like we were quite close and it wasn’t clear whether changing the approach would give us significant benefits.

  • The main compilation errors in the code were actually tests which no longer compiled due to the fact that IEnumerable doesn’t have a ‘Length’ property on it whereas array does.

    CC.NET is a .NET 2.0 code base so we weren’t able to introduce LINQ into the code which would have made it really easy to just make use of the ‘Count’ extension method instead of casting the results from ‘GetModification’ back to an array in the tests for the time being.

    We accidentally came across the ICollection interface towards the end which perhaps would have been a better choice than IEnumerable as it would have allowed us to avoid the nasty casting and just make use of the Count property on ICollection instead.

For next time

  • The pace this week was a bit slower but it definitely seemed to keep people more involved so we’re going to try and keep it more focused on the coding rather than experimentation. Possibly a refactoring exercise on some Java code as we have more people using that on their projects.
Be Sociable, Share!

Written by Mark Needham

June 12th, 2009 at 5:07 pm

Posted in Coding Dojo

Tagged with

  • It occured to me after, that a single call to GetModfications could result in multiple ChangeSets. So a ChangeSet that implements ICollection is not enough. A ChangeSetCollection that implements both IEnumerable and IEnumerable is needed for the return value of GetModifications.

  • Now with more generics:

    It occured to me after, that a single call to GetModfications could result in multiple ChangeSets. So a ChangeSet that implements ICollection<Modification> is not enough. The return value of GetModifications should implement both IEnumerable<Modification> and IEnumerable<ChangeSet>.

  • Hmmm that makes it a bigger refactoring/code change than we anticipated although I imagine our approach wouldn’t be that different than what we did in this dojo. Just there’s more steps to it now!

  • Pingback: introducing a ChangeSet class to CruiseControl.net: why? « in two places at once()

  • Pingback: introducing a ChangeSet class to CruiseControl.net: shifting to supertypes – in two places at once()