C# Anti-Patterns – Understanding the For-If Antipattern

anti-patternsc

I was reading on this blog post about the for-if anti-pattern, and I'm not quite sure I understand why it's an anti-pattern. 

foreach (string filename in Directory.GetFiles("."))
{
    if (filename.Equals("desktop.ini", StringComparison.OrdinalIgnoreCase))
    {
        return new StreamReader(filename);
    }
}

Question 1:

Is it because of return new StreamReader(filename); inside the for loop? or the fact that you don't need a for loop in this case?

As the author of the blog pointed out the less crazy version of this is:

if (File.Exists("desktop.ini"))
{
    return new StreamReader("desktop.ini");
}

both suffer from a race condition because if the file is deleted before the creation of the StreamReader, you'll get a File­Not­Found­Exception.

Question 2:

To fix the second example, would you re-write it without the if statement, and instead surround the StreamReader with a try-catch block, and if it throws a File­Not­Found­Exception you handle it in the catch block accordingly?

Best Answer

This is an antipattern as it takes the form of:

loop over a set of values
   if current value meets a condition
       do something with value
   end
end

and can be replaced with 

do something with value

A classic example of this is code like:

for (var i=0; i < 5; i++)
{
    switch (i)
        case 1:
            doSomethingWith(1);
            break;
        case 2:
            doSomethingWith(2);
            break;
        case 3:
            doSomethingWith(4);
            break;
        case 4:
            doSomethingWith(4);
            break;
    }
}

When the following works just fine:

doSomethingWith(1);
doSomethingWith(2);
doSomethingWith(3);
doSomethingWith(4);

If you find yourself performing a loop and an if or switch, then stop and think about what you are doing. Are you overcomplicating things and can the whole loop and test just be replaced with a simple "just do it" line. Sometimes though, you will find you need to do that loop (more than one item might match the one condition, for example), in which case the pattern is fine.

That's why it's an anti-pattern: it takes the "loop and test" pattern and abuses it.

Regarding your second question: yes. A "try do" pattern is more robust than a "test then do" pattern in any situation where your code isn't the sole thread on the whole device that can change the state of the item under test.

The problem with this code:

if (File.Exists("desktop.ini"))
{
    return new StreamReader("desktop.ini");
}

is that in the time between the File.Exists and StreamReader attempting to open that file, another thread or process could delete the file. So you'll get an exception. Therefore that exception needs to be guarded against via something like:

try
{
    return new StreamReader("desktop.ini");
}
catch (File­Not­Found­Exception)
{
    return null; // or whatever
}

@Flater raises a good point? Is this itself an antipattern? Are we using exceptions as control flow?

If the code read something like:

try
{
    if (!File.Exists("desktop.ini")
    {
        throw new IniFileMissingException();
        return new StreamReader("desktop.ini");
    }
}
catch (IniFileMissingException)
{
    return null;
}

then I would indeed be using exceptions as a glorified goto and it would indeed be an anti-pattern. But in this case we are just working around the undesirable behaviour of creating a new stream, so this isn't an example of that anti-pattern. But it is an example of working around that anti-pattern.

Of course, what we really want is a more elegant way of creating the stream. Something like:

return TryCreateStream("desktop.ini", out var stream) ? stream : null;

And I'd recommend wrapping that try catch code in a utility method like this if you find yourself using this code a lot.

Related Topic