76

Here is my code:

public void ReadSomeFile(string filePath)
{
    if (!File.Exists(filePath))
        throw new FileNotFoundException();

    var stream = new FileStream(filePath, ....)
    .....
}

Should I throw an exception myself (see the File.Exists check)? FileStream will already throw FileNotFoundException if the the file doesn't exist. What is good programming practice here? Code analysis says that we should validate our parameters. But if I am passing that parameter directly to another method (mine or someone else code) and that method will throw exception itself, then what is advantage of validating argument in my code?

5
  • 25
    There's no point in throwing a FileNotFoundException - in fact, it's just inviting race condition issues. Either you handle the exception, let it propagate, or wrap it in your own exception. This corresponds to "I know what to do with this", "I don't know what to do with this" and "I want to handle this higher on the stack" respectively.
    – Luaan
    Commented Oct 5, 2015 at 14:41
  • Side note: if you really need to validate arguments you could check if filePath looks valid (i.e. absolute path, or at least does not contain Path.GetInvalidFileNameChars()) Commented Oct 5, 2015 at 14:46
  • 2
    @AlexeiLevenkov I think that is not required as well. FileStream will handle that,
    – fhnaseer
    Commented Oct 5, 2015 at 14:47
  • 3
    Related: From Eric Lippert's blog - Vexing exceptions - This case is under "exogenous exceptions".
    – Kobi
    Commented Oct 6, 2015 at 7:46
  • this really depends on what you want to achieve. so there is no "correct answer" to this. it depends
    – swe
    Commented Oct 7, 2015 at 15:14

4 Answers 4

118

if (File.Exists(f)) { DoSomething(f) } (or the negation thereof) is an anti-pattern. The file can be deleted or created in between those two statements, so it makes little sense to check its existence like that.

Apart from that, as pointed out in the comments, while File.Exists() may return true, the actual opening of the file can then still fail for a variety of reasons. So you'll have to repeat the error checking and throwing around the opening of the file.

As you don't want to repeat yourself but instead keep your code DRY, just attempt to open the file and let new FileStream() throw. Then you can catch the exception, and if you wish, re-throw the original or throw an application-specific exception.

Of course calling File.Exists() can be justified, but not in this pattern.

12
  • 3
    @CodeCaster Sure. And some other process can create the file between File.Open(f); throwing and you catching it. The problem is less that the situation could change before you handle it (because that is nearly unavoidable in the failure case), but rather that you should error check once to avoid repetition (and you must error check on the open anyhow). (Also note that, when talking to some file systems, a successful open doesn't mean the file won't be deleted on you: so even there the failure can happen after the open seems to work.) Commented Oct 5, 2015 at 18:15
  • 2
    Besides race condition, this code ignores situation where file exists but is not readable (because of permissions etc.), and possibly even more errors. Commented Oct 5, 2015 at 19:16
  • 8
    While checking for a file's existence and immediately trying to open it would generally be an anti-pattern, I would not consider it an anti-pattern if other logic separated the check and the open. On some networking systems, checking for a file's apparent availability may be cheaper than acquiring the lock necessary to open it. If code won't be able to do anything useful unless multiple files are available, it may be useful to ensure that all the files seem to be available before acquiring the locks necessary to open any of them.
    – supercat
    Commented Oct 5, 2015 at 20:49
  • 3
    Python has a policy named "Easier to Ask for Forgiveness than for Permission" (EAFP). It means that sometimes, due to race conditions and things out of your control, it's best to just ask your program to do the thing it wants to do, and handle all the possible errors there (exceptions) rather than checking at every step if the conditions are optimal. In this example, I think that EAFP can be applied for best practices.
    – sleblanc
    Commented Oct 6, 2015 at 2:51
  • 1
    Also a FileNotFoundException could occur because the user doesn't have permission to see it or the folder doesn't exist...so if you want to help the user you could, in the catch, check for more specific reasons e.g. Folder.Exists - or just ensure your friendly user message says "file cannot be found or accessed"!
    – AndrewD
    Commented Oct 6, 2015 at 23:05
15

Your method is called ReadSomeFile and takes a filename as its input, therefore it is reasonable for it to throw a FileNotFoundException. As you cannot add any value by catching the exception then throwing it yourself, just let .NET throw it.

However if your method was LoadData(databaseName) and it has to access many files, catching the exception, and throwing a custom exception may be of value, as you could add the databaseName to the exception along with other helpful information.

2
  • And even in the last case you should try-catch the inner exception rather than use File.Exists. Commented Oct 6, 2015 at 11:10
  • it's that case when the answer that actually answers the question gets much less upvotes Commented Oct 13, 2015 at 10:55
1

Aside from the already-given answers, you could also say that this depends on what you expect to happen.

If you want to read a log file and it does not exist, do you want to throw an error, or just an empty String (or empty String array)?

If returning a default value (like an empty String) I would simply wrap the content of the function in a try-catch (but only for expected errors) and return the default in the catch block, while returning the actual content in the try block.

This would leave three possible situations:

  1. The content of a file is returned;
  2. The default value is returned, because an expected error occurred;
  3. An error is thrown by .NET, because you did not catch that specific error.
0

Let the correct method try to open the file while you don't have any idea about full file name, somethings like special file names (eg. Device files and UNC paths):

In some cases other file methods may be failed, but opening the file is successful.

Some examples for special file names are:

  • CON
  • NUL
  • COM1, COM2, COM3, COM4
  • \\server\share\file_path
  • \\teela\admin$\system32 (to reach C:\WINNT\system32)
  • C:..\File.txt
  • \\.\COM1
  • %TEMP%
  • and more...
1
  • 2
    I don't know much about Windows, but I'm pretty sure %TEMP% is an environment variable, not a file, and that the "open file" function won't expand it.
    – Functino
    Commented Oct 8, 2015 at 19:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.