Is this defensive code or are there possibilities that it could encouter problems?

Question

I've got the following code which makes a connection to a db > runs a stored proc > and then moves on.

I believe it is easy to get db programming wrong so it is important to be defensive: is the following defensive? (or can it be improved?)

public int RunStoredProc()
{
SqlConnection conn = null;
SqlCommand dataCommand = null;
SqlParameter param = null;
int myOutputValue;

try
{
    conn = new SqlConnection(ConfigurationManager.ConnectionStrings["IMS"].ConnectionString);                  
    conn.Open();
    dataCommand = conn.CreateCommand();
    dataCommand.CommandType = CommandType.StoredProcedure;
    dataCommand.CommandText = "pr_blahblah";
    dataCommand.CommandTimeout = 200; //seconds
    param = new SqlParameter();
    param = dataCommand.Parameters.Add("@NumRowsReturned", SqlDbType.Int);
    param.Direction = ParameterDirection.Output;
    dataCommand.ExecuteNonQuery();
    myOutputValue = (int)param.Value;

    return myOutputValue;
}
catch (SqlException ex)
{
    MessageBox.Show("Error:" + ex.Number.ToString(), "Error StoredProcedure");
    return 0;
}
finally
{
    if (conn != null)
    {
        conn.Close();
        conn.Dispose();
    }
}
}

CODE NOW LOOKS LIKE THE FOLLOWING

I've tried to use all the help offered by everyone and the above code has now been amended to the following which I hope is now sufficiently defensive:

public SqlConnection CreateConnection()
{
    SqlConnection conn = new SqlConnection(ConfigurationManager.ConnectionStrings["IMS"].ConnectionString);
    return conn;
}
public int RunStoredProc()
{
    using (var conn = CreateConnection())
    using (var dataCommand = conn.CreateCommand()) 
    {
            conn.Open();
            dataCommand.CommandType = CommandType.StoredProcedure;
            dataCommand.CommandText = "pr_BankingChargebacks";
            dataCommand.CommandTimeout = 200; //5 minutes
            SqlParameter param = new SqlParameter();
            param = dataCommand.Parameters.Add("@NumRowsReturned", SqlDbType.Int);
            param.Direction = ParameterDirection.Output;
            dataCommand.ExecuteNonQuery();
            int myOutputValue = (int)param.Value;

            return myOutputValue;

    } 
}

looking for problems as I've had to create a whole new project; the old one seems to have a bug in it that I can't find.. like something is going on in the background; even in the .cs code files if I want to scroll up and down everything is delayed sort of dragging — whytheq, Jul 27 '12 at 11:53
I also have issues with `MessageBox.Show`; doesn't that halt the thread until the user clicks "OK" keeping the connection open indefinitely if the user happened to go away from keyboard? — Chris Sinclair, Jul 27 '12 at 11:53
@ChrisSinclair +1 other people have also mentioned this and I'm now dealing with it — whytheq, Jul 27 '12 at 12:07

score 7 · Accepted Answer · answered Jul 27 '12 at 11:49

7

Try using, well, the using construct for such things.

using(var conn = new SqlConnection(ConfigurationManager.ConnectionStrings["IMS"].ConnectionString)
{
}

Once you do that, I think you will be at the right level of "defense". Similarly try to do the same for anything that has to be disposed ( like the command)

answered Jul 27 '12 at 11:49

manojlds

290,304
63
469
417

3

Indeed (I would also highlight that the command needs to be disposed too) – Marc Gravell Jul 27 '12 at 11:51
thanks - will this deal with the command object aswell? I assume the command object can only exist within the scope of the `using` so will be collected aswell? – whytheq Jul 27 '12 at 11:54
@marc Gravell ...so have a seperate `using` nested within the above for the command object? – whytheq Jul 27 '12 at 11:55
@whytheq - You can have a similar using statement inside the outer one. – manojlds Jul 27 '12 at 11:55

score 4 · Answer 2 · edited Aug 08 '16 at 16:14

4

There is no need to call both .Close() and .Dispose()
Prefer the using block instead of try-finally
Dispose of the command object
I would remove the catch clause. It doesn't belong here (though YMMV).

If you are going to write this code all over the place, stop. At least create a small helper class to do this, or use a light-weight 'ORM' like Massive, Dapper or PetaPoco. For an example of an ADO.Net helper class, see https://github.com/jhgbrt/yadal/blob/master/Net.Code.ADONet.SingleFile/Db.cs.

edited Aug 08 '16 at 16:14

Caleb Bell

520
6
23

answered Jul 27 '12 at 11:53

jeroenh

26,362
10
73
104

+1 thanks; I'll add my attempt at the correct code - with what I think is a helper method in the OP – whytheq Jul 27 '12 at 14:02

score 3 · Answer 3 · answered Jul 27 '12 at 11:53

3

The main thing I would notice is a MessageBox in database-access code. I can't think of a single scenario that is useful. Just let the exception rise. Don't catch that.

As a general template:

using(var conn = CreateConnection())
using(var cmd = conn.CreateCommand())
{
    // setup cmd and the parameters
    conn.Open();
    cmd.ExecuteNonQuery();
    // post-process cmd parameters (out/return/etc)
}

Note: no Close(), no catch; all the finally are handled by the using. Much simpler; much harder to get wrong.

Another thing to emphasise is the use of a factory method for creating the connection; don't put:

new SqlConnection(ConfigurationManager.ConnectionStrings["IMS"].ConnectionString)

into every method; after all... that could change, and it is unnecessary repetition.

answered Jul 27 '12 at 11:53

Marc Gravell

1,026,079
266
2,566
2,900

nice one for the help...as a newbie I'm unsure of what you mean by "Another thing to emphasise is the use of a factory method for creating the connection" - how do I do this? – whytheq Jul 27 '12 at 12:02
ok - so this line of yours `var conn = CreateConnection())`is what you are describing by "factory method" ? – whytheq Jul 27 '12 at 12:07
@whytheq by writing a method such as `CreateConnection`, and calling it from the various places that currently do `new SqlConnection(...)` – Marc Gravell Jul 27 '12 at 12:07
@whytheq if you wrap it up in a method such as `CreateConnection()`, then yes – Marc Gravell Jul 27 '12 at 12:07
so it'd be something like `Public SqlConnection CreateConnection() {SqlConnection conn = new SqlConnection(ConfigurationManager.ConnectionStrings["IMS"].ConnectionString); return conn;}` – whytheq Jul 27 '12 at 12:12
when you say "Just let the exception rise" how is this done if I do away with the `catch` ... are you saying just test until no exceptions occur, then put app into production and if a bug happens then just let it happen? – whytheq Jul 27 '12 at 17:33

score 1 · Answer 4 · answered Jul 27 '12 at 11:52

1

If MessageBox.Show("Error:" + ex.Number.ToString(), "Error StoredProcedure"); is how you're going to handle an exception then you're not logging or even retrieving the actual exception details.

answered Jul 27 '12 at 11:52

ForkandBeard

874
10
28

score 1 · Answer 5 · answered Jul 27 '12 at 11:53

In addition to manojlds's advice I recommend that you make yourself some reusable helper methods to call the database. For example, make yourself a method that reads the connection string, creates the connection and opens it. Don't repeat infrastructure stuff everywhere.

You can do the same for invoking an sproc or a command text.

Is this defensive code or are there possibilities that it could encouter problems?

5 Answers5

Linked