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

Post a Comment

Popular posts from this blog

Trimming strings in action parameters in ASP.Net Web API

In standard MVC it's very easy to create a model binder to trim all of the strings passed into action methods. The approach I used for that was from here:  http://blogs.taiga.nl/martijn/2011/09/29/custom-model-binders-and-request-validation/ . It works well and it trims the properties of models too. The problem came when I tried to do the same thing for Web API. I added a similar StringTrimmingBinder and registered it as a service using the very non-discoverable API: var provider = new SimpleModelBinderProvider( typeof ( string ),  new StringTrimmingModelBinder()); GlobalConfiguration.Configuration.Services.Insert( typeof (ModelBinderProvider),  0,  provider); As a side note, I have no idea how you're supposed to find out how to register a model binder without opening up Bing and Googling it. Anyway, this worked great when the parameters to the action were actually strings but doesn't work with complex types as parameters. Now, I'v...
Read more

Full text search in Entity Framework 6 using command interception

Image
Background One of the main problems we encountered using Entity Framework was its lack of support for full text search. Our search function is based around a series of processors that attempt to match the incoming phrase against a set of patterns that we're expecting (10+). For example, there's one for dates and times, recent financial quarters as well as more domain specific ones. Everything is cached and so is really fast, except when it isn't: Here's the monitoring chart for our SQL DB in Azure That's a spike to 100% resource utilisation because we fall through to a phrase search as a last resort (which cannot be cached) - it's exceedingly rare but it still hurts. Before EF, the app used full text search and it was one of the casualties when the app was migrated to EF4 several years ago. The lack of full text search has been getting more and more painful as the amount of data has grown. We really needed full text search back... Command intercept...
Read more

Composing Expressions in C#

This post follows on from  Reusing predicates in Entity Framework . In my previous post I introduced the "and" extension method which allowed me to create a new entity framework safe expression from two existing expressions. Here's how we make that happen: The code is from a blogpost from Colin Meek from 2008:  InvocationExpression and LINQ to Entities . I'll just dump my version of the code here. The idea is exactly the same (even using the visitor as an immutable stack): public class InvocationExpander : ExpressionVisitor { private readonly ParameterExpression _parameter; private readonly Expression _expansion; private readonly InvocationExpander _previous; public static T Expand<T>(T expr) where T : LambdaExpression { return (T) new InvocationExpander().Visit(expr); } public InvocationExpander() { } private InvocationExpander(ParameterExpression parameter, Expression expansion, Invocati...
Read more