Replacing thrown exception with more descriptive exception during file I/O

Question

My code needs to open a specific file. Under certain (expected, but rare) conditions, the code will get handed a file path to a file that does not exist -- the path is expected to be otherwise valid though.

If I get handed a path to a file that does not exist, this is an explicit failure scenario that could be reported to the logfile differently, with a more explicit exception message, thereby probably increasing the chance of analysing the error appropriately.

I have solved this by catching the thrown IOException and checking its type. If it is a FileNotFoundException or DirectoryNotFoundException I throw an InvalidOperationException with a message relating to how my code runs.

Here are the relevant parts from my code:

    public MyUpdater(...)
    {
        _reader = ...;
        try {
            // Open immediately to ensure we can read the file:
            _reader.AccessForReading();
        } catch (IOException ioex) {
            var extype = ioex.GetType();
            if (extype == typeof(FileNotFoundException) || extype == typeof(DirectoryNotFoundException)) {
                // This is an explicit failure scenario in which case we do not want the raw IOException to escape
                throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ioex);
            } else {
                // re-throw all other
                throw;
            }
        }

Essentially, I'd like to replace the exception message with the likely reason for the error, so that support personnel looking at the logfile can quickly identify the problem.

Does this solution make sense?

Note:

• This is not example code. It's actually copied verbatim out of the source, that's the very reason for the ellipsis, because that stuff there isn't relevant for the question.

• By "injecting" I do mean what you see. The exception message (the one above is made up for posting it here) just gives a hint as to what may be the likely cause. This message should end up being the first related entry in the log file, thereby giving support personnel a first hint as to what may have gone wrong.)

• One point that I'm not sure about specifically is using InvalidOperationException -- Since it's only for logging, the exception type is really not all that important, but should one create a separate exception type for such a scenario?

Answer 1

You should use is to check the exception type:

try 
{
    // Open immediately to ensure we can read the file:
    _reader.AccessForReading();
} 
catch (IOException ioex) 
{
    if (ioex is FileNotFoundException || ioex is DirectoryNotFoundException)    
    {
        throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ioex);
    } 

    throw;
}

Braces should be on a new line.
Remove redundant else.
Don't make obvious comments about the code. I know that throw will rethrow any exceptions not handled by the above code.

I'd also lean towards a custom exception but I have used InvalidOperationException for this kind of thing myself and I've certainly used programs that have thrown it in similar circumstances. I feel like it's supposed to be more to do with the current object's state rather than outside factors.

Answer 2

Give your code some room to breathe with a few new lines, they're free.

Before:

public MyUpdater(...)
{
    _reader = ...;
    try {
        // Open immediately to ensure we can read the file:
        _reader.AccessForReading();
    } catch (IOException ioex) {
        var extype = ioex.GetType();
        if (extype == typeof(FileNotFoundException) || extype == typeof(DirectoryNotFoundException)) {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ioex);
        } else {
            // re-throw all other
            throw;
        }
    }
}

After:

public MyUpdater(...)
{
    _reader = ...;

    try 
    {
        // Open immediately to ensure we can read the file:
        _reader.AccessForReading();
    } 
    catch (IOException ioex) 
    {
        var extype = ioex.GetType(); 

        if (extype == typeof(FileNotFoundException) || extype == typeof(DirectoryNotFoundException)) 
       {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ioex);
        } 
        else 
        {
            // re-throw all other
            throw;
        }
    }
}

Secondly, catching, filtering and then rethrowing seems wasteful here, why not just catch the explicit exceptions you hope to catch?

public MyUpdater(...)
{
    _reader = ...;

    try 
    {
        // Open immediately to ensure we can read the file:
        _reader.AccessForReading();
    } 
    catch (FileNotFoundException ex) 
    {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ex);
    }
    catch (DirectoryNotFoundException ex) 
    {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ex);
    }
}

Granted, this breaks DRY a little, but it is significantly easier to read. You can reduce duplication further by extraction your exception creation to another method.

public MyUpdater(...)
{
    _reader = ...;

    try 
    {
        // Open immediately to ensure we can read the file:
        _reader.AccessForReading();
    } 
    catch (FileNotFoundException ex) 
    {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw CreateUpdateFileMissingException(ex);
    }
    catch (DirectoryNotFoundException ex) 
    {
            // This is an explicit failure scenario in which case we do not want the raw IOException to escape
            throw CreateUpdateFileMissingException(ex);
    }
}

public InvalidOperationException CreateUpdateFileMissingException(Exception ex)
{
    return new InvalidOperationException("File to update does not exist! It may have been moved because of foobar reason xyz ...", ex)
}

On the other hand this bonus is negated if there are a lot of exception types you want to catch, so if that list grows, then sticking with your filtering method may be the better course of action.

As for what exception you should be throwing here, it appears that the error is that the file cannot be found, so what's wrong with a FileNotFoundException with a different message?

Lastly, the comment // re-throw all other is simply describing what your code is doing. That is your code's job. Comments should explain why you have made certain choices. If the code is too complex to understand it should be refactored and comments explaining what code does should only be considered in extreme circumstances where refactoring is a poor choice.