reusable data access class in C#

Question

Below is my attempt at the beginnings of a reusable ado.net data access class. Please let me know of any improvements that I could make.

Thanks!

public class DataAccess : IDisposable 
{
 public DbProviderFactory Factory { get; set; }
 public string Connection { get; set; }
 public string ProviderName { get; set; }
 public DataAccess()
 {
 Get_ConnectionString_From_AppSettings();
 Get_ProviderName_From_AppSettings();
 }
 private void Get_ConnectionString_From_AppSettings()
 {
 this.Connection = ConfigurationManager.AppSettings.Get("Database");
 }
 private void Get_ProviderName_From_AppSettings()
 {
 this.ProviderName = ConfigurationManager.AppSettings.Get("DBProvider");
 }
 public DataTable Connection_Return_DataTable( string sql)
 {
 DbConnection connection = null;
 try
 {
 DbProviderFactory factory = DbProviderFactories.GetFactory(this.ProviderName);
 this.Factory = factory;
 connection = Factory.CreateConnection();
 connection.ConnectionString = Connection;
 DataTable dt = FillDataTable(connection, sql);
 return dt;
 }
 catch (DataException ex)
 {
 Debug.WriteLine(ex.ToString());
 throw;
 }
 finally
 {
 connection.Close();
 connection.Dispose();
 } 
 }
 private DataTable FillDataTable(DbConnection connection, string sql)
 {
 DbCommand command = connection.CreateCommand();
 command.CommandText = sql;
 command.Connection = connection;
 DbDataAdapter adapter = this.Factory.CreateDataAdapter();
 adapter.SelectCommand = command;
 DataTable table = new DataTable();
 adapter.Fill(table);
 return table;
 }
 public void Dispose()
 {
 this.Factory = null;
 this.Connection = null;
 this.ProviderName = null;
 }
}

Answer 1

You do not need IDisposable in your current implementation as you do not use any unmanaged resources or have any fields or properties which implement IDisposable which your class owns.

Setting the properties to null in the dispose method is unnecessary.

Method names should be Pascal cased and not contain underscores to adhere to standard naming conventions:

private void GetConnectionStringFromAppSettings()

Since the class cannot operate without a connection string or provider, you should throw an exception if they are not set:

private void GetConnectionStringFromAppSettings()
{
 this.Connection = ConfigurationManager.AppSettings.Get("Database");
 if (string.IsNullOrEmpty(this.Connection))
 {
 throw new InvalidOperationException("No connection string found in the configuration");
 }
}

You should use the Connection Strings section of the app.config instead of app settings.

You should resolve the factory once in your constructor instead of on every call to ConnectionReturnDataTable.

ConnectionReturnDataTable should be renamed to something like GetQueryResults.

DbCommand and DbDataAdapter are both IDisposable and should be used in a using statement (or a try/finally which is what a using compiles to).

private DataTable FillDataTable(DbConnection connection, string sql)
{
 using (DbDataAdapter adapter = this.Factory.CreateDataAdapter())
 {
 using (DbCommand command = connection.CreateCommand())
 {
 command.CommandText = sql;
 command.Connection = connection;
 adapter.SelectCommand = command;
 DataTable table = new DataTable();
 adapter.Fill(table);
 return table;
 }
 }
}

The connection property should be called ConnectionString

public string ConnectionString { get; set; }

In fact, the properties should probably become private fields as they shouldn't be modified from outside the class if you are using the app.config to specify their values.

You should be using parameterised queries instead of raw SQL

• dreza: Suggestion. Do we really need the two private methods to set the connection and provider? I would probably consider also making them read-only and just set them directly in the constructor for simplicities sake?? Or, even consider injecting them in the constructor instead...
• Trevor Pilley: No, you are right if the constructor reads the connection strings section and assigns the 3 fields that would be the better approach.