Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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));
}
share|improve this question

3 Answers 3

up vote 5 down vote accepted

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 using supports 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();
            }
        }

    }
}
share|improve this answer
    
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 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 at 21:55
    
You are right! I missed the null check. –  Vaccano Mar 24 at 18:34
/// <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 - Paramed makes 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.

share|improve this answer

Looking for any resource leaks or other missed/hidden issues.

Have a using statement for your var transaction instance: because SqlTransaction implements IDisposable (see link and link).

share|improve this answer
    
Thanks! I missed that! –  Vaccano Mar 24 at 18:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.