CODEWITHSUNDEEP
c#try-catchusing
AgentFire
AgentFire

Reputation: 9780

True coding convention about usings/try/catches

Let's assume i got this code:

internal static bool WriteTransaction(string command)
{
    using (SqlConnection conn = new SqlConnection(SqlConnectionString))
    {
        try
        {
            conn.Open();

            using (SqlCommand cmd = new SqlCommand(command, conn))
               cmd.ExecuteNonQuery();
        }
        catch { return false; }
    }

    return true;
}

Well, i have placed conn's using outside the try/catch clause because SqlConnection's constructor will not throw any exception (as it says). Therefore, conn.Open() is in the clause as it might throw some exceptions.

Now, is that right coding approach? Look: SqlCommand's constructor does not throw exceptinos either, but for the code reduction i've placed it along with cmd.ExecuteNonQuery() both inside the try/catch.

Or,

maybe this one should be there instead?

internal static bool WriteTransaction(string command)
{
    using (SqlConnection conn = new SqlConnection(SqlConnectionString))
    {
        try { conn.Open(); }
        catch { return false; }

        using (SqlCommand cmd = new SqlCommand(command, conn))
            try { cmd.ExecuteNonQuery(); }
            catch { return false; }
    }

    return true;
}

(sorry for my english)

Upvotes: 0

Views: 1013

Answers (6)

Mitch Wheat
Mitch Wheat

Reputation: 300559

Unless you can handle the exception in some meaningful way, do not catch it. Rather, let it propagate up the call-stack.

For instance, under what conditions can a SqlCommand ExecuteNonQuery() throw exceptions? Some possibilities are sql query that is improperly formed, cannot be executed or you've lost connection to the database server. You wouldn't want to handle these all the same way, right?

One exception you should consider handling is the SQLException deadlock (erro number 1205).

As was pointed out in a comment, at the very minimum you should be logging exceptions.

[BTW, WriteTransaction() is probably a poor name for that method, given the code you have shown.]

Upvotes: 2

Tim Murphy
Tim Murphy

Reputation: 4932

Given your method 'WriteTransaction' is swallowing all exceptions and returning the success of the transaction, or not, then I would use one try/catch block around the entire method.

internal static bool WriteTransaction(string command) {
    try {
        using (SqlConnection conn = new SqlConnection(SqlConnectionString)) {
            conn.Open();

            using (SqlCommand cmd = new SqlCommand(command, conn)) {
                cmd.ExecuteNonQuery();
            }
        }
    }
    catch { 
        return false; 
    }
    return true;
}

Now you should ask yourself if catching all exceptions and returning true/false is the right thing to do. When in production how are you going to diagnose and fix errors if you haven't at least logged the exception?

Upvotes: 0

Thomas Weller
Thomas Weller

Reputation: 11717

Your first approach is the preferable one, for a very simple reason: The two versions of the method are identical in terms of behavior, but the first one needs less code.

Sometimes it's as simple as counting lines of code ;-)...

HTH!

P.S. I wouldn't have a try/catch statement in this method at all, but I would place it on a higher level, where this internal method is invoked.

Upvotes: 0

Jeremy McGee
Jeremy McGee

Reputation: 25200

Your first code sample, with a single try-catch block, is equivalent to the second. The first one is, however, easier to read and shorter.

It's worth bearing in mind that the usual C# coding approach is not to catch exceptions except at the very top layer of your code.

A well-written reference on this is here: http://www.codeproject.com/KB/architecture/exceptionbestpractices.aspx.

In your case this will simplify your code: instead of returning a bool to indicate that the method succeeded or failed, then testing for this, you "assume for the best" by making the method void and merely handle unexpected exceptions at a top level exception handler.

Exception handling for writing to databases can be considered a slight exception to this general rule: but my personal approach would be to trap specifically for concurrency issues and if that happens retry a few times.

Upvotes: 1

dantuch
dantuch

Reputation: 9283

with good catch clause (exceptions logging) you don't realy need 2x try block, 1 is enough

... your second approach is wrong - there's no need to try catch openning connection - becouse even if you catch this you are done anyway - you can't execute query. In your catch block you could try to "repair" that problem, but what would it give you - while loops in catch ?

Upvotes: 0

Marco
Marco

Reputation: 57573

I think your first solution is better because if you get an error establishing connection is not useful trying to execute a command on that connection.
So with first solution if you get an error you go directly do catch block and then dispose objects, while with the second you go through two exceptions, overloading memory useless.

Upvotes: 0

Related Questions