Handling multiple queries in C#

In my project I am using 10-15 SQL queries, for that I have a common class for database connectivity and using that I have executed the queries by passing the connection, command and dataReader.

With some reference in SO, I learned that my code is not properly structured. I would like to learn and write high performance code.

DBConnectivity.cs:

class DBConnectivity
    {
        public SqlConnection connection = null;
        public SqlCommand command = null;
        public SqlDataReader dataReader = null;
        public string connectionString = null;
        public List masterTableList;
        public DBConnectivity()
        {
            connectionString = ConfigurationManager.ConnectionStrings["Master"].ConnectionString;
            connection = new SqlConnection(connectionString.ToString());

            //-----Master table results 
            connection.Open();
            string masterSelectQuery = "SELECT * FROM MASTER_TABLE";
            command = new SqlCommand(masterSelectQuery, connection);
            dataReader = command.ExecuteReader();
            masterTableList = new List();

            while (dataReader.Read())
            {
                MasterTableAttributes masterTableAttribute = new MasterTableAttributes()
                {
                    fileId = Convert.ToInt32(dataReader["Id"]),
                    fileName = Convert.ToString(dataReader["FileName"]),
                    frequency = Convert.ToString(dataReader["Frequency"]),
                    scheduledTime = Convert.ToString(dataReader["Scheduled_Time"])
                };
                masterTableList.Add(masterTableAttribute);
            }
            dataReader.Close();
            connection.Close();
        }
    }

Program.cs:

```
class Program
{
static void Main(string[] args)
{
DBConnectivity dbConnectivity = new DBConnectivity();
Transaction txnObj = new Transaction(dbCon

The reader/command/connection fields of DBConnectivity are not really being used to represent state of DBConnectivity. This is obvious because of usage like:

dbConnectivity.command = new SqlCommand(salesQuery, dbConnectivity.connection);
dbConnectivity.dataReader = dbConnectivity.command.ExecuteReader();
while (dbConnectivity.dataReader.Read()) {...}

frankly, they should just be local variables:

var command = new SqlCommand(salesQuery, connection);
var dataReader = dbConnectivity.command.ExecuteReader();
while (dataReader.Read()) {...}

Additionally, in all uses, you are not correctly disposing your resources. The IDisposable contract is important: if something is IDisposable, then it is your job to make sure it gets disposed when done. So with the above example:

using(var command = new SqlCommand(salesQuery, connection))
using(var dataReader = dbConnectivity.command.ExecuteReader())
{
    while (dataReader.Read()) {...}
}

This using makes a huge difference, and ensures you don't drop connections etc on the floor when exceptions happen.

Personally, I would not advocate doing this type of work in a constructor - that is not intuitive. A static method that returns a populated instance, sure - or an instance method that populates an existing instance. But a constructor? not so clear.

I also suspect that you could benefit from the use of tooling to avoid boiler-plate code that is error-prone and not important to your actual work. For example, tools like dapper allow much simpler object population. For example:

public class DBConnectivity
{
    private readonly string connectionString;
    public DBConnectivity(string connectionString = null)
    {
        if(string.IsNullOrEmpty(connectionString))
        {
            connectionString = ConfigurationManager.ConnectionStrings["Master"].ConnectionString;
        }
        this.connectionString = connectionString;
    }
    public SqlConnection OpenConnection() {
        var conn = new SqlConnection(connectionString);
        conn.Open();
        return conn;
    }
    public List GetMasterTableList()
    {
        using(var connection = OpenConnection())
        {
            const string sql = "SELECT Id as [FileId], FileName, Frequency, Scheduled_Time as [ScheduledTime] FROM MASTER_TABLE";
            return connection.Query(sql).ToList();
        }
    }
}

Here the only useful state is the connection string, with a method that creates new connections. Note how this is used with using to create and dispose the connection, with "dapper" then taking the load for handling the data.

If you really wanted to pass this around, you can:

public List SomeMethodOnSales(DBConnectivity dbConnectivity)
    {            
        using(var conn = dbConnectivity.OpenConnection())
        {
            return conn.Query(someQuery).ToList();
        }
    }

From your code, I'm guessing you have:

class MasterTableAttributes
{
    public int fileId;
    public string fileName;
    public string frequency;
    public string scheduledTime;
}

this would preferably be:

class MasterTableAttributes
{
    public int FileId {get;set;}
    public string FileName {get;set;}
    public string Frequency {get;set;} // maybe an enum?
    public string ScheduledTime {get;set;} // datetime? timespan?
}

StackExchange Code Review Q#26149, answer score: 5