Mark Needham

Thoughts on Software Development

Coding: The guilty bystander

with 4 comments

While discussing the duplication in our code based which I described in an earlier post with some other colleagues earlier this week I realised that I had actually gone past this code a couple of times previously, seen that there was a problem with it but hadn’t taken any steps to fix it other than to make a mental note that I would fix it when I got the chance.

At the time we needed to fix a bug around this code and noticed that the logic was scattered all around the place but decided to just add to the mess and put our fix in without refactoring the code to make it better for people coming to the code in future.

In a way then it was perhaps poetic justice that when the bug was re-tested there were still problems with it.

This time we decided to fix it properly and with the improved expressiveness the code now has hopefully it will make life easier for other people who have to work with the code.

I think this was also a really clear case of the broken windows theory whereby the likelihood of someone doing something bad to the code is much higher if it’s already in a suboptimal state.

After hearing me describe this Halvard pointed out that it sounded quite like the idea of the guilty bystander whereby if you see a problem with the code and do nothing about it then you are guilty by association even though it wasn’t you who wrote it.

This sounds quite similar to a post written by Ayende where he pointed out that if you own the code then it is your responsibility to keep it in good shape, you can’t keep blaming the previous owners forever.

Another colleague of mine, Silvio, pointed out that sometimes it might not be feasible for us to stop doing what we’re doing to go and fix something else, especially if it’s unrelated code that we just happen to come across. Indeed doing this might lead to a yak shaving situation.

In this situation he suggested that we need to let it be known that something should be done possibly by writing a ‘TODO’ comment in capital letters around the offending code so that the next person who comes across the code can take a look at the offending code without having to assess whether or not it’s in a bad state.

To continue the analogy perhaps this would be the equivalent of calling 999/911/000 to report the situation to the police which is definitely better than just ignoring the problem although not as good as helping to solve it.

In this case the help you’re providing is a bit more indirect than calling the emergency services would be so I’m not sure if the metaphor quite fits!

There are certainly exceptions (such as when we inherit code which is a mess) but in general I always think we’ve gone horribly wrong when we get to the stage where we need to ask the business whether we can have a whole iteration (or more) to refactor the code into shape so that we can start moving faster again.

I believe that while in the short term it’s our responsibility to make sure we deliver the features we’re working on in the current iteration, we also need to make sure that we spend some time ensuring that we can continue to deliver features in future iterations as well and part of that second responsibility is taking the time to mould code into a state where we can build on top of it more easily even if it wasn’t us who wrote it in the first place.

Be Sociable, Share!

Written by Mark Needham

August 30th, 2009 at 8:07 pm

Posted in Coding

Tagged with

  • I think that taking a mental note of the code that needs improvement and returning to it at the first opportunity is the right thing todo. “The first opportunity” is a subtle thing, however. The very next time the code needs to be touched is the right time to clean it up. As you experienced, not doing so could easily lead to further bugs.

    There is another reason not to change code besides Silvio’s point that there might not be time. Changing code introduces risk. Any change can introduce bugs, even while making the code more flexible. Even code that seems obviously incorrect might be providing a behaviour that the business relies on. If an area is already being changed then there will already be acceptance and regression plans in place. Any clean up efforts can be validated by the same tests.

    A drive-by refactoring of an area where change is not planned, on the other hand, can easily introduce defects not detected until much later. This will end up slowing the team down when the goal was to go faster. If there is no other work to do besides refactoring it is best to treat it as a scratch refactoring. Try out some wild ideas but the throw the changes away. When the opportunity does come up to change that code, you will probably know exactly the changes you’d like to make.

  • There’s definitely a high risk of a yak shaving escapade if you set off to clean up everything that you find while coding – it’s a judgement call to choose how far to go. I think more often than not teams I work with by default choose the easy option and do not shave enough yak.

    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”.

    I love the ‘guilty bystander’ analogy btw.

  • @Dave – I think you’ve articulated better than I have the idea of ‘The first opportunity’ – the next time you use it seems to work quite nicely as a rule of thumb although I guess it would depend a lot on when that next time was. If it was while doing a production fix then I imagine we probabably wouldn’t want to do the refactoring at that stage since there’s a chance of introducing another problem into the code.

    Which then links to your idea that we can introduce bugs at any time when we change code.

    I don’t think I quite understand when we will and when we won’t experience gains by choosing to refactor an area.

    There are certainly smells in code that are really easy to see that indicate something should be done but like you say if we go and refactor an area that noone is currently working on then it might be pointless anyway.

    I wonder if we should look to spend more time refactoring code that is in the core domain or inside the green house to use the Eric Evans analogy?

    @Evan – I think like you I’d prefer the default to err on the side of overdoing the refactoring but being aware that we have that tendency so we can work out when we’ve gone too far instead of choosing the easy way and ending up with a code base that’s really difficult to work with.

    I’m not sure of my thinking of TODO – what you say makes a lot of sense & in our current code base we have so many TODOs it’s ridiculous.

    I think maybe it’s more a team discipline thing to make sure we do go and fix the code around the areas that are important to our progress.

  • Thinking about this more today, my opinion on it comes from a pretty specific context. It was a project where we were aiming for monthly releases, had only unit tests but no functional automated tests, a large code base with a lot of features, and generally high code quality. Changes required manual test effort from a team that was half the size of the total development team. The team was fairly enthusiastic about improving the code base; technical activities were not tightly controlled by management. Probably in another context, more cleanup or more effort to clean up would be a good thing.

    In either situation though, I think TODOs are pretty counterproductive. I agree with Evan, they are usually more of an order or command but without a defined person actually being commanded. I would rather people experiment with quick scratch refactorings to see how the code could be improved. A scratch refactoring should take 10 minutes or less. The time will come when the refactoring makes good sense in the context of a story being played and one of the ideas generated from the scratches can be done and committed.