Data Access Layer Code

Wondering if I could do this a little bit better. Currently, my DAL has a blank constructor that sets the connection string from the web.config.

private string cnnString;

public DAL()
{
    cnnString = ConfigurationManager.ConnectionStrings["ApplicationServices"].ConnectionString;
}

Then each method in here is mapped directly to a stored procedure, and they ALL look something like this:

public bool spInsertFeedback(string name, string subject, string message)
{
    int rows = 0;

    SqlConnection connection = new SqlConnection(cnnString);
    try
    {
        connection.Open();
        SqlCommand command = new SqlCommand("[dbo].[spInsertFeedback]", connection);
        command.CommandType = CommandType.StoredProcedure;

        // params
        SqlParameter messageName = new SqlParameter("@name", SqlDbType.VarChar);
        messageName.Value = name;
        SqlParameter messageSubject = new SqlParameter("@subject", SqlDbType.VarChar);
        messageSubject.Value = subject;
        SqlParameter messageText = new SqlParameter("@message", SqlDbType.VarChar);
        messageText.Value = message;

        // add params
        command.Parameters.Add(messageName);
        command.Parameters.Add(messageSubject);
        command.Parameters.Add(messageText);

        rows = command.ExecuteNonQuery();
    }
    catch
    {
        return (rows != 0);
    }
    finally
    {
        connection.Close();
    }

    return (rows != 0);
}

Obviously some return a DataSet or a list of an object or something, where as this one just returns whether or not any rows were affected.

However, each method does things this way, and I just feel like I have a lot of redundant code, and I'd like to simplify it. From the other classes, I'm calling the DAL like this:

DAL dal = new DAL();
bool success = dal.spInsertFeedback(name, subject, message);
return Json(success);

Thanks guys.

I would not recommend putting your dal code in a try-catch block. At least you should be logging or doing something about it, currently you're just returning false (or no value). If an exception occur, let it happen, or put a try-catch on the function that calls it. Remember that exceptions vary (did the db server stop? was the sql malformed? these all are different cases.)

Using using is a better practice for all IDisposable objects, thus embrace it. If you run Visual Studio's code analysis tools (Ultimate Edition only I guess) it would also tell you to do so.

My version of your code is similar to Jeff's (which I wanted to vote up but didn't have enough rep yet) with an exception, SqlCommand is also disposable and can/should be used with using block.

public bool spInsertFeedback(string name, string subject, string message)
 {
     int rows = 0;
     using (var connection = new SqlConnection(cnnString))
     {
         connection.Open();
         using (var command = new SqlCommand("[dbo].[spInsertFeedback]", connection))
         {
             command.CommandType = CommandType.StoredProcedure;
             // params
             SqlParameter messageName = new SqlParameter("@name", SqlDbType.VarChar);
             messageName.Value = name;
             SqlParameter messageSubject = new SqlParameter("@subject", SqlDbType.VarChar);
             messageSubject.Value = subject;
             SqlParameter messageText = new SqlParameter("@message", SqlDbType.VarChar);
             messageText.Value = message;

             // add params
             command.Parameters.Add(messageName);
             command.Parameters.Add(messageSubject);
             command.Parameters.Add(messageText);
             rows = command.ExecuteNonQuery();
         }
     }
     return (rows != 0);
 }

Also I would suggest you to use connectionString instead of cnnString, which is not a proper naming.

StackExchange Code Review Q#2590, answer score: 4