Eliminate copy/paste and Factor logic out of foreach loop

I have two conditions that need to traverse the same data but inside the traversal the logic changes slightly for each respective condition and in different places. If it was the same place that would be easy to factor out, but what's the best approach if it's not all in the same place for every condition? I seem to run into this situation often when parsing data.

```
switch(batch.DocumentType.ImportedFileDisposition)
{
case ImportedFileDispositionActions.Delete:
foreach (string documentMetadataFile in documentMetadataPaths)
{
string documentPath = Path.GetDirectoryName(documentMetadataFile);
XDocument metadata = XDocument.Load(documentMetadataFile);
foreach (var pageElement in metadata.Root.Element("Pages").Elements("Page"))
{
string pageName = null;
XAttribute attribute = pageElement.Attribute("FileName");
if (attribute != null)
pageName = attribute.Value;

string pagePath = null;
string originalPagePath = null;
if (!string.IsNullOrWhiteSpace(pageName))
{
pagePath = IOUtility.PathCombine(documentPath, pageName);
File.Delete(pagePath);
}
}
File.Delete(documentMetadataFile);
Directory.Delete(documentPath, true);
}
break;
case ImportedFileDispositionActions.Archive:
foreach (string documentMetadataFile in documentMetadataPaths)
{
string documentPath = Path.GetDirectoryName(documentMetadataFile);
string baseImportDirectory = documentPath;
string archiveDirectory = GenerateArchiveDirectory(batch, documentPath);
XDocument metadata = XDocument.Load(documentMetadataFile);
foreach (var pageElement in metadata.Root.Element("Pages").Elements("Page"))

I'd start by writing another method, containing what's common in both blocks; then I'd extract another method for processing a single file, and in that method I'd decide if I want to archive or just delete the file... and then I'd get annoyed with having a method that does two things and I'd extract some MoveOrCopyToArchive method, and try to delete the file in all cases.

But that's just easy moving stuff around and reducing redundancies.

This is more worrying:

try
{
    IOUtility.MoveFile(pagePath, newArchivePath);
}
catch
{
    try
    {
        IOUtility.CopyFile(pagePath,newArchivePath, true);
    }
    catch (Exception ex)
    {
        throw new IOException("Failed to move or copy file: " + pagePath + " to archive: " + newArchivePath, ex);
    }
}
File.Delete(pagePath);

Nested try/catch blocks have a funky smell, but the worrying line is the last one: what makes you think pagePath can be safely deleted? If something went wrong moving or copying it, my bet is on File.Delete throwing an exception as well - and that one will bubble up your stack, meaning Directory.Delete would never get called... and the entire application might just die a horrible death.

Now back to that nested try/catch - first you attempt to move the file, and if that fails, you try to copy it. There are several reasons for failing to move a file, several of which will also fail to copy it. I think that's a good case for cherry-picking which exceptions will warrant trying to copy, and which ones aren't worth it:

• ArgumentNullException is almost certainly ruled out, you're checking if pageName is null already. But I don't know what IOUtility.PathCombine does, and I have no garantee that GenerateArchiveDirectory returns a valid path either. Better safe than sorry; catching this exception means attempting to copy the same file/path is 100% certain to fail as well.

• ArgumentException or NotSupportedException would be thrown for a path containing characters invalid in a path, or for a path in an invalid format. No need to try copying that file, it's garanteed to fail anyway.

• DirectoryNotFoundException and PathTooLongException would also fail copying.

• IOException would be thrown if the destination file already exists, or if the source filename wouldn't be found. Clearly not worth trying to copy.

• UnauthorizedAccessException - ok, moving can't work, but maybe the file can be copied.

https://msdn.microsoft.com/library/system.io.file.move(v=vs.110).aspx

The catch block that doesn't specify any exception not only enters on any exception, without a handle on the exception object you have lost the original exception that tells you how the first operation - moving the file - failed.

Catching System.Exception would be over-the-top here; as shown above, you'd only really want to try a copy when an UnauthorizedAccessException is thrown - every other exception you can possibly catch there, would be caused by something that would also make the copy fail.

But then if any of these exceptions is caught, my bet is on File.Delete throwing the same exception.

I'll add that throwing your own IOException is confusing here, because the System.IO methods you're using can throw that same exception type, with a different meaning. If you really want to wrap the exception with one of your own, then you have a case for a custom exception type here - a FileArchiveException or something similar.

Why is there no try/catch block in the ImportedFileDispositionActions.Delete case?

I'd do it something like this:

if (fileDisposition == ImportedFileDispositionActions.Archive)
{
    SendToArchive(pagePath, documentMetadataFile, batch);
}

if (File.Exists(pagePath))
{
    File.Delete(pagePath);
}

//Assert.IsFalse(File.Exists(pagePath));

Understanding at a glance what's going on is easier if each idea reads like a sentence.

The nested try/catch can be avoided by extracting a method out of the try block:

private readonly ILogger _logger; // do something about exceptions!

private void SendToArchive(string path, string metadata, FooBar batch)
{
    try
    {
        var archivePath = GenerateArchiveDirectory(path, metadata, batch);
        MoveOrCopyToArchive(archivePath);
    }
    catch(IOException exception)
    {
        _logger.Info(exception);
    }
    catch(Exception exception)
    {
        _logger.Debug(exception);
    }
}

private void MoveOrCopyToArchive(string path)
{
    try
    {
        File.Move(path, ArchivePath);
    }
    catch(UnauthorizedAccessException exception)
    {
        _logger.Warn(exception);
        File.Copy(path, ArchivePath, true);
    }
}

The stack trace will contain enough information to determine if the exception was thrown moving, copy, or deleting the file; there's not enough information in your post to recommend whether exceptions should be handled, rethrown or simply bubbled up - I've made a few assumptions here.

StackExchange Code Review Q#83048, answer score: 5