private readonly ProductRepository prodRep = new ProductRepository();
private readonly UserRepository userRep = new UserRepository();
private readonly CarRepository carRep = new CarRepository();
private readonly RegistryRepository regRep = new RegistryRepository();
From a very pragmatic point of view, there's nothing inherently wrong with newing up your dependencies like this - especially since they're just default constructors without any side-effects.
From an architectural point of view, there's now absolutely no way to bring up that form without having it work with the MySQL database: the form is tightly coupled with its dependencies, and in a maintainable, testable code base, you want exactly the opposite of that.
The main problem is in the form's constructor:
var product = prodRep.Get();
var listUsers = userRep.GetAll();
Turning a blind eye on the extraneous vertical whitespace and the uselessness of these local variables, what's happening here is that the form's constructor is side-effecting, and has a very significant chance of throwing exceptions - two things that directly and severely contradict best practices.
When I do this:
using (var form = new FormTest())
I expect to get a
FormTest instance to work with, period.
What's happening is, I may get a
FormTest instance to work with, and I'm hitting a MySQL database, synchronously at that. If anything goes wrong in any of these hidden repository dependencies, I'm not getting a
FormTest instance. Instead I get an unhandled exception I had no reason to expect.
So, when should the initial load happen then, if not in the form's constructor? Forms have a
Load event (see order of events in Windows Forms) that's exactly for this. Handle the form's
Load event, and move the database work there.
Load += FormTest_Load;
private void FormTest_Load(object sender, EventArgs e)
products = prodRep.Get();
listUsers = userRep.GetAll();
The constructor is invoked by the WinForms designer: having it do database work means everytime the designer is loaded, you're hitting the database. Don't do this.
A constructor should do as little work as possible, and tells its caller what it needs, what its dependencies are.
private readonly ProductRepository _products;
private readonly UserRepository _users;
private readonly CarRepository _cars;
private readonly RegistryRepository _registries;
public FormTest(ProductRepository products, UserRepository users, CarRepository cars, RegistryRepository registries)
_products = products;
_users = users;
_cars = cars;
_registries = registries;
We're still tightly coupled with concrete types, but at least now the dependencies are explicit. What's missing is a unit of work abstraction:
private readonly IUnitOfWork _context;
public FormTest(IUnitOfWork context)
_context = context;
But that's still leaving the form responsible for running the entire show: it's a Smart UI that knows how everything works and does everything.
The form doesn't need to know about repositories or a unit of work; the form needs products, users, cars, registries: it needs a model object that encapsulates the data it wants to present:
private readonly AppModel _model;
public FormTest(AppModel model)
_model = model;
The object responsible for creating the form, is also responsible for populating that model: let's call it the presenter. The presenter's own dependencies include the unit of work, and its job is to update the model as needed, and respond to whatever happens in the view - the form.
So if there's a button on the form that can create a new product, its
Click handler should be as simple as this:
public event EventHandler CreateItem;
private void CreateProductButton_Click(object sender, EventArgs e)
The role of a form isn't to run the entire show and know everything that needs to happen for new data to end up in the database: the role of a form is strictly to present the model to the user, and provide an interface for the user to interact with it.
So the presenter would handle that
CreateItem event, by hitting the database through the repositories (asynchronously?), and then updating the model accordingly - and the view should have data bindings against the model, so no code whatsoever should be needed for this in the view.
Look into the Model-View-Presenter UI pattern if this sounds interesting.