Initialize global repository or in forms? [closed]

Question

Until now we have always initialized the repositories, that containing MySql functions, at the beginning of each Form

Something like that

Form example code:

public partial class FormTest: Form
{
    private readonly ProductRepository prodRep = new ProductRepository();
    private readonly UserRepository userRep = new UserRepository();
    private readonly CarRepository carRep = new CarRepository();
    private readonly RegistryRepository regRep = new RegistryRepository();

    public FormTest()
    {
        InitializeComponent();

        var product = prodRep.Get();

        var listUsers = userRep.GetAll();
    }
}

Repository example code:

namespace Gestionale.DataRepository
{
    public class ProductRepository
    {
        public Product GetId(string id)
        {
            using (var db = new DatabaseConnection())
            {
                const string query = "SELECT * FROM product WHERE id_product = @Id";
                return db.con.QueryFirstOrDefault<Product >(query, new { Id = id });
            }
        }

        public List<Product> GetAll()
        {
            using (DatabaseConnection db = new DatabaseConnection())
            {
                const string query = "SELECT * FROM product";
                return db.con.Query<Product>(query).ToList();
            }
        }
    }
}

We were thinking of creating a new file with all the repositories inside and initializing it at the startup of the program.

1. Is it a suitable procedure?

2. Positive and negative aspects ?

3. Other methods?

Answer 1

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.

public FormTest()
{
    InitializeComponent();
    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)
    : this()
{
    _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)
    : this()
{
    _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)
    : this()
{
    _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)
{
    CreateItem?.Invoke(sender, 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.

Answer 2

Frankly, if your repositories don't have state (i.e. private member variables), their methods should be marked as static, and ergo, each repository class should be marked as static and you have no need to worry about instantiation anywhere. They're essentially utility functions to be called whenever you need them.