A coder's new year's resolution

When I write and review code I'm always looking to have one thing happen per line. I've mostly favoured this because of how much more readable it makes the code e.g. see this review http://codereview.stackexchange.com/a/111706/22816.

However, I refactored some code and introduced a massive bug (caught by tests!) because I missed a side effect... The real code was a lot longer (100+ LoC method - hence the refactor). It was something of this form though:

var i = 0;
var maxAttempts = 5;
while (true)
{
     var workRequest = someQueue.GetWork();
     var workResult = someService.Execute(workRequest);
     if (!workResult.IsSuccess)
     {
         Log("{0}, {1}, {2}", workRequest.Name, i++, workResult.ErrorsDisplay);
     }
     if (i > maxAttempts)
     {
         throw new DomainException("Some details!");
     }
}

When I refactored the code I changed the line that incremented the i variable and lost the side effect! It's a really important bit of information and it's buried within a line so it's easy to miss (and I did). I ran our tests and a couple were aborted due to timeouts... I quickly realised that I'd dropped an important bit of code and used a diff to figure out what had gone wrong.

There's another thing that would have helped - variable naming. The name i doesn't convey any importance. As developers, we basically skip over anything that looks like a loop variable. While I was refactoring I probably thought I was in a for loop because the method was too long to see on one page.

I'm guilty of committing code that was only a 'quick and dirty' experiment that I'd meant to clean up; so I'm making it my new year's resolution to write cleaner code the first time!

Comments

Popular posts from this blog

Trimming strings in action parameters in ASP.Net Web API

Full text search in Entity Framework 6 using command interception

Composing Expressions in C#