http://codereview.stackexchange.com/questions/44991/generic-class-to-call-table-parametered-stored-procedures

Generic class to call table parametered stored procedures

up vote4down votefavorite 1

This code is to allow calling any stored procedure with one user defined table as the parameter. I usually use Entity Framework, but I just don't need it for my current solution, so I am rolling my own connection here: public class GenericDataModel {     public GenericDataModel(string connectionString)     {         this.connectionString = connectionString;     }     /// <summary>     /// Connection string for the database     /// </summary>     private readonly String connectionString;     /// <summary>     /// Calls a stored procedure with a single table as the parameter     /// </summary>     /// <param name="storedProcedureName">Name of the stored procedure to call (ie integration.UpsertTestOrderTrackingNum)</param>     /// <param name="parameterName">Name of the parameter (ie "@TestOrderTrackingNumObjects")</param>     /// <param name="sprocParamObjects">Parameter for the sproc</param>     /// <param name="tableParamTypeName">name of the table valued parameter.  (ie. integration.TestOrderTrackingNumTableType)</param>     /// <param name="connection">The connection to use.  This is optional and is there to allow transactions.</param>     public void ExecuteTableParamedProcedure<T>(string storedProcedureName, string parameterName, string tableParamTypeName, IEnumerable<T> sprocParamObjects, SqlConnection connection = null)     {         // If we don't have a connection, then make one.         // The reason this is optionally passed in is so we can do a transaction if needed.         bool connectionCreated = false;         if (connection == null)         {             connection = new SqlConnection(connectionString);             connection.Open();             connectionCreated = true;         }         // Create the command that we are going to be sending         using (SqlCommand command = connection.CreateCommand())         {             command.CommandText = storedProcedureName;             command.CommandType = CommandType.StoredProcedure;             SqlParameter parameter = command.Parameters.AddWithValue(parameterName, CreateDataTable(sprocParamObjects));             parameter.SqlDbType = SqlDbType.Structured;             parameter.TypeName = tableParamTypeName;             // Call the sproc.             command.ExecuteNonQuery();         }         // if we made the connection then we need to clean it up         if (connectionCreated)             connection.Close();     }     /// <summary>     /// Calls a list of sprocs in a transaction.     /// Example Usage: CallSprocsInTransaction(connection=>model.SprocToCall(paramObjects, connection), connection=>model.Sproc2ToCall(param2Objects, connection...);     /// </summary>     /// <param name="sprocsToCall">List of sprocs to call.</param>     public void CallSprocsInTransaction(params Action<SqlConnection>[] sprocsToCall)     {         // Create a new connection that will run the transaction         using (SqlConnection connection = new SqlConnection(connectionString))         {             // Create a transaction to wrap our calls in             var transaction = connection.BeginTransaction();             try             {                 // Call each sproc that was passed in.                 foreach (var action in sprocsToCall)                 {                     // We send the connection to the action so that it will all take place on the same connection.                     // If we don't then if we do a rollback, the rollback will be for a connection that did not run the sprocs.                     action(connection);                 }             }             catch (Exception e)             {                 // If we failed then roll back.                 // The idea here is that the caller wants all the sprocs to succeed or none of them.                 transaction.Rollback();                 throw;             }             // If everything was good, then commit our calls.             transaction.Commit();         }     }     /// <summary>     /// Create the data table to be sent up to SQL Server     /// </summary>     /// <typeparam name="T">Type of object to be created</typeparam>     /// <param name="sprocParamObjects">The data to be sent in the table param to SQL Server</param>     /// <returns></returns>     private static DataTable CreateDataTable<T>(IEnumerable<T> sprocParamObjects)     {         DataTable table = new DataTable();         Type type = typeof (T);         PropertyInfo[] properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);         foreach (PropertyInfo property in properties)         {             table.Columns.Add(property.Name, property.PropertyType);         }         foreach (var sprocParamObject in sprocParamObjects)         {             var propertyValues = new List<object>();             foreach (PropertyInfo property in properties)             {                 propertyValues.Add(property.GetValue(sprocParamObject, null));                              }                       table.Rows.Add(propertyValues.ToArray());             Console.WriteLine(table);         }         return table;     } } Looking for any resource leaks or other missed/hidden issues. I have tested it and it works fine for normal calls. I have not been able to test the Transaction method yet. Example useage: public void UpsertMethod(List<ITypeThatHasTheSamePropertiesAsTheTbleType> rows,                          SqlConnection connection = null) {     ExecuteTableParamedProcedure("schema.UpsertSproc", "@UpsertParam",                                  "schema.UpsertTableType", rows, connection); } Example Transaction Usage: public void CallInTransaction(List<ITypeThatHasTheSamePropertiesAsTheTbleType> firstObjects,                               List<ISecondObjects> secondObjects) {    CallSprocsInTransaction(connection => UpsertMethod(firstObjects, connection),                            connection => UpsertSecondObjects(secondObjects, connection)); } c# sql shareimprove this question edited Mar 21 '14 at 17:22 asked Mar 21 '14 at 17:16 Vaccano 18528

