We help IT Professionals succeed at work.

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

294 Views
Last Modified: 2014-01-06
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

Comment
Watch Question

Surendra NathTechnology Lead
CERTIFIED EXPERT

Commented:
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.

Author

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.
Senior Systems and Integration Developer
CERTIFIED EXPERT
Commented:
This one is on us!
(Get your first solution completely free - no credit card required)
UNLOCK SOLUTION
CERTIFIED EXPERT
Most Valuable Expert 2011
Top Expert 2015

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.

Author

Commented:
thank you

Gain unlimited access to on-demand training courses with an Experts Exchange subscription.

Get Access
Why Experts Exchange?

Experts Exchange always has the answer, or at the least points me in the correct direction! It is like having another employee that is extremely experienced.

Jim Murphy
Programmer at Smart IT Solutions

When asked, what has been your best career decision?

Deciding to stick with EE.

Mohamed Asif
Technical Department Head

Being involved with EE helped me to grow personally and professionally.

Carl Webster
CTP, Sr Infrastructure Consultant
Empower Your Career
Did You Know?

We've partnered with two important charities to provide clean water and computer science education to those who need it most. READ MORE

Ask ANY Question

Connect with Certified Experts to gain insight and support on specific technology challenges including:

  • Troubleshooting
  • Research
  • Professional Opinions
Unlock the solution to this question.
Join our community and discover your potential

Experts Exchange is the only place where you can interact directly with leading experts in the technology field. Become a member today and access the collective knowledge of thousands of technology experts.

*This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.

OR

Please enter a first name

Please enter a last name

8+ characters (letters, numbers, and a symbol)

By clicking, you agree to the Terms of Use and Privacy Policy.