SQL server database access helper class

Question

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Configuration;
using System.Data.SqlClient;
using System.Data;

namespace ExamManagement.DLL
{
    public class SQLDataAccessHelper
    {
        string connectionString=string.Empty;

        public SQLDataAccessHelper()
        {
            try
            {
                connectionString = ConfigurationManager.ConnectionStrings["DefaultConnectionString"].ConnectionString;

            }
            catch (Exception)
            {
                //todo error handling  mechanism
                throw;
            }
        }



        public void ExecuteNonQuery(string commandText,CommandType commandType, params SqlParameter[] commandParameters)
        {
            using(var connection= new SqlConnection(connectionString))
            using (var command = new SqlCommand(commandText, connection))
            {
                command.CommandType = commandType;
                command.Parameters.AddRange(commandParameters);
                connection.Open();
                command.ExecuteNonQuery();
            }
        }

        public DataSet ExecuteQuery(string commandText,CommandType commandType,params SqlParameter[] parameters)
        {
            using(var connection=new SqlConnection(connectionString))
            using(var command= new SqlCommand(commandText,connection))
            {
                DataSet ds=new DataSet();
                command.CommandType=commandType;
                command.Parameters.AddRange(parameters);
                SqlDataAdapter da = new SqlDataAdapter(command);
                da.Fill(ds);
                connection.Close();
                return ds;
            }
        }
    }
}

This is helper class I have written to retrieve data and execute CRUD operation on sql server database either using queries or stored procedures.

Currently it is used from DAL classes as follows :

1. I create instance of it
2. I call method "ExecuteQuery" and it returns DataSet

Details as follows:

namespace ExamManagement.DLL.Implementations
{
    public class StudentDAO : IStudentDAO
    {
        SQLDataAccessHelper sqlHelper;
        public StudentDAO()
        {
            sqlHelper = new SQLDataAccessHelper();
        }

        public List<Entities.Student> AllStudents()
        {
            List<Entities.Student> studentsList = new List<Entities.Student>();

            SqlParameter[] parameterList = { new SqlParameter("@Id",DBNull.Value),
                                               new SqlParameter("@name",DBNull.Value),
                                               new SqlParameter("@address",DBNull.Value)};

            var table = sqlHelper.ExecuteQuery("GetStudents", System.Data.CommandType.StoredProcedure,parameterList).Tables[0];
            studentsList = table.AsEnumerable().Select(row => new Entities.Student { Id = row.Field<int>("Id"), Name = row.Field<string>("Name"), Address = row.Field<string>("Address") }).ToList();
            return studentsList;
        }

I have following questions with regards to it

1. Helper class should be static class or the methods in it should be static methods?

2. Connection string is not supplied by class user, instead its is auto calculated from Web.config using ConfigurationManager. Is this approach ok?

3. I'm creating connection in "Using" statement, so can I ignore calling connection.Close() or its required even then ?

4. How can I improve it.

5. Possible to write unit tests for this class?

Answer 1

Everything looks good over here, however, I would like to mention some points which cover some of your questions.

I can see your methods are dependent on the global variable connectionString.

• Global variables creates confusion when code grows.
• It harder to maintain.
• The code which uses global variables gets tightly coupled.

I suggest you to create a class to handle all database connections. This will make your code more flexible:

• It allows to use same connection in other classes.

• It allows to use different database connections.

  public class ConnectionManager
  {
      public static SqlConnection GetSqlConnection()
      {
          string connectionString = ConfigurationManager.ConnectionStrings["DefaultConnectionString"].ConnectionString;
          var connection = new SqlConnection(connectionString);
          connection.Open();
          return connection;
      }
  
      public static OleDbConnection GetOleDbConnection()
      {
          string connectionString = ConfigurationManager.ConnectionStrings["OleDbConnectionString"].ConnectionString;
          var connection = new OleDbConnection(connectionString);
          connection.Open();
          return connection;
      }
  }
  

• Above method returns open connection so you don't have to open it.

• It force to implement using, try-catch-finally or IDisposable to make sure you always close connection which is a good thing.
• No need to have constructor for each class which use database.

---

• It is not a good practise to ignore return value. Your method ExecuteNonQuery has return type void which ignores the return value of command.ExecuteNonQuery();
• You should use {} to make code more readable.

Before:

public void ExecuteNonQuery(string commandText, CommandType commandType, params SqlParameter[] commandParameters)
{
    using (var connection = new SqlConnection(connectionString))
    using (var command = new SqlCommand(commandText, connection))
    {
        command.CommandType = commandType;
        command.Parameters.AddRange(commandParameters);
        connection.Open();
        command.ExecuteNonQuery();
    }
}

After:

public int ExecuteNonQuery(string commandText, CommandType commandType, params SqlParameter[] commandParameters)
{
    int affectedRows = 0;
    using (var connection = ConnectionManager.GetSqlConnection())
    {
        using (var command = new SqlCommand(commandText, connection))
        {
            command.CommandType = commandType;
            command.Parameters.AddRange(commandParameters);
            affectedRows = command.ExecuteNonQuery();
        }
    }
    return affectedRows;
}

Hope this helps.