4
\$\begingroup\$
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?

\$\endgroup\$
  • \$\begingroup\$ have a look at dapper.net, it would reduce much code(creating SQL parameter, mapping/creating DAO object) \$\endgroup\$ – Arjun Vachhani Nov 28 '15 at 14:28
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the review. Regarding connection class you spoke, is it really needed in this case ? I have only one class in the whole application which needs connection string and I take it from configuration file. So if I have to change later database etc, I just need to change it in configuration file. Also, you talked about global variable, it's not global variable, it's a class member and initiated in the constructor of class. \$\endgroup\$ – Rajesh Naik Nov 29 '15 at 10:13
  • \$\begingroup\$ The approach I've suggest you is more of flexibility. Currently your approach will work, however, later if you want to add more class then it will be difficult with current approach. The class member used globally between the methods creates tight coupling, IMO we should avoid it. \$\endgroup\$ – SiD Nov 30 '15 at 5:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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