3
\$\begingroup\$

I need to dynamically call a stored procedures from my repository, and here is a my implementation:

        public void ExecProcedure(string name, Dictionary<string, string> param = null)
        {
            List<object> sqlParametesList = new List<object>();

            try
            {
                if (param != null)
                {
                    name = NormalizeProcedureName(name, param);
                    foreach (var item in param)
                    {
                        sqlParametesList.Add(new SqlParameter(item.Key, item.Value));
                    }
                }

                context.Database.ExecuteSqlCommand(name, sqlParametesList.ToArray());
            }
            catch (Exception e)
            {                
                throw new Exception(string.Format("Error in ExecProcedure: {0}", e.Message));
            }
        }

There is 3 case of call ExecProcedure():

  1. _repo.ExecProcedure("usp_Test"); // call without parameters
  2. _repo.ExecProcedure("usp_Test2 @id", param); // set parameter name after procedure name and pass value as param
  3. _repo.ExecProcedure("usp_Test2", param); // if passing param then we use method NormalizeProcedureName() to set parameter name after procedure name

    private string NormalizeProcedureName(string name, Dictionary<string, string> param)
    {
        if (!name.Contains("@"))
        {
            StringBuilder sb = new StringBuilder();
            string delimiter = "";
            sb.Append(name);
    
            foreach (var item in param)
            {                    
                sb.Append(delimiter);
                sb.Append(" @");
                sb.Append(item.Key);
                delimiter = ",";
            }
    
            name = sb.ToString();
        }
        return name;
    }
    

What do you think about this implementations?
Any advice will be appreciated. Thanks

\$\endgroup\$
2
\$\begingroup\$

Dictionary<string, string> param: I realize why you named it param and not params, but the real solution was to call it parameters.

Even worse as a name is sqlParametesList: it contains a typo, and the name also contains the type ("List") which is completely superfluous.

But the real problem is that a Dictionary<string, string> is too limited. What if you need to pass an int or a datetime for instance? You even acknowledge this by converting the strings to objects, which isn't a solution at all.


NormalizeProcedureName is a seriously bad idea. It doesn't even do what it says, and what it does can be done far simpler with a string.join() -- not that this would be a good idea either.


I don't get the point of this:

throw new Exception(string.Format("Error in ExecProcedure: {0}", e.Message));

The stack trace should tell you where the exception happened, but you throw it away to create a far simpler error message.


IMO it would be far better to create a custom class to hold the necessary parameter data: the parameter name, its value and its type. You then pass a list of these objects to ExecProcedure, and loop through the list to construct the parameters you need to pass to ExecuteSqlCommand, making sure to also include the correct SqlDbType.

Or perhaps you can simply use SqlParameter instead of a custom class, it depends a bit on how you've abstracted your db logic from your business logic.


EDIT: To provide some guidance, here's an example of how I handled a similar problem.

I had to call a stored proc in an Oracle DB, which required a number of parameters. So I created this class:

public class LogMessageParameters
{
    public DateTime? Date { get; set; }

    public string From { get; set; }

    public long? Code { get; set; }

    public List<OracleParameter> ToOracleParameters()
    {
        return new List<OracleParameter>
        {
            new OracleParameter("DATE", OracleDbType.Date, Date, ParameterDirection.Input),
            new OracleParameter("FROM", OracleDbType.VarChar, From, ParameterDirection.Input),
            new OracleParameter("CODE", OracleDbType.Number, Code, ParameterDirection.Input),
            new OracleParameter("ID", OracleDbType.Number, ParameterDirection.Output)
        };
    }
}

Now all I needed to do was assign the proper values to the properties, and pass this class to the method where I needed to use the parameters.

Is this the ideal way? Not if I had to create a dozen of these classes, no: then I'd made sure to use reflection to figure out the type of a property so I could return the proper OracleDbType, I'd have used an attribute on a property to store the parameter name, etc.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. I fixed naming and NormalizeProcedureName according to comment. Could you please explain me how can I create a custom class to hold the necessary parameter? \$\endgroup\$ – Roman Marusyk Sep 10 '15 at 10:15
  • \$\begingroup\$ This class have to be Generic, right? And then add all of instance to collections and pass in my method? \$\endgroup\$ – Roman Marusyk Sep 10 '15 at 10:43
  • \$\begingroup\$ @MegaTron It depends on how your program is structured, etc. I wouldn't be surprised if there are already solutions out there, though I haven't found them when I did a quick search. Experiment, refactor, etc. Perhaps you should have a class that contains all the necessary params for one particular StoredProc, and a method on that class (e.g. ToSqlParameters) where you return an appropriate array. \$\endgroup\$ – BCdotWEB Sep 10 '15 at 11:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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