views:

1237

answers:

5
    private static readonly string dataProvider = ConfigurationManager.AppSettings.Get("Provider");
    private static readonly DbProviderFactory factory = DbProviderFactories.GetFactory(dataProvider);
    private static readonly string connectionString = ConfigurationManager.ConnectionStrings[dataProvider].ConnectionString;
    /// <summary>
    /// Executes Update statements in the database.
    /// </summary>
    /// <param name="sql">Sql statement.</param>
    /// <returns>Number of rows affected.</returns>
    public static int Update(string sql)
    {
        using (DbConnection connection = factory.CreateConnection())
        {
            connection.ConnectionString = connectionString;

            using (DbCommand command = factory.CreateCommand())
            {
                command.Connection = connection;
                command.CommandText = sql;

                connection.Open();
                return command.ExecuteNonQuery();
            }
        }
    }

I need help in re-writing this so that it'll work with stored procedures. (Pass in sproc name and params) Does anyone have any idea how i should go about doing this? Edit: The area I'm having problems with is trying to figure out ways to fill in params.

Thanks

A: 

You could add 3 parameters :

  1. string ShemaName -- Store the name of the database shema
  2. string StoreProcName -- Store the name of the stored procedure
  3. List -- list of your store proc parameter
Drahakar
A: 

Use an overload such as

public static int Update(string storedProc, Dictionary<string,string> params)
{
    ...
}
benPearce
+7  A: 

You already need parameters independent of whether you're implementing stored procedures.

Right now, your code could be called with a query like SELECT * FROM Table WHERE ID = @ID, in which case, you already need to pass a Dictionary<string,object> params. Have your code fill in the Parameters collection of the command you already have, and test that, before worrying about stored procedures.

Once that works, you should simply create an overload that accepts a bool that says this is a stored procedure, then use it to set the CommandType property of the command.


Edit: Here's How I Would Refactor It

Step 1: Generalize Update

There's nothing special about the Update method that prevents it being used for other non-query operations, aside from the name. So:

    /// <summary>
    /// Executes Update statements in the database.
    /// </summary>
    /// <param name="sql">Sql statement.</param>
    /// <returns>Number of rows affected.</returns>
    public static int Update(string sql)
    {
        return NonQuery(sql);
    }

    public static int NonQuery(string sql)
    {
        using (DbConnection connection = factory.CreateConnection())
        {
            connection.ConnectionString = connectionString;

            using (DbCommand command = factory.CreateCommand())
            {
                command.Connection = connection;
                command.CommandText = sql;

                connection.Open();
                return command.ExecuteNonQuery();
            }
        }
    }

Step 2: What About Parameters?

Your current code couldn't even handle UPDATE queries that used parameters, so let's start fixing that. First, make sure it still works if no parameters are specified:

    public static int NonQuery(string sql)
    {
        Dictionary<string, object> parameters = null;

        if (parameters == null)
        {
            parameters = new Dictionary<string, object>();
        }

        using (DbConnection connection = factory.CreateConnection())
        {
            connection.ConnectionString = connectionString;

            using (DbCommand command = factory.CreateCommand())
            {
                command.Connection = connection;
                command.CommandText = sql;

                foreach (KeyValuePair<string, object> p in parameters)
                {
                    var parameter = command.CreateParameter();
                    parameter.ParameterName = p.Key;
                    parameter.Value = p.Value;

                    command.Parameters.Add(parameter);
                }

                connection.Open();
                return command.ExecuteNonQuery();
            }
        }
    }

Once that works, promote parameters to be a parameter. This doesn't affect any existing callers of Update:

    /// <summary>
    /// Executes Update statements in the database.
    /// </summary>
    /// <param name="sql">Sql statement.</param>
    /// <returns>Number of rows affected.</returns>
    public static int Update(string sql)
    {
        return NonQuery(sql, null);
    }

    public static int NonQuery(string sql, Dictionary<string, object> parameters)

At this point, test NonQuery with a parameterized query. Once that works, create an overload of Update that accepts the parameters:

    /// <summary>
    /// Executes Update statements in the database.
    /// </summary>
    /// <param name="sql">Sql statement.</param>
    /// <returns>Number of rows affected.</returns>
    public static int Update(string sql)
    {
        return NonQuery(sql, null);
    }

    /// <summary>
    /// Executes Update statements in the database.
    /// </summary>
    /// <param name="sql">Sql statement.</param>
    /// <param name="parameters">Name/value dictionary of parameters</param>
    /// <returns>Number of rows affected.</returns>
    public static int Update(string sql, Dictionary<string, object> parameters)
    {
        return NonQuery(sql, parameters);
    }

Step 3: Take Stored Procedures Into Account

