tags:

views:

737

answers:

11

This question is close to what I want to do, but not quite there.

Is there a way to simplify the following code?

private bool ValidDirectory(string directory)
{
    if (!Directory.Exists(directory))
    {
        if (MessageBox.Show(directory + " does not exist. Do you wish to create it?", this.Text) 
            == DialogResult.OK)
        {
            try
            {
                Directory.CreateDirectory(directory);
                return true;
            }
            catch (IOException ex)
            {
                lblBpsError.Text = ex.Message;
            }
            catch (UnauthorizedAccessException ex)
            {
                lblBpsError.Text = ex.Message;
            }
            catch (PathTooLongException ex)
            {
                lblBpsError.Text = ex.Message;
            }
            catch (DirectoryNotFoundException ex)
            {
                lblBpsError.Text = ex.Message;
            }
            catch (NotSupportedException ex)
            {
                lblBpsError.Text = ex.Message;
            }
        }
    }

    return false;
}

It seems a waste, and if I later want to change how I report an error back to the user, or perhaps I want to log these errors, or whatever, then I've got to change 5 different catch blocks. Am I missing something, or is this blatantly against code-reuse?

Am I just trying to be (too) lazy?

A: 

You can do

ex.GetType()

see http://msdn.microsoft.com/en-us/library/system.exception.gettype.aspx

EDIT

try            
{                
    Directory.CreateDirectory(directory); 
    return true;           
}            
catch (Exception ex)            
{   switch(ex.GetType())
         case .....
         case ..........
    blBpsError.Text = ex.Message;            
}
Polo
That'd require catching `Exception`, but yes, that's one solution.
Matthew Scharley
Do a switch on the type and default can just throw it again?
qui
You'd catch Exception, but then you could always throw it again if it's not a type that you're handling.
Keith
The best solution for me would be to have a ExceptionCustom class and to do the type there so it wouldn't offuscate the code...
Polo
Id' add default: throw; to that case statement
Keith
You can't do switch on ex.GetType(): the exression must have an implicit conversion to one of the following types: sbyte, byte, short, ushort, int, uint, long, ulong, char, string. This is not the case for Type. You *can* use `switch (ex.GetType().Name)`, but then you lose type safety on the other hand...
Fredrik Mörk
+8  A: 

If the exceptions share a common super-class then you can just catch the superclass.

Benedict Cohen
They don't. Or rather, they don't share a common superclass that no other possible exception I want to handle doesn't.
Matthew Scharley
A: 

You can catch a base class exception (all your exceptions derive from SystemException):

try
{
  Directory.CreateDirectory(directory);
  return true;
}
catch (SystemException ex)
{
  lblBpsError.Text = ex.Message;
}

But then you may end up catching exceptions you don't want to catch.

Martin Liversage
The problem here is, the `ArgumentException` s are `SystemException` s too, and I specifically don't want to catch those, because that's my issue, not the users.
Matthew Scharley
Then you just add a catch branch which catches ArgumentExceptions prior to the branch which catches Exception (or does the compiler optimise try catches?)
RobV
It runs them in order. I'm fairly sure that much atleast doesn't get optimised. But then you get back to having three catch statements that purely `throw;`
Matthew Scharley
+14  A: 

You can use :

catch (SystemException ex)
{
  if(    (ex is IOException)
      || (ex is UnauthorizedAccessException )
// These are redundant
//    || (ex is PathTooLongException )
//    || (ex is DirectoryNotFoundException )
      || (ex is NotSupportedException )
     )
       lblBpsError.Text = ex.Message;
    else
        throw;
}
Lotfi
This might be simplified a bit - remove DirectoryNotFoundException and PathTooLongException because (ex is IOException) will return true for them
Alex
No it won't. The `is` operator is very exact, it won't return true for subclasses, only that exact class. I thought this too, then tested it in LINQPad.
Matthew Scharley
Matthew, the following test code returns true. I'm not sure why you've got the different result...try { throw new DirectoryNotFoundException(); }catch (Exception ex) { return (ex is IOException);}
Alex
Hrrm... You're right, I probably messed up my test (checked if an IOException was a DirectoryNotFoundException in your example). Oops. Ignore me.
Matthew Scharley
One thing I will also say here is that you should probably only catch `SystemException`. Rethrowing isn't the same as not catching it in the first place. Beyond that, FxCop and other similar programs will warn about catching Exception, and the best way to shut them up is to not do it.
Matthew Scharley
+1  A: 

