tags:

views:

442

answers:

5

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.

+6  A: 

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;
}
Stan R.
What is the dx.Dispose() for? Shouldn't it be automatically disposed by the GC? Should I put this in the `finally` section of all my try..catches using LinqToSql?
Nate Bross
@Nate, when using a DataContext it is a good idea to use it in a using statement **using(DataContext dx = new .......)** this will make sure that the connection is closed as soon as possible and the object is disposed of. Since we are using a try-catch here we need to make sure that no matter what we dispose of our connection. Exception or not.
Stan R.
That just makes sure the connection is closed quickly? Would the GC close it eventually on its own? Now that you pointed it out it makes senes, I just don't understand is it necessary, or just a good idea?
Nate Bross
@Nate, considering that you don't want to keep the connection longer than necessary and you have no idea when GC will pick it up, i would consider it a necessity as well as good practice.
Stan R.
+6  A: 
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.

DrJokepu
This answers the specific case; what about the more general idea of using the nested try..catch blocks when something ~ `SingleOrDefault()` is not available? Is that a bad idea?
Nate Bross
@Nate Bross: Exceptions are for exceptional circumstances. Throwing and catching an exception is a very expensive operation and doesn't make your code very readable. Try to avoid them for normal control flow (such as this) and only use them when there's something exceptional happening in your program.
DrJokepu
@DrJokepu: Should exceptions be used when inserting/updating/deleting with Ling to SQL? Or should it be handled elsewhere?
keyboardP
@TenaciousImpy - Use an exception if there's an error you didn't expect normally to happen. If your insert statement fails because a constraint check fails (e.g. duplicate primary key), that's something the data provider didn't expect so it threw an exception. However, if you think that trying to insert something and then failing will be a regular use case, you might want to avoid exceptions and do the appropriate checks before inserting to the database. The same applies to other database commands as well, of course.
DrJokepu
A: 

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

Shay Erlichmen
Refactor, not Reflector :P
Pierre-Alain Vigeant
+1  A: 

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.

TrueWill
The Catch(Exception ex) is a safty net, its an abbreviated code snippet, that basically does some logging and then `throw;`s the exception to be handled at the presentation layer.
Nate Bross
@Nate - that's perfectly acceptable. Logging + rethrowing is one of the few cases where catching base-class Exception is a good idea.
TrueWill
A: 

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.

Pedro Sobota