1
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ You still ought to include the whole complete code; mark the relevant/irrelevant parts as such with comments. That's why you're getting close votes. \$\endgroup\$ – Snowbody Apr 21 '15 at 13:55
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ is is appreciated, thanks! :-) (and could we stop with the braces places, please ;-) ?) \$\endgroup\$ – Martin Ba Apr 21 '15 at 14:53
  • \$\begingroup\$ @MartinBa - I promise I won't mention it again :) but I think the majority of C# developers are more comfortable with brace on new line. Obviously you should do what your team has decided! \$\endgroup\$ – RobH Apr 21 '15 at 15:00
  • \$\begingroup\$ Yeah. I actually can understand, because in C#, braces-on-sparate-line is the default. It's just that our C# code shares with a large body of a large C++ code base, where the brace-on-same-line style is used and it was decided to use the same style in C#. (for better or worse) \$\endgroup\$ – Martin Ba Apr 21 '15 at 18:16
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks. It seems we would have to agree to disagree as to what constitutes readability :-) \$\endgroup\$ – Martin Ba Apr 21 '15 at 12:34
  • \$\begingroup\$ Glad I could help! Out of curiosity, which readability case to your disagree with, spacing or the exception catching technique? \$\endgroup\$ – Nick Udell Apr 21 '15 at 12:35
  • 1
    \$\begingroup\$ I fear I disagree with both. :-) The spacing is just our team guideline nothing I care too strongly about. The catching is something where I do prefer the type switch if the action is exactly the same. Exception filters will help with that though. \$\endgroup\$ – Martin Ba Apr 21 '15 at 12:38
  • 2
    \$\begingroup\$ Adding to what @Snowbody said, if you do this, make sure to add a [DebuggerHidden] attribute to the separate function or the stack trace will include your separate function, which might mess up your logging (and is not the source of the error, so should not be included). \$\endgroup\$ – Nick Udell Apr 21 '15 at 13:58
  • 4
    \$\begingroup\$ @Snowbody - there are definitely times when you need .GetType() and typeof... \$\endgroup\$ – RobH Apr 21 '15 at 14:38

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.