Can you please look over my code and tell me if I can improve it.

Hello experts. My goal is to create one Sub that does Select from SQL Database and returns a set of data. So below is the sub I created that accepts a stored procedure name as its first param, and an array of parameters as its second parameter.

I need criticism - Please tell me if I am doing something that might later bite me the youknowhat. And is returning a DataTable OK? Is there a better way to do this?


//[S]QLQuery
    /*  This function will query the SQL Engine for a specific stored procedure and paramters. Returns a recordset
     *  @params             String                  Required        Stored Procedure name
     *  @params             object[] Array          Optional        List of variables to be used in the Stored Prcedure
     *  @Returns            DataTable dt    If Success      Returns a recordset object with the selected rows
     *                                              If failure      Calls Error Sub
     */
    public static DataTable SQLQuery(string stProcedureName, object[] sqlParams)
    {
        SqlDataReader reader = null;
        SqlConnection con = new SqlConnection(ConnString);
        SqlCommand cmd = new SqlCommand(stProcedureName, con);

        if (sqlParams != null)
        {
            foreach (string[] param in sqlParams)
            {
                // param[0].ToString() is the procedure varible name
                // param[1].ToString() is the incoming value
                cmd.Parameters.Add(new SqlParameter(param[0].ToString(), param[1].ToString()));
            }
        }

        con.Open();

        try
        {
            reader = cmd.ExecuteReader(CommandBehavior.CloseConnection);
        }
        catch(Exception err)
        {
            Utilities.Error(err);
        }
        DataTable dt = new DataTable();
        dt.Load(reader);
        return dt;
    }

Open in new window

LVL 1
vituxaAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

Surendra NathTechnology LeadCommented:
Few points

1) Not every time a stored procedure will return some data.. a stored procedure might just update something and return nothing back... In that case the above sub will not hold good

2) Microsoft security specifically object this type of programming as these are more prone to the security issues.

3) trying to use the using clause instead of manually opening the connection and closing the connection, as these are more prone to issue in case of errors.
vituxaAuthor Commented:
Is DataTable as a return type efficient enough? Is there a better way?

Is it possible to ask you to maybe provide a sample code of how you would do write this sort of procedure?

I realize that some procedures don't return anything - this sub is created to only call procedures that do return data.

Thank you.
Carl TawnSystems and Integration DeveloperCommented:
If you want to provide flexibility then there isn't really anything you can return other than a DataTable. I've written my own library that has a generic method that can execute a procedure and load the results into a custom object type defined by the caller - but that is a bit more involved.

One possible improvement might be to code to interfaces, which I do myself, which allows you to support something other than SQL Server if you needed to without recoding.

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
kaufmed   ( ⚆ _ ⚆ )Patches? We Ain't Got No Patches! We Don't Need No Patches! I Don't Have to Push You No Stinkin' Patches!Commented:
If you're going to call ToString on your query values anyway, why not use a string array rather than an object array?

e.g.

public static DataTable SQLQuery(string stProcedureName, string[] sqlParams)

Open in new window


I'm not terribly comfortable with your approach because it seems like you are exporting database-related metadata (e.g. sproc and parameter names) outside of the layer that deals with the database. This will introduce a dependency on these external layers. If this code is executed from only within your data layer, then my point is irrelevant.
vituxaAuthor Commented:
thank you
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
C#

From novice to tech pro — start learning today.