0

I am working on a Winforms application with EF database-first approach, when my application loads in the home page I need to get some data and populate the form.

Most of the controls on my form are combobox controls which get data from database tables and there are approximately 20 combobox controls, so this means I have almost 20 methods which are calling the database and bind the data to combobox controls.

I invoke these methods in the Form_Load method.

I am not satisfied with the approach I followed, need suggestions on how this can be improved.

My BAL looks like below.

Public List<ContractEngineers> GetContractEngineers()
{
    using(EmployeeEntities contractEng = new EmployeeEntities())
    {
        List<ContractEngineers> contractEngineersList = new List<ContractEngineers>();
        contractEngineersList = /* Code that gets the total list of contract employees. */
    }
}

Public List<PayRollEmployees> GetPayRollEmployees()
{
    using(EmployeeEntities contractEng = new EmployeeEntities())
    {
         List<PayRollEmployees> payRollEmployeesList = new List<PayRollEmployees>(); 
         payRollEmployeesList = /* Code that gets the total list of payroll  employees. */
    }
}

There are almost 20 methods on my form which look similar, they get data and bind to a combobox, in order to make this better, I created a common class called 'CommonComboBox' which can be used for all the comboboxes and I have removed all the classes like 'ContractEngineers', 'PayRollEmployees'

public class CommonComboBox
{
    public virtual int Id {get;set;}
    public virtual string Name {get;set;}
}

My question is: instead of using the using block in all the methods which just open the connection and close the connection, is it a better approach declaring initializing 

EmployeeEntities EMPEntities = new EmployeeEntities()

at the top and using the object EMPEntities in all the methods?

Need some suggestions on how this code can be improved as I am not happy with this code

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
Developer
  • 45
  • 3

2 Answers2

0

Functionally there isn't anything bad or inefficient about the code as you describe, but it does have some limitations.

  • The multiple operations are not contained in a single transaction.
  • Entities cannot be shared easily between requests.
  • It cannot be tested in isolation of the database.

The most common solution to the first two points is to use an IoC container to manage the lifetime scope for a DbContext across the request, and dependency injection to inject that scoped DBContext to the page/services where it is needed. Common IoC containers include Autofac, Unity, or Castle Windsor. This way your pages would contain a private DbContext variable that is initialized in the Constructor. The container constructs the single DBContext to use for the request, and is configured to construct the Webform class instances for ASP.Net so that when these pages and related services are constructed, the DbContext is provided. This ensures that all requests are made across the same DbContext instance meaning operations for SaveChanges apply all at once, and methods can share entity references without complex and error-prone detach/attach logic. Have a look at the documentation for Autofac for examples. (https://autofaccn.readthedocs.io/en/latest/integration/webforms.html)

For the 3rd point, this may or may not be a concern. In these cases I utilize a repository pattern for code to fetch entities. The repository returns IQueryable via EF's Linq operations. With a repository layer defined between the business logic (pages, services, etc.) and the database (DbContext) it is now a simple matter to mock out the data access. Mocks return collections of entities as desired using AsQueryable. If you want to get a bit more advanced and leverage Async then this can help wrap mocked IQueryable collection results. (Unit-testing .ToListAsync() using an in-memory)

Steve Py
  • 26,149
  • 3
  • 25
  • 43
0

Steve Py answer covers WebForm (web applications in general), and also contains some good general guidelines. Specific to your question regarding WinForm, yes you could use class level variable to hold the context, even you could have it as singleton for you windows application. But each of these approaches have some pros and cons that you need to consider. Holding context in a larger scope means that you have to manage it, for example in case of losing connection or corrupted connection you have to have some code to handle the cases gracefully. By limiting the scope and wrapping connection in using you take advantage of some features (connection and resources being disposed properly) so based on application and performance and other requirements you may keep context or connection available in broader scope.

P Motameni
  • 31
  • 4