While the idea is a decent one, the approach has many holes. Let's tart with the DatabaseConnection class. .NET already has an abstract DbConnection class which SqlConnection inherits from and both (obviously) implement the IDbConnection interface. This implementation seems to want to create and manage SqlConnection objects, as well as maintain its state, AND do some error reporting via the UI (MessageBox call). This is itself a violation of the Single Responsibility principle. It does three different things.
So let's move those three things apart and see what we get. First, let's get a thing that creates database connections:
static class DatabaseFactory
{
public static IDbConnection CreateDbConnection(string server, string db, string user, string pass)
{
DbConnectionStringBuilder builder = new SqlConnectionStringBuilder
{
DataSource = server,
InitialCatalog = db,
UserID = user,
Password = pass
};
return new SqlConnection(builder.ConnectionString);
}
public static IDbCommand CreateCommand(string storedProcName, params DbParameter[] parameters)
{
var storedProcedure = new SqlCommand(storedProcName)
{
CommandType = CommandType.StoredProcedure
};
storedProcedure.Parameters.AddRange(parameters);
return storedProcedure;
}
}
Note that I used the SqlConnectionStringBuilder class to put together the connection string rather than string concatenation. Eliminates any potential typos and it sanitizes/escapes any strings properly. By the by, I didn't do this here, but parameter names such as pass should really be spelled out to password and the like as (in this case), the shortened version is another word altogether that can be confusing.
Next, something is needed to manage connection state. Let's put that together:
class DatabaseConnectionManager : IDatabaseConnectionManager
{
private readonly IDbConnection _Connection;
public DatabaseConnectionManager(IDbConnection connection)
{
this._Connection = connection;
}
public IDbConnection Connection
{
get
{
return this._Connection;
}
}
public void Open()
{
if ((this._Connection != null) && (this._Connection.State == ConnectionState.Closed))
{
this._Connection.Open();
}
}
public void Close()
{
if ((this._Connection != null) && (this._Connection.State == ConnectionState.Open))
{
this._Connection.Close();
}
}
}
This thing is really kinda boring, to be honest. It does things that SqlConnection already does, but that's not the point. You can change the constructor to pass in loggers or what-not to add on to what you need out of the class. But as-is, it does what we want. Three things to note here, unlike the original, this one does not open the connection in the constructor. Reason being, as a best practice, constructors should not do something that may cause an exceptional condition to occur and opening a database connection is certainly an opportunity for that. Second, the try..catch is gone. There's not much the class can do with it -- the original threw up a MessageBox which is a mixing of concerns, but we'll leave someone higher up the call chain to catch it and do something with it (unless you need logging as above). The last thing is that the class implements a new interface, called IDatabaseConnectionManager. Here it is in all its glory:
interface IDatabaseConnectionManager
{
void Open();
IDbConnection Connection { get; }
void Close();
}
Kinda cut and dry, but it will allow us to develop to interfaces.
So that brings us to the DataViewer class. It's another blending of UI and data, which feels weird. Let's keep it data-only for a moment. Let's say it's a class designed to execute a single stored procedure and return some data. I've left out some details, but you can fill those in! :)
class DataViewer : IDataViewer
{
private IDatabaseConnectionManager _ConnectionManager;
public DataViewer(IDatabaseConnectionManager connectionManager)
{
this._ConnectionManager = connectionManager;
}
public DataSet GetDataInStoredProc(IDbCommand storedProcedure)
{
storedProcedure.Connection = this._ConnectionManager.Connection;
this._ConnectionManager.Open();
try
{
// Logic here to get and return data somehow.
}
finally
{
this._ConnectionManager.Close();
}
}
}
Another interface for our use:
interface IDataViewer
{
DataSet GetDataInStoredProc(IDbCommand storedProcedure);
}
Okay. So let's put it all together somewhere (semi pseudo-code)...
using (var connection = DatabaseFactory.CreateDbConnection("localhost", "products", "test", "p455w0rd"))
{
var manager = new DatabaseConnectionManager(connection);
var viewer = new DataViewer(manager);
using (var procedure = DatabaseFactory.CreateCommand("GetProducts"))
{
var products = viewer.GetDataInStoredProc(procedure);
myDataGrid.DataSource = products;
}
}
This is all only semi-usable code. You'll want to fill in the blanks appropriately. There's a few new abstractions and layers to keep pieces separated so that they may be replaced with other versions or mock versions for unit testing.
