CRUD strongly typed data access

Question

I have a data access class:

using CompanyX.DataAccess.CompanyXTableAdapters;

namespace CompanyX.DataAccess
{
    public class CompanyXDataAccess
    {
        #region Invoice

        public CompanyX.InvoiceDataTable GetInvoiceByID(int ID)
        {
            InvoiceTableAdapter ta = new InvoiceTableAdapter();
            using (ta.Connection = new SqlConnection(DB.GetReaderConnectionString()))
            {
                return ta.GetByID(ID);
            }
        }

        public CompanyX.InvoiceDataTable GetInvoice(string userSID)
        {
            InvoiceTableAdapter ta = new InvoiceTableAdapter();
            using (ta.Connection = new SqlConnection(DB.GetReaderConnectionString()))
            {
                return ta.Get(userSID);
            }
        }

        public void DeleteInvoice(int ID)
        {
            InvoiceTableAdapter ta = new InvoiceTableAdapter();
            using (ta.Connection = new SqlConnection(DB.GetWriterConnectionString()))
            {
                ta.Delete1(ID);
            }
        }

        public void UpdateInvoice(Invoice m)
        {
            InvoiceTableAdapter ta = new InvoiceTableAdapter();
            using (ta.Connection = new SqlConnection(DB.GetWriterConnectionString()))
            {
                ta.Update1(m.ID, ... n numbers of params ..., m.ModifiedBy);
            }
        }

        #endregion

        // more regions for modules come in here with CRUD and other queries
    }
}

Is there any way I can improve this code without losing any performance?

Please note: I had to change names in code and trim screenshot for company privacy reasons.

Answer 1

Readability

• Use namespace CompanyX instead of using in every method's return type.
• Create single instance of InvoiceTableAdapter and SqlConnection and reuse it.
• In UpdateInvoice method, instead of passing multiple properties, pass data model itself and use properties which are required inside Update1 method.
• You will need to put the cleanup of the connection in a Dispose() after making the data access class IDisposable. As pointed out by ratchet freak in another answer.

Not sure why you're using 2 SQL connections. I would suggest you to use only one.

After refactoring code should look like this:

    using CompanyX;
    using CompanyX.DataAccess.CompanyXTableAdapters;

    namespace CompanyX.DataAccess
    {
        public class CompanyXDataAccess : IDisposable
        {
            InvoiceTableAdapter ta = new InvoiceTableAdapter();
            SqlConnection readerConnection = new SqlConnection(DB.GetReaderConnectionString());
            SqlConnection writerConnection = new SqlConnection(DB.GetWriterConnectionString());

            public InvoiceDataTable GetInvoiceByID(int ID)
            {
                ta.Connection = readerConnection;
                return ta.GetByID(ID);
            }

            public InvoiceDataTable GetInvoice(string userSID)
            {
                ta.Connection = readerConnection;
                return ta.Get(userSID);
            }

            public void DeleteInvoice(int ID)
            {
                ta.Connection = writerConnection;
                ta.Delete1(ID);
            }

            public void UpdateInvoice(Invoice m)
            {
                ta.Connection = writerConnection                                    
                ta.Update1(m);
            }

            public void Dispose()
            {
                // Do clean up
            }
        }
    }

Answer 2

• Please read: Are #regions an antipattern or code smell?

• Instead of retrieving the connectionstrings for reading and writing each time any of this methods are called, you should retrieve them once and store them in meaningful named variables.

• Instead of explicitly declaring the type e.g InvoiceTableAdapter ta = new InvoiceTableAdapter(); you should make use of the var type because it is obvious by the right hand side what type is assigned. In addition having a name like ta doesn't tell much about its meaning/usage, so you should consider changing that name.

Answer 3

You keep recreating both the adapter and the connection, instead cache them after first creation.

You will need to put the cleanup of the connection in a Dispose() after making the data access class IDisposable.