Link to home
Start Free TrialLog in
Avatar of vituxa
vituxa

asked on

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

Avatar of Surendra Nath
Surendra Nath
Flag of India image

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.
Avatar of vituxa
vituxa

ASKER

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.
ASKER CERTIFIED SOLUTION
Avatar of Carl Tawn
Carl Tawn
Flag of United Kingdom of Great Britain and Northern Ireland image

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
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.
Avatar of vituxa

ASKER

thank you