add a comment

3 Answers

activeoldestvotes

up vote5down voteaccepted	Naming On the whole, your naming is just fine. I wonder, though, why you called your class GenericDataModel? What makes it generic? Can't it be called DataModel? Comments Since your method naming and parameter naming are descriptive - most of your comments are completely redundant - I don't need a comment to tell me that CallSprocsInTransaction call sprocs in transaction, or that sprocsToCall is a list of sprocs to call... These comments just add clutter and nothing else. This also goes for inline comments, which tell us trivial things like you are creating the command you are going to send. Potential leak - not using using When you create the connection inside the method, you Close it at the end of the method, but you do not count for possible Exceptions inside the method. In such a case - the connection will not be closed. To prevent it you can use try...finally, but you can also take advantage of the fact that usingsupports null objects: public void ExecuteTableParamedProcedure<T>(string storedProcedureName, string parameterName, string tableParamTypeName, IEnumerable<T> sprocParamObjects, SqlConnection connection = null)     {         SqlConnection adHocConnection = null;         if (connection == null)         {             connection = new SqlConnection(connectionString);             connection.Open();             adHocConnection = connection;         }         using (adHocConnection)         {             using (SqlCommand command = connection.CreateCommand())             {                 command.CommandText = storedProcedureName;                 command.CommandType = CommandType.StoredProcedure;                 SqlParameter parameter = command.Parameters.AddWithValue(parameterName, CreateDataTable(sprocParamObjects));                 parameter.SqlDbType = SqlDbType.Structured;                 parameter.TypeName = tableParamTypeName;                 command.ExecuteNonQuery();             }         }     } } shareimprove this answer answered Mar 21 '14 at 17:55 Uri Agassi 5,9421744
	Thank you for your comments! The only problem with the using is that if it is in a transaction, then I don't want to close the connection. However, I will add in the catch block. –  Vaccano  Mar 21 '14 at 20:53 2 Read again - if the connection was not opened by the method - adHocConnection is null, so the connection won't be closed. –  Uri Agassi Mar 21 '14 at 21:55 You are right! I missed the null check. –  Vaccano  Mar 24 '14 at 18:34 add a comment
up vote5down vote	/// <summary> /// Calls a stored procedure with a single table as the parameter /// </summary> /// <param name="storedProcedureName">Name of the stored procedure to call (ie integration.UpsertTestOrderTrackingNum)</param> /// <param name="parameterName">Name of the parameter (ie "@TestOrderTrackingNumObjects")</param> /// <param name="sprocParamObjects">Parameter for the sproc</param> /// <param name="tableParamTypeName">name of the table valued parameter.  (ie. integration.TestOrderTrackingNumTableType)</param> /// <param name="connection">The connection to use.  This is optional and is there to allow transactions.</param> public void ExecuteTableParamedProcedure<T>(string storedProcedureName, string parameterName, string tableParamTypeName, IEnumerable<T> sprocParamObjects, SqlConnection connection = null) This is probably highly arguable, but I don't like C# optional parameters. The language supports method overloading, which produces methods that are more focused/cohesive. Thus, I would consider: /// <summary> /// Calls a stored procedure with a single table as the parameter. /// </summary> public void ExecuteTableParamedProcedure<T>(string storedProcedureName,                                             string parameterName,                                             string tableParamTypeName,                                             IEnumerable<T> sprocParamObjects) {     using (var connection = new SqlConnection(this.connectionString))     {         connection.Open();         ExecuteTableParamedProcedure(storedProcedureName,                                      parameterName,                                      tableParamTypeName,                                      sprocParamObjects,                                      connection); // multiline to avoid side-scrolling     } }   /// <summary> /// Calls a stored procedure with a single table as the parameter, /// using the specified connection. /// </summary> /// <remarks> /// Useful when executing a stored procedure within a transaction. /// </remarks> public void ExecuteTableParamedProcedure<T>(string storedProcedureName,                                             string parameterName,                                             string tableParamTypeName,                                             IEnumerable<T> sprocParamObjects,                                             SqlConnection connection) {    // do your thing, you *do* have a connection, and don't need to care about cleaning up. } This way the overload that does take a connection parameter can do away with bool connectionCreated and the comments that explain why a connection needs to be created and closed (what about disposed?). The overload that does not take a connection parameter wraps it in a using block, so you're always sure it gets disposed correctly. Naming I think there's possibly a typo in the method's name: ExecuteTableParamedProcedure doesn't look right. Either Parameterized, or the commonly recognized (and used elsewhere) Param - Paramedmakes me wonder where the ambulance is. Comments // Create the command that we are going to be sending and // Call the sproc. These comments say nothing that the code doesn't say already. They should be removed. shareimprove this answer answered Mar 21 '14 at 17:49 Mat's Mug♦ 39.2k385277
	add a comment

up vote4down vote

Looking for any resource leaks or other missed/hidden issues. Have a using statement for your var transaction instance: because SqlTransactionimplements IDisposable (see link and link).