Handling multiple queries in C#

Question

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<MasterTableAttributes> 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<MasterTableAttributes>();
 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(dbConnectivity); //Query1
 Sales saleObj = new Sales (dbConnectivity);//Query2
 }
 }

Transaction.cs:

class Transaction
 { 
 public Transaction(DBConnectivity dbConnectivity)
 { 
 dbConnectivity.connection.Open();
 string txnQuery = "SELECT ...";
 dbConnectivity.command = new SqlCommand(txnQuery, dbConnectivity.connection);
 dbConnectivity.dataReader = dbConnectivity.command.ExecuteReader();
 while (dbConnectivity.dataReader.Read())
 {
 ...
 }
 dbConnectivity.dataReder.close();
 dbConnectivity.connection.Close();
 }
 }

Sales.cs:

class Sales
 { 
 public Sales(DBConnectivity dbConnectivity)
 { 
 dbConnectivity.connection.Open();
 string salesQuery = "SELECT ...";
 dbConnectivity.command = new SqlCommand(salesQuery, dbConnectivity.connection);
 dbConnectivity.dataReader = dbConnectivity.command.ExecuteReader();
 while (dbConnectivity.dataReader.Read())
 {
 ...
 }
 dbConnectivity.dataReder.close();
 dbConnectivity.connection.Close();
 }
 }

Answer 1

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<MasterTableAttributes> GetMasterTableList()
 {
 using(var connection = OpenConnection())
 {
 const string sql = "SELECT Id as [FileId], FileName, Frequency, Scheduled_Time as [ScheduledTime] FROM MASTER_TABLE";
 return connection.Query<MasterTableAttributes>(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<SomethingSalesEsque> SomeMethodOnSales(DBConnectivity dbConnectivity)
 { 
 using(var conn = dbConnectivity.OpenConnection())
 {
 return conn.Query<SomethingSalesEsque>(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?
}

• Praveen: I tried using Dapper.net tool from NuGet package, while debugging I was redirected to a new tab "No Source Available". Is there anything I am missing and will this cause any problem further?
• Marc Gravell: @user1671639 presumably there was an exception: if so - what was the exception message? it is probably trying hard to tell you what the problem is. You can see the exception when debugging in the IDE, or by using catch.
• Praveen: there is no exception. But when I tried to debug it took to a new tab saying "No Source Available" where the stacktrace message of SqlMapper.cs took place.
• Marc Gravell: @user1671639 for it to do that, either there is an exception, or you are hitting "step into". If the latter: well, don't "step into" if there is nothing to step into. If the former: try just hitting the "play" button (F5) to see what it says next. Can you clarify what you mean by "the stacktrace message" ?
• Praveen: Yeah this happens only when I hit "stepInto". Sorry that is not stacktrace it is "Call stack location". Would be helpful If there are tutorials in dapper.net.