Data Access Layer Code

Question

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.

Answer 1

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.

Answer 2

I would highly recommend using an ORM instead of writing your own DAL, and you'll be a much more productive programmer overall. NHibernate and Entity Framework are both good choices, or even Linq2Sql if it's a small project that's not performance critical. That way you can spend your time solving the problems that matter, not writing boilerplate code for "plumbing".

It also opens the door to testability, which isn't possible with the code you've written; using an ORM you can substitute in-memory data structures or mocks in your unit tests to simulate the behavior of the database to the rest of your program. That's not really possible when you're using the SqlClient namespace directly and relying on stored procedures in your database.

Answer 3

You could move your try / catch / return logic to a private method that takes an SqlCommand as a parameter.

Also, since you are only using your finally block to close the connection, you might consider a using block instead:

private bool runSpCommand (SqlCommand command)
{
  int rows = 0;

  using (SqlConnection connection = new SqlConnection(connectionString)) 
  {
    connection.Open();
    command.Connection = connection;
    rows = command.ExecuteNonQuery();
    // connection closed on following line, even if there's an exception
  }

  return (rows != 0);

}

I've omitted exception handling from this example, but it should be pretty clear.

Also, in your calling code, you can do this:

// params                
command.Parameters.Add("@name", SqlDbType.VarChar).Value = name;
command.Parameters.Add("@subject", SqlDbType.VarChar).Value = subject;
command.Parameters.Add("@message", SqlDbType.VarChar).Value = message;         

Answer 4

I would turn it into a generic method with this signature:

public bool ExecuteSp(string spName, List<MyParameter> parameters) {

   // ...

   SqlCommand command = new SqlCommand(spName, connection);

   // ...

   foreach(var p in parameters) {
      SqlParameter sqlParameter = new SqlParameter(p.Name, p.Type);
      sqlParameter.Value = p.Value;
      command.Parameters.Add(p);          
   }
   // ...
}

Using an ORM is much better of course.

Answer 5

        SqlConnection connection = new SqlConnection(cnnString);

EDIT You can do this because the connections are pooled. As a result you won't open any more connections then necessary. Stylistically, I prefer passing around Connection objects, but that's your call.

        catch
        {
            return (rows != 0);
        }

This is a very evil three lines of code. Firstly, you catch every sort of exception which is going to hide bugs. You'll never know if a particular exceptions happened. Secondly, you carry one pretty much ignoring the fact that the exception happened. This is very very bad. DO NOT DO THIS!

            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();

I think what you really want is a simpler interface for this piece of the code. Something like:

SQLProcedureCall insertFeedback = new SQLProcedureCall("[dbo].[spInsertFeedback]", connection);
insertFeedback.set("name", name);
insertFeedback.set("subject", subject);
insertFeedback.set("message", message);
return insertFeedback.Execute();

You should be able to easily define a class which implements that. That the duplication in your is effectively eliminated.

Answer 6

The solution to replacing this code.... is to stop writing this code.

These types of DALs are great targets for the rise of the Micro ORM. Including Dapper, Massive, Simple.Data, and PetaPoco to name a few.

Personally I'm a fan of PetaPoco the most. It's available through Nuget. You have choices of Core or Full where Full includes T4 templates that allow you to generate models from your database (not for me but some people like this stuff) and Core (which is a dependency of the full package) facilitates all of your querying.