There's little difference in terms of how stored procedures would be handled. What you've already got is implicitly as follows:

            using (DbCommand command = factory.CreateCommand())
            {
                command.Connection = connection;
                command.CommandText = sql;
                command.CommandType = CommandType.Text;

So take the CommandType.Text and promote it to be a parameter in an overload:

    public static int NonQuery(string sql, Dictionary<string, object> parameters)
    {
        return NonQuery(sql, CommandType.Text, parameters);
    }

    public static int NonQuery(string sql, CommandType commandType, Dictionary<string, object> parameters)

Finally, if you like, update Update:

    /// <summary>
    /// Executes Update statements in the database.
    /// </summary>
    /// <param name="sql">Sql statement.</param>
    /// <param name="parameters">Name/value dictionary of parameters</param>
    /// <returns>Number of rows affected.</returns>
    public static int Update(string sql, Dictionary<string, object> parameters)
    {
        return Update(sql, CommandType.Text, parameters);
    }

    /// <summary>
    /// Executes Update statements in the database.
    /// </summary>
    /// <param name="sql">Sql statement.</param>
    /// <param name="commandType">CommandType.Text or CommandType.StoredProcedure</param>
    /// <param name="parameters">Name/value dictionary of parameters</param>
    /// <returns>Number of rows affected.</returns>
    public static int Update(string sql, CommandType commandType, Dictionary<string, object> parameters)
    {
        return NonQuery(sql, parameters);
    }

Of course, as a final exercise for the reader, you might replace all your Update calls with calls to NonQuery and get rid of Update entirely.


Of course, this simple technique doesn't handle output parameters, or situations where it's necessary to specify the DbType of the parameter. For that, you'd need to accept a ParameterCollection of some kind.

John Saunders
Yeah this seems to make sense. I'm guessing that i'll make use simple objects to go within the dictionary such as string,int,bool,datetime,etc.. and then use reflection to determine the param type upon creation.Does that seem reasonable or do you know of a better method?Thanks
zSysop
Why would you need to use reflection? My code above does not.
John Saunders
Yeah i noticed that it. The method i used to create a parameter in the past was paramater.Add(name,dbtype).value = object.value, but i guess i don't really need to use the dbtype property.Thanks
zSysop
A: 

I do something similar to this using a interface and generics.

Example

    using System.Data.SqlClient;

public interface IParameter<T> where T : IEntity<T>
{
 void Populate(SqlParameterCollection parameters, T entity);
}

A class that implements the interface using System.Data; using System.Data.SqlClient;

public class UserParameter : IParameter<User>
{
 public void Populate(SqlParameterCollection parameters, User entity)
 {
  parameters.Add("@ID", SqlDbType.UniqueIdentifier).Value = entity.Id;
  parameters.Add("@Name", SqlDbType.NVarChar, 255).Value = entity.Name;
  parameters.Add("@EmailAddress", SqlDbType.NVarChar, 255).Value = entity.EmailAddress;
 }
}

Then we have the method that performs the update

         public void Update<T>(string prefix, IParameter<T> parameters, T entity)
  where T : class, IEntity<T>
 {
  using (var connection = this.Connection())
  {
   using (var command = new SqlCommand(string.Format("dbo.{0}_Update", prefix), connection))
   {
    command.CommandType = CommandType.StoredProcedure;

    parameters.Populate(command.Parameters, entity);

    connection.Open();

    command.ExecuteNonQuery();

    connection.Close();
   }
  }
 }

I then call this by doing somethind like

Update("Users", new UserParameter(), value);

I also do the same for populating a entity from the reader values.

Mike Geise
This seems to run counter to the premise of refactoring, which is to make a series of _small_ changes. This here seems like a pretty big change to the code.
John Saunders
and the other answers are not?
Mike Geise
Since you implicitly include my answer in that, I'll consider that a challenge.
John Saunders
A: 

You may have a hard time with this, depending on the environment in which you plan to place this.

If I understand correctly, you'd like to keep the function signature (a single string parameter) but process it differently if the first word in the string is not "UPDATE". For example, this string should execute a stored proc named UpdateTable1 with four arguments:

UpdateTable1 NULL, A5KMI, "New description", #5/10/2009#

or something like that.

The problem is that you need a delimiter between arguments in the rest of the string. For this, you will have to break apart the string into tokens, and this can get really trick.

So the alternative is to prepend the command EXEC in front of the entire string, set the whole thing into the CommandText property, and then call one of the Execute methods.

What I don't like about this is that it really opens you up with SQL injection vulnerability. If it's at all possible, use overloaded methods like the ones others have answered with, and make the caller split the arguments into a collection of some kind.

Alan McBee
I don't see where the OP said anything about "UPDATE".
John Saunders
I know - I'm reading into the design of the original code that it is passed a full SQL UPDATE command normally. Generally speaking, that would mean UPDATE would be the first token in the statement.
Alan McBee
I don't think I'd use the word "generally". It might start with a WITH clause, or who knows what else.
John Saunders
Agreed, but there should still be a limited number of keywords that would have to be checked. I do like your solution better, but like all of the others, it requires all the callers of the UPDATE method to pass in the arguments separately, calling an overload. I wasn't assuming that would be always possible to refactor all the callers to the Update method.
Alan McBee
Not so. All the existing callers presumably work without passing any parameters. It's only new callers that we need be concerned about (assuming the existing callers have passed all their unit tests and have good code coverage). The new callers are those who need parameters or those who need to use stored procedures. And, BTW, the "limited number of keywords" are all the possible keywords in the SQL dialect being used. I don't see that it's practical to restrict them.
John Saunders
I stand corrected; I meant "all NEW callers must pass in additional parameters." And you only need to check against ALL keywords if you have no idea/control of the callers' semantics. That's not always true.I think we're both making a few assumptions. You're assuming there are unit tests and that all new callers have yet to be written. I was assuming refactoring meant "refactoring" and not "designing new interfaces," which the accepted answer turned out to be doing. Clearly, you're a little more experienced here and know which assumptions are safer than others. Point to you, sir.
Alan McBee