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;

    }
}

Trevor Pilley · Accepted Answer · 2012-10-14 22:12:01Z

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

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... — dreza, Oct 15 '12 at 2:08
No, you are right if the constructor reads the connection strings section and assigns the 3 fields that would be the better approach. — Trevor Pilley, Oct 15 '12 at 11:40

asked	3 years ago
viewed	4108 times
active	3 years ago

current community

your communities

more stack exchange communities

reusable data access class in C#

1 Answer 1

Your Answer

Not the answer you're looking for? Browse other questions tagged c# .net database or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

reusable data access class in C#

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c# .net database or ask your own question.

Related

Hot Network Questions