Tag Archives: Anti-pattern

Overusing the Null-Conditional Operator

The null-conditional operator is the ?. between object and field/property/method. It simply says that if the thing on the left hand side is null then the thing on the right hand side is not evaluated. It is a shorthand, so:

if (a != null)
{
    a.DoSomething();
}

becomes

a?.DoSomething();

And that’s great. It makes life much simpler, and if you’re using ReSharper it will alert you when you could use this operator over a null guard check.

But, and this is quite a big “but”, I have noticed a trend to overuse it.

I’ve seen people do crazy stuff like replace most (almost all) instances of the dot-operator so their code is littered with these question-marks before the dot.

var result = myObject?.GetSomething()?.SomeValue?.ToString()?.Split()?.Where(s=>s?.Length > 0);

And when you get to that level of lunacy you’re basically turning on the old Visual Basic OnError Resume Next head-in-the-sand error handling anti-pattern.

I want to make this absolutely clear. The null-conditional operator is not bad, per se. However, over using it or using it without thought to the logic of your application is bad as it hides potential bugs.

You should only use it when you would normally do a null check in advance. If ReSharper says you can refactor your code to use it, then it most likely is fine to use (you were probably using a longer construct for the same thing already which implies you’ve most likely thought about it – No on likes writing reams of code for no good reason).

Parallel Loop Anti-pattern

Here’s a quick parallel loop anti-pattern. In other words, don’t do this, it will only make you miserable.

If you want to start tasks in a loop watch out for including the loop variable as a closure to the task body. For example:

Task[] tasks = new Task[20];
for (int i = 0; i < 20; i++)
    tasks[i] = Task.Factory.StartNew(
        () => Console.WriteLine("The loop index is {0}", i));
Task.WaitAll(tasks);

What happens is that the variable i is updated in each loop iteration. So, the task uses the value of i as it is when the task runs. Since the task does not run in the loop (it may run at any time the task schedule sees fit to run the task) the value of i could have updated by the time the task actually runs.

In the example program above, my output showed that i was 20 for every single iteration. Incidentally, if you are using i for an indexer you’ll notice that 20 will be out of range for something with 20 elements (which uses indexes 0 to 19) which just adds to the misery.

If you have something like ReSharper installed the it will warn you that you “Access to modified closure” and underline the uses of i that are affected.

So, if you must use run the body of a loop in parallel you are much better off using Parallel.For than trying the above.

Rant of the day: IDisposable

My colleagues are probably used to the fact that I rant about code quality frequently. I take code quality very seriously. Not because I’m especially expert in it, but because features of basic code quality make it easier for other people to read and maintain the code.

Today’s irritation comes from some code (replicated in a number of classes I might add) that implements IDisposable. It is a fine interface and by implementing it you are telling the rest of the world that you have some stuff that can’t just be left to the garbage collector to clean up. These are things like file streams, database connections, etc. Any type of scarce resource that you want to hand back as soon as you are finished with it rather than leave it up to the garbage collector.

However, I came across this “gem” in some code today where the class, basically a utility class, contained no fields (so it wasn’t holding on to anything at all, let alone anything that might be a scarce resource). Yet, for some reason it implemented IDisposable. What was it going to dispose? What could it dispose?

The answer was in the code:

public void Dispose()
{
    // Nothing to dispose of.
}

Quite!

Quote of the day

“Just changing your variable name rarely affects the outcome of your code. Just as naming your dog one name vs another. The dog is still the same dog.” [^]

The try-catch-ignore anti-pattern (again!)

I’ve blogged about this a few times, but today I just want to highlight the frustration this causes on fellow developers. Earlier today I saw a tweet from Chris Canal that said:

“Are you swallowing exceptions there?! Hold on, let me get something to break your fingers with 😐” [^]

All too often I’ve seen the just-ignore-it school of software development when it comes to error messages. It makes it very difficult to track down bugs.

If there is a valid reason for ignoring an exception then document it. State clearly in the comments exactly why you are ignoring the exception. Log the exception at the very least – I want to know when it happens, how often and why. I don’t like errors being swallowed up. I like to have them all fixed.

Incidentally, when I set up the development wiki in my company one of the first things was to put the quote on the front page “Always write software as if the person that will have to maintain it is an axe wielding maniac” – It is a very good rule to develop software by. I highly recommend it.

The try-catch-return-false-throw-catch-return-false-throw-fail anti-pattern

I recently came across a wonderful anti-pattern. Well, anti-patterns are not wonderful, but the mind just boggles at the sheer bloody lunacy of this particular anti-pattern. I’m not going to show you the original code, but I’ll show a general representation of what it did:

        private void DoStuff()
        {
            if (!C())
                throw new Exception("The widget failed to process");
        }

        private bool C()
        {
            try
            {
                B();
            }
            catch (Exception ex)
            {
                return false;
            }
            return true;
        }

        private void B()
        {
            bool worked = A();
            if (!worked)                throw new Exception("The process failed");
        }

        private bool A()
        {
            try
            {
                // Do stuff that might fail.
            }
            catch (Exception ex)
            {
                return false;
            }
            return true;
        }

Now I’ve not shown all the code in order to concentrate on the weird stuff. For context DoStuff() was called from the code behind file for an ASPX page, so when it throws an exception the user will be redirected to the error page. The original exception (caught in method A()) is thrown by code from within the .NET Framework itself. It is a valid exception as the situation should only ever occur if the server is misconfigured.

There are many things wrong with this, not least is the number of things thrown. Mostly, what got me was that the wonderful diagnostic information in exceptions is repeatedly thrown away. Nothing is logged anywhere, so no one would be able to ever tell what went wrong. This is utterly hellish from a debugging point of view. In this case it was because of a misconfigured server and took several hours to track down. Had the original exception been allowed to bubble to the top (with appropriate finally blocks in place for clean up) and the exception information logged then the error could have been tracked and fixed in a matter of minutes. Luckily this happened on a dev system. I’m sure a customer would not have been pleased if their revenue generating website was brought down for hours because of a minor, but incorrect, change in a web.config file. In fact, in a production environment the tools for tracking down the error would most unlikely not exist at all. On a dev system at least a debugger and easy access to source code is present.

So, how could this code have been better written?

        private void DoStuff()
        {
            try
            {
                C();
            }
            catch (Exception ex)
            {
                // If you can add information for diagnostic purposes
                // then create a new exception and throw it. Remember to
                // add the existing exception in as the innerException.
                throw new Exception("The widget failed to process", ex);
            }
        }

        private void C()
        {
            try
            {
                B();
            }
            // Do not catch the exception, nothing was was done with it.
            finally
            {
                // Do any cleanup.
            }
        }

        private void B()
        {
            try
            {
                A();
            }
            catch (Exception ex)
            {
                // Put this caught exception into the new exception
                // in order to keep the detail. This new exception
                // adds detail to the error.
                throw new Exception("The process failed", ex);
            }
            finally
            {
                // Do cleanup
            }
        }

        private void A()
        {
            try
            {
                // Do stuff that might fail.
            }
            finally
            {
                // Do cleanup
            }
        }

This is better, if you ignore the fact that I’m catching and throwing general Exception objects – You should always throw and catch the most specific exceptions that you can.

The main benefit here is that the original exception is allowed to bubble up. When it is possible to add value to the exception a new exception is created and it holds the original as the innerException object.

Note that “The process failed” and “The widget failed to process” are stand-ins to represent meaningful and valuable exception messages. In normal circumstances messages like that do not usually add value.