9

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?

  • 1
    see Discuss this ${blog} – gnat May 18 '18 at 15:58
  • 1
    @amon - Thanks, but I don't make any additional points, I'm just trying to understand what the article is saying and how I would correct it. – user306112 May 18 '18 at 16:04
  • 3
    I would not give this much thought. The blog poster is making up some idiotic code that nobody writes, gives it a name and calls it an anti-pattern. Here's another one: "I call this the pointless assignment pattern: x = x; I see this being done all the time. So stupid. I think I will write a blog about it." – Martin Maat May 18 '18 at 16:43
  • 4
    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, that's exactly what I would do. Solving the race condition is more important than some notion of "exceptions as control flow," and it solves it elegantly and cleanly. – Robert Harvey May 18 '18 at 17:13
  • 1
    What does it do if neither of the files meets the given criteria? Does it return null? You can usually use LINQ to clean up code that looks like the code your example: return Directory.GetFiles(".").FirstOrDefault(fileName => fileName.Equals("desktop.ini", StringComparison.OrdinalIgnoreCase))?.Select(fileName => new StreamReader(filename)); Notice the ?. operator between the two LINQ calls. Also people may argue that object creation like this isn't the most appropriate use of LINQ, but I think it's okay here. This isn't an answer to your question, but does digress on one part of it. – Panzercrisis May 18 '18 at 21:03
7

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.

  • 1
    @Flater, I tend to agree that it's not ideal (though I dispute it's an example of the anti-pattern that that answer talks of). Code should show its intent, not the mechanics. So I'd favour something like Scala's Try, eg return Try(() => new StreamReader("desktop.ini")).OrElse(null);, but C# doesn't support that construct (without using a 3rd party library), so we have to work with the clunky version. – David Arno May 18 '18 at 16:31
  • 1
    @Flater, I completely agree that exceptions should be exceptional. But they rarely are. For example, a file not existing is hardly exceptional, yet File.Open will throw one if it can't find the file. Since we already have an unexceptional exception being thrown, trapping it as part of the control flow is a necessity (unless one wants to simply let the app crash). – David Arno May 18 '18 at 16:41
  • 1
    @Flater: the second code sample is the right way to open the file. Anything else introduces a race condition. Even if you check for the files existence, between that check and when you actually open it, something could render that file unavailable to you, and the Open() call will explode. So you have to be ready for the exception anyway. So you might as well just not bother checking and just try and open it. Exceptions are tools to help us get stuff done and their usage should not be viewed as religious dogma. – whatsisname May 18 '18 at 16:58
  • 1
    @Flater: any way to avoid try/catch around file operations introduces race conditions. The filesystem you want to write to can become unavailable the instant before you call File.Create, rendering any checking invalid. – whatsisname May 18 '18 at 17:06
  • 1
    @Flater "You're effectively arguing that every method call needs a return type ... which is effectively trying to reinvent exceptions in a different way". Correct. Though that reinvention is not mine. It's been used in functional languages for years. They generally avoid the whole "exceptions as control flow issue" (which you confusingly claim is an anti-pattern in one breath and then argue in favour of in the next) by using union types, eg in this case we'd use a Maybe<Stream> type that returns nothing if the file doesn't exist and a stream if it does. – David Arno May 18 '18 at 17:32
1

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.

0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy