For-if antipattern

Question

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?

Answer 1

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.

Answer 2

Question 1: (is this for-loop an antipattern)

Yes, because you don't need to do your own search for items stored in a queryable subsystem.

In the abstract, the file system, like a database, is capable of responding to queries.  When interacting with a queryable subsystem, we have a fundamental choice as to whether we want such a subsystem to enumerate its contents to us in order that we perform the matching outside the subsystem, or to use the subsystem's native query capabilities.

Let's pretend for a moment that you're looking up a record in a database instead of a file in a directory in a file system.  Would you rather see

SELECT * FROM SomeTable;

and then in loop (e.g. in C#) over the returned cursor looking for ID = 100, or letting the queryable subsystem do what it can to find exactly what you're looking for instead?

SELECT * FROM SomeTable WHERE ID = 100;

I should think that most of us would rightly choose to let the subsystem perform the exact query of interest.  The alternative involves potentially numerous round trips with the subsystem, inefficient equality testing, and forgoing the use of any indexes or other search accelerators, which both databases and file systems provide us.

---

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?

Yes, because that is just how that particular API works — it's not really our choice since this is a library function.  The if checking, before the call, offers no additional value: we have to use the try/catch anyway, since (1) other errors beyond FileNotFound can occur, and (2) the race condition.

Answer 3

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?

The streamreader has nothing to do with it. The anti-pattern emerges because of a clear conflict of intention between the foreach and the if:

What is the purpose of the foreach?

I assume your answer will be something like: "I want to repeatedly execute a particular piece of code"

How many files are you expecting to process?

Since you can only have one specific filename (including extension) in a particular folder, this proves that your code is intended to find one applicable file.

This is also confirmed by the fact that you're returning a value immediately. You don't actually care about a second match, even if it were to exist.

---

There are situation where this is not an anti-pattern.

• If you also look in subdirectories ( Directory.GetFiles(".", SearchOption.AllDirectories) ), then it is possible to find more than one file with the same filename (including extension)
• If you look for partial filename matches (e.g. every file whose name starts with "Test_", or every "*.zip" file.

Note that both of these cases would require you to actually process multiple matches and therefore not return a value immediately.

---

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?

Exceptions are expensive. They should never be used in lieu of proper flow logic. Exceptions are, as their name suggests exceptional circumstances.

For that reason, you shouldn't remove the if.

As per this answer on SoftwareEngineering.SE:

Generally, the use of exceptions for control flow is an anti-pattern, with notable situation- and language-specific cough exceptions cough.

As a quick summary for why, generally, it's an anti-pattern:

• Exceptions are, in essence, sophisticated GOTO statements
• Programming with exceptions, therefore, leads to more difficult to read, and understand code
• Most languages have existing control structures designed to solve your problems without the use of exceptions
• Arguments for efficiency tend to be moot for modern compilers, which tend to optimize with the assumption that exceptions are not used for control flow.

Read the discussion at Ward's wiki for much more in-depth information.

Whether you need to wrap this in a try/catch, is highly dependent on your situation:

• How likely is it that you're going to encounter a race condition?
• Are you actually able to handle this situation, or do you want this problem to bubble up to the user because you don't know how to handle it.

Nothing is ever a matter of "always use it". To prove my point:

Separate studies have proven that you're less likely to get hurt when you wear a safety helmet, safety goggles, and bulletproof vests.

So why aren't we all wearing this safety equipment at all times?

The simple answer is because there are drawbacks to wearing them:

• It costs money
• It makes your movement more cumbersome
• It can be quite warm to wear it.

Now we're getting somewhere: there are pro's and cons. In other words, it only makes sense to wear this equipment in cases where the pro's outweigh the cons.

• Construction workers are much more likely to suffer an injury during their job. They benefit from a safety helmet.
• Office workers, on the other hand, have a much lower chance of getting injured. Safety helmets are not worth it.
• A SWAT team member is much more likely to get shot at, compared to an office worker.

Should you wrap the call in a try/catch? That very much depends on whether the benefits of doing so outweighs the cost of implementing it.

Note that others may argue that it only takes a few keystrokes to wrap it, so it should obviously be done. But that's not the entire argument:

• You need to decide what to do once you catch the exception.
• If there are a lot of different calls to different files all over the codebase, deciding to wrap one in a try/catch will generally mean that you have to wrap all of these cases. This can have a dramatic effect on the amount of effort required to implement it.
• It's perfectly possible that you intentionally want to not handle an exception.
  • Note that your application will have to handle the exception at some point, but that is not necessarily immediately after the exception was raised.

So the choice is yours. Is there a benefit to doing so? Do you think that it improves the application, more so than costing effort to implement it?

Update - From a comment I wrote on to the other answer, as I think it's a relevant consideration for you too:

It very much hinges on the surrounding context.

• If the opening of the streamreader is preceded by if(!File.Exists) File.Create(), then the absence of the file when opening the streamreader is indeed exceptional.
• If the filename was selected from a list of existing files, its sudden absence is again exceptional.
• If you're working with a string that you haven't actually tested against the directory yet; then the absence of the file is a perfectly logical outcome, and therefore not exceptional.