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
 }
}

enter image description here

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.

• RobH: Can I ask why you would prefer to use stored procedures?
• IEatBagels: You're looking for a design review it seems. I think you'd receive better answers on Programmers.SE
• IEatBagels: There might be potential, for sure. But Code Review is here to review already written and working code! Once you have something built that works, bring it here and we'll help you out! :)
• IEatBagels: I guess you're right. I checked with my folks on the chat and it seems that question is okay!
• kayess: To me it sounds like that you are really asking for a design pattern for separating reads and writes. Maybe you are looking for a basic implementation of CQRS on the data access level.

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
 }
 }
 }

• Mathematics: Thanks for the effort +1, But I have lots of Table Adapters though as mentioned in the code comments, that will end up me ending 5 - 10 table adapters makes it look like a code smell - I think
• moarboilerplate: @Mathematics then make this class generic and parameterize the adapter and table types. Should work pretty well for you.

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.