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?
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;
}
C#.NET ProgrammingMicrosoft SQL Server
Last Comment
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.
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
THIS SOLUTION ONLY AVAILABLE TO MEMBERS.
View this solution by signing up for a free trial.
Members can start a 7-Day free trial and enjoy unlimited access to the platform.
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.
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.
e.g.
public static DataTable SQLQuery(string stProcedureName, string[] sqlParams)
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.
ASKER
thank you
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.