Proper way to execute parameterized commands within a method

Question

This method allows for the execution of parameterized commands within a DAL class, and I tried to make it as reusable as possible. It works well but it feels a bit verbose to me. Is there a way to simplify this or is it good as is?

/// <summary>
/// Retrieves records from the specified data source.
/// </summary>
/// <param name="query">The command to be executed.</param>
/// <param name="dataSrc">The data source to use for the command.</param>
/// <param name="parameters">Optional parameters for a parameterized command.</param>
/// <returns>A System.Data.DataTable containing the returned records.</returns>
public static DataTable GetRecords(string query, DelConnection dataSrc, params SqlParameter[] parameters)
{
    if (query == "") throw new ArgumentException("No command provided.", "query");
    string conString = Dal.GetConnectionString(dataSrc);

    using (SqlConnection con = new SqlConnection(conString))
    using (SqlCommand cmd = new SqlCommand(query, con))
    {
        // Add parameters to command
        foreach (SqlParameter p in parameters)
        {
            cmd.Parameters.Add(p);
        }

        // Fill data table
        DataTable data = new DataTable();
        using (SqlDataAdapter adap = new SqlDataAdapter(cmd))
        {
            adap.Fill(data);
        }
        return data;
    }
}   //// End GetRecords()

Why are you returning null when the result is empty? It just makes the code that works with the result more complicated (it always has to check for null), more error prone (what if your forget to check for null?) and doesn't actually give you anything.
What is a DelConnection? I can't find it on Google. Is it an enum like Development, Test, Production etc?
@abuzittingillifirca: Yes, it's a custom enum that identifies the data source. The DAL is currently translating it into the underlying connection string.

tia · Accepted Answer · 2013-04-26 05:51:30Z

I would suggest few improvements:

Validate all parameters as early as possible. In your method, the validation of parameters collection is not early enough. This is not really about performance because 99.9% of the time there should be no exception thrown, but it is about grouping the validation logic together
Make it an extension method. You can easily make this an extension method of DelConnection and then simplify your code at call site like dataSrc.GetRecords("some query here")
Consider return empty result instead of null value. Especially when you meant to return empty result. My rule of thumb is that every method that returns some kind of collection should not return null at all.
Using params instead of concrete-type collection. Instead of using SqlParameterCollection, you can use params SqlParameter[] parameters instead. This will make it more convenient to call this method.

For the verbosity, your code is not verbose at all in my opinion. It is as short as it should be.

Thanks to everyone for their input. I accepted tia's answer but really used bits from everyone's response. I'll update the question to show the changes I made.

Jeff Vanzella · Answer 2 · 2013-04-25 23:19:49Z

Why is it static?

First off, I would remove the check for no parameters. You've already checked that it is not null, if somebody passes in an empty list, the foreach for parameters will just be ignored.

As svick said, I would not return a null value if no results are found. This complicates code on the calling side because you have to check for null before you work with it. Same principle as the parameters. If I have a foreach on an empty list, my code within the foreach would not be called.

I would change conString == "" to string.IsNullOrEmpty(constring) I would do the same thing for query, because most people will pass an empty string before they pass a space string. If you think space will be a problem, do both.

I would change this DataTable data = new DataTable(); to use var var data = new DataTable(); It makes the code easier to scan quickly.

If you insist on returning null for empty sets, change (data.Rows.Count > 0) to (data.Rows.Any()). I think it makes the code much easier to read because it says exactly what you are looking for.

RobH · Answer 3 · 2013-04-26 09:28:59Z

1: This method is public - are you expecting to use it outside of your Data Access Layer? The method signature includes a SqlParameterCollection... Now all of your calling code has to know that you're using SQL underneath.

2: This code smells:

string conString = Dal.GetConnectionString(dataSrc);
if (conString == "") throw new ArgumentException("Invalid connection source provided.", "dataSrc");

You have a class called Dal responsible for getting connection strings. This seems like more of a configuration thing - I think the class should be renamed to something more descriptive.

I also think that the GetConnectionString method should throw an exception if there isn't a connection string for the data source - it shouldn't be the responsibility of this method.

3: As others have said, don't return null - return an empty collection for no results.

4: Check for empty strings consistently. I.e. don't mix == "" and string.IsNullOrWhiteSpace - always use one of the methods on string.

user1614543 · Answer 4 · 2013-04-26 12:41:44Z

If you say that this method should be as reusable as possible then I assume that it will be used throughout the program. Then why it takes SqlParameterCollection parameter? All non DAL classes should not know such details. List(Of string, object) is enough for parameters.
DelConnection parameter. Your DAL implementation should encapsulate all logic related to DB connections. So if you have one connection string than remove this parameter and acquire connection in this method directly. If you have multiple connection strings than your business layer classes should know only connection string names and pass it to your method.
You use provider specific classes for connection and command. Why not use base classes (or interfaces) as DbConnection, DbCommand and specify DB provider in your connection string?

asked	19 days ago
viewed	108 times
active	18 days ago

Proper way to execute parameterized commands within a method

4 Answers

Your Answer

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

Community Bulletin

Proper way to execute parameterized commands within a method

4 Answers

Your Answer

Sign up or log in

Post as a guest

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

Community Bulletin

Related