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
What is the purpose of the
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
Note that both of these cases would require you to actually process multiple matches and therefore not return a value immediately.
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 FileNotFoundException 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
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.