1

I'm wondering how better to perform this operation for a large amount of files. The bit I'd like some thoughts on whether this copy/paste is acceptable enough of a tradeoff.

  • try to write a file
  • if the target dir doesn't exist, create it
  • try again to write the file. If something else throws and exception, let it raise.
try{
   File.Copy("@D:\foo.txt", @"C:\mydir\foo.txt");
}
catch (DirectoryNotFoundException){    
   CreateDirectoryForFile(@"C:\mydir");
   File.Copy("@D:\foo.txt", @"C:\mydir\foo.txt"); //copy pasted from the try block
}

There's a simpler block that's easier to read, but leads to a higher number of disk IO calls that is necessary:

CreateDirectoryForFile(@"C:\mydir\foo.txt");
File.Copy("@D:\foo.txt", @"C:\mydir\foo.txt");

Can pattern 1 be improved?

  • 1
    Measure. Don't guess. – Telastyn Jun 22 '14 at 23:36
  • 2
    Are you always writing to the same directory? Why not check if it exists before running the copy? – Daenyth Jun 22 '14 at 23:55
  • @Telastyn I guess teh bulk of the question was about the repetition and copy/pasting of a line of code into the catch block, rather than the perf. – p.campbell Jun 23 '14 at 0:00
  • @p.campbell - in that case, just CreateDirIfNecessary; CopyFile; every time. Less, simpler code. – Telastyn Jun 23 '14 at 0:03
  • Performance wise exceptions are not that expensive. The main problem with unnecessary exceptions is that they make debugging more annoying. – CodesInChaos Jun 23 '14 at 8:29
6

The simple principle is that this is not an Exception per se, it is an expected condition and thus should be checked using if (Directory.Exists(...)) to see if destination directory does not exist (and likewise if (File.Exists(...)) to check if the file already exists in that directory if that is important for your scenario).

You are paying a high performance price for this because exceptions are more costly. As an example, see that when you try to get an item from a Dictionary using an indexer, it throws a KeyNotFoundException if the item did not exist. To avoid having getting and catching this exception, usually people would first check the existence of the item using the Contains method and fetch it only if it existed. So, the Dictionary class thus provided a TryGetValue which will try to fetch the item, but if the item does not exist in the Dictionary, it just returns a false indicating so.

As DeadMG pointed out in the comments, there might be cases where concurrency works out better when you do not check and just perform the operation. An example could be that if your code was only creating the directory, it might be better to just use Directory.Create instead of first checking for existence because the file system state might change between the check and the actual operation.

A coding guideline that I use is that if a method allows one to check for a condition for which an exception may be thrown, consider using that method first, and only rely on exception if there is a strong reason not to check first. There are valid cases, especially over the network where if you checked for the existence of a file and then deleted it, it might incur high latency or bandwidth costs, in which case it might be preferable to just attempt an action and catch an exception. Azure Storage APIs, for example, use such an approach.

Avoid exceptions when a condition is one of the valid possibilities and there is no good reason to prefer exceptions. In your case, it is normal for the directory to not exist, and not an error condition, and you are dealing on the local disk.

EDIT: Updated after DeadMG's comment, my initial response was only focused on the code in the question.

  • 3
    The return values of those methods cannot be trusted, since the filesystem can be mutated by other processes asynchronously. Using them creates a race condition. It's one thing to check them quickly beforehand and another to design your code around the diea that they always produce results you can depend on. – DeadMG Jun 23 '14 at 0:50
  • 1
    @DeadMG: The file system is unsafe for multi-threaded scenarios and there's hardly anything C# or any language can thus do. For example, in the sample code provided, even in the catch block, when the new directory is created, it could be removed/renamed by another process before the file is copied. Whether it was a conditional check or an exception caught, the result is the same. So, I don't understand your concern. – Omer Iqbal Jun 23 '14 at 1:07
  • @DeadMG FYI, my response was to this particular question (and code). If you meant safety in the case of a single operation, then I can definitely see that point (e.g. the code intended to just create a directory), in which case one might as well Directory.Create instead of Directory.Exists and Directory.Create. I updated my response to clarify this & thanks for your feedback! – Omer Iqbal Jun 23 '14 at 1:19
0

This is a violation of DRY. I really would avoid it. Sooner or later something will change and it will bite you.

You could

  1. Refactor the relevant code as a function
  2. Break out the strings as constants
  3. Write a loop that can only allows one retry.

Anything to avoid cut and paste.

Your Answer

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

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