views:

77

answers:

3

Looking on some source code I've inherited, there's a snippet of code that calls a SQL stored procedure making an Update.

The stored procedure returns -1 in case something goes wrong:

IF @@Error <> 0
    BEGIN
        ROLLBACK TRAN
        SELECT -1
        RETURN
    END
COMMIT TRAN
SELECT 0

The C# code is something like this:

    System.Data.SqlClient.SqlDataReader myReader;
    try{
        SqlDbConnection.Open();
        SqlDbCommand.Connection = SqlDbConnection;
        SqlDbCommand.CommandType = System.Data.CommandType.StoredProcedure;
        SqlDbCommand.CommandText = "StoredProcedured_UpdateFoo";
        SqlDbCommand.Parameters.Clear();
        SqlDbCommand.Parameters.Add("@FooData", SqlDbType.DateTime); SqlDbCommand.Parameters["@FooData"].Value = System.DateTime.Now.ToString("yyyy-MM-dd");
        myReader = SqlDbCommand.ExecuteReader();   
        if (myReader.Read())
           {
             if (int.Parse(myReader.GetValue(0).ToString()) == -1) throw new ErrorDataTxRx("Error FOO ");
           }
    } finally {
      if (SqlDbConnection.State != ConnectionState.Closed){
           SqlDbConnection.Close();
      }

      if (myReader != null){              
          if (!myReader.IsClosed) myReader.Close();
      }
    }

I also see part of the same code, checking the same thing using System.Data.DataSet() with Fill method.
Is there a more elegant way to check if the returned value is -1?
Is it OK to use an ExecuteReader in this case?

+6  A: 

Have you tried using ExecuteScalar? That's designed for queries which return a single value:

Executes the query, and returns the first column of the first row in the result set returned by the query. Additional columns or rows are ignored

Jon Skeet
-1.... the first row of the result set is something fundamentally different from a return value. SP's actually RETURN A VALUE (numeric) which can be used to indicate error / status information. Check the SP call syntax at http://msdn.microsoft.com/en-us/library/aa258848(SQL.80).aspx
TomTom
@TomTom: While that's certainly true, look at the existing code - it *is* reading the first value of the result set, as would filling a `DataSet`. Assuming that's what the OP wants, I believe `ExecuteScalar` will do the same thing, just more simply.
Jon Skeet
+1 - for not getting confused by the slightly incorrect terminology used by the OP. ;)
Lucero
A: 

Use ExecuteNonQuery instead of the reader and you will get the number of rows affected.

Your question is not clear enough. You may need to do it with a ReturnValue like Lucero has written.

ibram
@ibra check my edit
systempuntoout
+3  A: 

As Jon correctly noted, you're actually not using the return value of the SP, but actually getting the first value of the first row, which is what ExecuteScalar would do in a simpler way.

However, in order to get the return value from the SP (e.g. from something like RETURN @i; in the SP SQL code), you need to add a new Parameter and set its direction to ReturnValue, something like this:

SqlParameter returnValueParam = new SqlParameter();
returnValueParam.DbType = DbType.Int32;
returnValueParam.IsNullable = false;
returnValueParam.Direction = ParameterDirection.ReturnValue;
SqlDbCommand.Parameters.Add(returnValueParam);
// execute SP here...
int returnValue = (int)returnValueParam.Value;
Lucero
@Lucero I've added part of the stored procedure code. As you can see, it's a SELECT -1.
systempuntoout
@systempuntoout thanks for the edit. Since it is about code-reviewing, I'd say that using `SELECT someint` instead of a SP return value is a bad practice and adds unnecessary overhead.
Lucero
@Lucero thanks, you can call it a copy-paste metastasis.
systempuntoout