Yes, you're trying to be lazy, but laziness is one of the virtues of a programmer, so that's good.

As for your question: There is no way I am aware of, but there are some workarounds available:

  • Give the Exceptions a common ancestor. I think this won't be possible in your case, since they seem to be builtin.
  • Catch the most generic exception you can.
  • Move the handling code into its own function and call that from each catch block.
soulmerge
For the record, all of them are built-in, and so is the method being called.
Matthew Scharley
Thx, updated answer
soulmerge
+3  A: 

This is annoying, and other answers have suggested good workarounds (I'd use @Lotfi's).

However this behaviour is a requirement given the type-safety of C#.

Suppose you could do this:

try
{
    Directory.CreateDirectory(directory);
    return true;
}
catch (IOException, 
    UnauthorizedAccessException,
    PathTooLongException,
    DirectoryNotFoundException,
    NotSupportedException ex)
{
    lblBpsError.Text = ex.Message;
}

Now what type is ex? They all have .Message because they inherit System.Exception, but try accessing any of their other properties and you have a problem.

Keith
You'd have to use the lowest common denominator. You can only catch derivatives of `Exception`, so that way has no real issues.
Matthew Scharley
So in this case, you'd end up with a SystemException. The compiler can work this out, so type-safety isn't jeapordised either.
Matthew Scharley
The compiler could, and the IDE could too, but it would be somewhat obscured for the developer - what intellisense should they expect on ex? I can see a way something like this could work though - something like the co/contravariance support in C#4
Keith
I can see it now... `catch (in SystemException)`... Except that's exactly what happens already anyway. Perhaps `catch(in<IOException, UnauthorizedAccessException, ...> SystemException)`. Could be fun.
Matthew Scharley
+2  A: 

Its also important to note that when catching more then one type of exception they should be order by most specify to most general. The Exception will find the first one in the list that it matches and throw that error, none of the other errors will be thrown.

Audioillity
I realised after posting this that IOException was a base class for some of these, but it's the first in the list above. Doesn't matter for this code anyway, but a very good point.
Matthew Scharley
A: 

You could use delegates, this will do what you want:

EDIT: simplified a bit

static void Main(string[] args)
{
    TryCatch(() => { throw new NullReferenceException(); }, 
        new [] { typeof(AbandonedMutexException), typeof(ArgumentException), typeof(NullReferenceException) },
        ex => Console.WriteLine(ex.Message));

}

public static void TryCatch(Action action, Type[] exceptions, Action<Exception> catchBlock)
{
    try
    {
        action();
    }
    catch (Exception ex)
    {
         if(exceptions.Any(p => ex.GetType() == p))
         {
             catchBlock(ex);
         }
         else
         {
             throw;
         }
    }
}

Your particular try/catch would be:

bool ret;
TryCatch(
    () =>
        {
            Directory.CreateDirectory(directory);
            ret = true;
        },
    new[]
        {
            typeof (IOException), typeof (UnauthorizedAccessException), typeof (PathTooLongException),
            typeof (DirectoryNotFoundException), typeof (NotSupportedException)
        },
    ex => lblBpsError.Text = ex.Message
);

return ret;
legenden
A: 

I understand some of these exceptions may not be foreseeable but where possible try to implement your own "pre-emptive" logic. Exceptions are expensive, though in this case probably not a deal breaker.

For example, use Directory.GetAccessControl(...) rather than relying on an UnauthorizedAccessException to be thrown.

Neil
Exceptions aren't so expensive so as to be human noticable. You're right, in this case it is most definitely not a deal breaker, and more than likely will never happen, I'd just rather not crash if it does.
Matthew Scharley
+1  A: 

Just for completeness’ sake:

In VB, you could use conditional exception handling:

Try
    …
Catch ex As Exception When TypeOf ex Is MyException OrElse _
                           TypeOf ex Is AnotherExecption
    …
End Try

Such a Catch block would only get entered for the specified exceptions – unlike in C#.

Perhaps a future version of C# will offer a similar feature (after all, there's a specific IL instruction for that code).

MSDN: How to: Filter Errors in a Catch Block in Visual Basic

Konrad Rudolph
A: 

Check out the The Exception Handling Application Block from EntLib. They articulate a very nice policy and configuration based exception handling methodology that avoids large conditional logic blocks.

JP Alioto