Is it bad to use nested Try..Catch blocks like this?

Question

Is this a bad idea? Is there a better way to achieve the same effect?

// assume that "name" is a string passed as a parameter to this code block
try
{
    MainsDataContext dx = new MainsDataContext();
    try
    {
        Main m = dx.Main.Single(s => s.Name == name);
        return m.ID;
    }
    catch (InvalidOperationException)
    {
        Guid g = Guid.NewGuid();

        Main s = new Main 
        {
            Name = name,
            ID = g
        };

        dx.Mains.InsertOnSubmit(s);
        dx.SubmitChanges();

        return g;
    }
}
catch (Exception ex)
{
    // handle this
}

The objective here is to get the ID of a record if it exists, otherwise create that record and return it's ID.

Answer 1

You should use SingleOrDefault, that way if a record doesn't exist it will return the default value for the class which is null.

MainsDataContext dx = null;    
try
    {
         dx = new MainsDataContext();

        Main m = dx.Main.SingleOrDefault(s => s.Name == name);

        if ( m == null)
        { 
           Guid g = Guid.NewGuid();

           m = new Main 
          {
              Name = name,
              ID = g
          };

         dx.Mains.InsertOnSubmit(m);
         dx.SubmitChanges();

        }

        return m.ID;
    }
    catch (Exception ex)
    {
        // handle this
    }
    finally
    {
       if(dx != null)
          dx.Dispose();
    }

it is a good idea to use the using keyword when using a DataContext

using ( MainsDataContext dx = new MainsDataContext())
{
        Main m = dx.Main.SingleOrDefault(s => s.Name == name);

        if ( m == null)
        { 
           Guid g = Guid.NewGuid();

           m = new Main 
          {
              Name = name,
              ID = g
          };

         dx.Mains.InsertOnSubmit(m);
         dx.SubmitChanges();

        }

        return m.ID;
}

Answer 2

Main m = dx.Main.SingleOrDefault(s => s.Name == name);

if (m == default(Main))
{
    // it does not exist
}
else
{
    // it does exist
}

It is not apparent from the question if the type Main is a class or a struct (EDIT: I just realized that actually it must be a class), hence I used default() instead of just comparing to null.

Answer 3

My question would be what code you intend to put in here:

// handle this

With the first catch block, you know that Single() threw an InvalidOperationException because the sequence contains more than one element or is empty.

In the second, you could get all kinds of errors. Null reference, data access, etc. How are you going to handle these?

Only catch what you know how to handle, and be as specific in the exception type as you can.

Answer 4

Anyways, I think nested Try Catch blocks are good, like sibling Catch blocks each catching a different problem. It's good to be specific about errors. The catch-all should be only a safety net for production.

Answer 5

No, but you might want to refector the inner block into an external method.