Improving the way a C# application communicates with a SQL database (Via SqlConnection)

Question

*Originally posted this on stackoverflow but was told that it would be better suited here.

So I'm looking for a better way to setup how an application talks with a database. I'm sure this question has been asked many times, and I dug through a few of the similar ones but couldn't find one that really flushed it out as much as I'd have liked.

I have an application that uses a database helper class to connect and retrieve records from a database. I was considering rewriting it and wanted to know what the best way to do it would be.

Here is roughly how it's set up now (Note: This is already in place, and there are thousands of lines of this stuff).

DatabaseHelper.CS

 private SqlConnection conn; public DatabaseHelper() { // Create database connection conn = new System.Data.SqlClient.SqlConnection(); SqlConnectionStringBuilder connection = new SqlConnectionStringBuilder(); connection.ConnectTimeout = 150; // Microsft fix for timeout error (known bug) connection.MinPoolSize = 20; // Microsft fix for timeout error (known bug) connection.DataSource = Properties.Settings.Default.DBString; connection.InitialCatalog = Properties.Settings.Default.DBInitCatalog; connection.IntegratedSecurity = true; if (conn.State != ConnectionState.Connecting) { conn.ConnectionString = connection.ConnectionString; } } public bool Open() { if (this.IsOpen()) // IsOpen is just a method that checks connectionstate. { return true; } else { try { conn.Open(); return true; } catch (System.Data.SqlClient.SqlException ex) { // omitted for post } } return false; } public bool Close() { if (!this.IsOpen()) { return true; } try { conn.Close(); return true; } catch (System.Data.SqlClient.SqlException ex) { // omitted for post } return false; } public List<string> GetTeamLeaders(string team) { List<string> leaders = new List<string>(); string query = "Select Leader FROM Teams WHERE Team = @team_vc"; try { using (SqlCommand cmd = new SqlCommand(query, conn)) { cmd.Parameters.Add("@team_vc", SqlDbType.NVarChar).Value = team; using (SqlDataReader sdr = cmd.ExecuteReader()) { int column = sdr.GetOrdinal("Leader"); while (sdr.Read()) { leaders.Add(sdr[column].ToString()); } } } } catch (Exception ex) { // omitted for post } return leaders; } private string GetTeamAbbrev(string team) { string abbrev= ""; string query = "SELECT Abbrev FROM Teams where Team = @team_vc"; using (SqlCommand cmd = new SqlCommand(query, conn)) { cmd.Parameters.Add("@team_vc", SqlDbType.NVarChar).Value = team; try { abbrev= Convert.ToString(cmd.ExecuteScalar()); } catch (Exception ex) { // omitted for post } } return (string.IsNullOrEmpty(location)) ? "None" : abbrev; } 

MainApp.CS

 private DatabaseHelper dbHelper; public MainApp() { InitializeComponent(); dbHelper= new DatabaseHelper(); // Instantiate database controller } private void someButton_Click(object sender, EventArgs e) { List<string> teamLeaders = new List<string>(); if (dbHelper.Open()) { teamLeaders = dbConn.GetTeamLeaders(textboxTeam.Text); dbHelper.Close(); } else { return; } // all the code to use results } private void someOtherButton_Click(object sender, EventArgs e) { List abbreviation = string.Empty; if (dbHelper.Open()) { abbreviation = dbConn.GetTeamLeaders(textboxTeam.Text); dbHelper.Close(); } else { return; } // all the code to use results } 

Now I'm sure there are some very serious issues with how this is setup, but for me my biggest complaints are always having to open and close the connection.

My first move was to just move the open and close inside the DatabaseHelper methods, so each method (i.e. GetTeamLeaders) would call open and close in itself. But the problem was if it did actually fail to open it was really hard to feed it back up to the main program, which would try to run with whatever value the variable contained when it was made. I was thinking I would almost need an "out" bool that would tag along to see if the query completed, and could check make and check that anytime I used I needed to get something from the database, but I'm sure that has issues to.

Another big problem with this approach is anytime I want to make a call from another form, I have to either make another instance of the helper on that form, or pass a reference to the main one. (Currently my way around this is to retrieve all the information I would need beforehand in the MainApp and then just pass that to the new form). I'm not sure if when I rewrite this there's a good static way to set it up so that I can call it from anywhere.

So is there anything here worth keeping or does it all need to be stripped down and built back from scratch?

Answer 1

I'm going to try to attack from a perspective of refactoring. In my mind "re-write" means starting over.

In MainApp.cs you want to say this:

teamLeaders = dbConn.GetTeamLeaders(textboxTeam.Text); 

You're right that having MainApp having to know/control the sql connection is bad design. We're going to get rid of the Open() and Close() calls by being "connection oriented" instead of "sql command oriented". Keep reading for clarification...

Then GetTeamLeaders() should set up the SqlCommand as it does now, but not execute the query. Instead pass that into some other DataBaseHelper method and ...

use "using" to get rid of Open / Close code

protected bool ExecuteQuery(SqlCommand myCommand) { // return bool? void? I dunno. But I don't want MainApp dealing with // what's going on here. I suppose it depends on how you handle your exceptions. using (SqlConnection myconnect = new SqlConnection(myCommand)) { // other setup as needed try{ myconnect.Open(); mySqlCmd.ExecuteNonQuery(); // or whatever }catch (Exception except) { // really? catch specific SqlConnection exceptions to gather better "telemetry" }catch (Exception except) {} // but also have a catch-all. }finally{ myconnect.Close(); // it's automatic, but i do this anyway. } } // using } 

It's my understanding that .NET handles connection pooling automatically and I've never worried about it in my experience.

My spidey sense tells me that with proper refactoring, all your exception handling will be right here.

Follow-on Refactoring - getting business logic out of DataBaseHelper

With the above in place, I can see the query string in GetTeamLeaders, and similar methods you must have in DataBaseHelper.cs pulled out. Now DataBaseHelper.cs is free of all "business context".

If the SqlCommand setup is very customized for each query, then perhaps GetTeamLeaders becomes a whole new class; very possibly derived from an abstract class with standard/common SqlCommand setup. Oh, you can use the same SqlCommand object with different parameter settings; Remember that parameters is a collection on SqlCommand and so you can have have custom methods to set up ParameterCollections and pass that into a SqlCommand object used over and over, living in that abstract class I mentioned.

Answer 2

It seems that you are looking for object-relational mapping (ORM) framework. ORM framework is a recommended way to talk to the database. There are several frameworks out there including most known Entity Framework and NHibernate. It will require a bit of learning after direct SqlConnection/SqlCommand/SqlDataReader, but that would allow you to get rid of manual SQL string manipulations and concentrate on main business logic.

anytime I want to make a call from another form, I have to either make another instance of the helper on that form, or pass a reference to the main one

When using one of the ORMs mentioned above you will have a notion of "session" (NHibernate) or context (Entity Framework). Both terms refer to the same concept of unit-of-work + repository. These objects are very cheap to create, so usually you create one (sometimes more) session/context per form, and don't share them between forms.