-2
\$\begingroup\$

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?
\$\endgroup\$

closed as off-topic by t3chb0t, yuri, pacmaninbw, Jamal May 17 at 2:57

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – t3chb0t, yuri, pacmaninbw, Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Depends. What's in these repositories' constructors? \$\endgroup\$ – Mathieu Guindon May 16 at 13:36
  • 3
    \$\begingroup\$ FWIW you would get much more useful feedback if you included your actual real code rather than a simple illustrative example that narrows everything down to one specific issue/question. This is Code Review, not Stack Overflow ;-) \$\endgroup\$ – Mathieu Guindon May 16 at 13:39
  • \$\begingroup\$ @MathieuGuindon It's only a collection of function. There isn't a costructor. \$\endgroup\$ – Lorenzo Belfanti May 16 at 13:39
  • \$\begingroup\$ There's a very interesting architecture discussion about coupling, dependencies, and UI design patterns that could happen if you were interested in feedback on any/all aspects of the code (real code), as mandated by the site's help center \$\endgroup\$ – Mathieu Guindon May 16 at 13:43
  • \$\begingroup\$ @MathieuGuindon I added an example of the repository code \$\endgroup\$ – Lorenzo Belfanti May 16 at 13:51
3
\$\begingroup\$
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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ You're right, we never thought about it! \$\endgroup\$ – Lorenzo Belfanti May 16 at 14:26
  • \$\begingroup\$ The OP's constructor is doing database work. That means the designer stops working if the connection string has a typo, the command times out, or anything else goes wrong with hitting the MySQL database in the constructor. \$\endgroup\$ – Mathieu Guindon May 16 at 14:36
  • \$\begingroup\$ Agree - that's a repudiation of the form class design, while I was addressing the repository class design. \$\endgroup\$ – Jesse C. Slicer May 16 at 14:46
  • \$\begingroup\$ @MathieuGuindon if i move the call of function in the form load event, like your answer, can this method be considered valid ? In the case that I showed you, yes the calls are in the constructor, but I have other forms where the functions are called inside the form load or only through the buttons. \$\endgroup\$ – Lorenzo Belfanti May 16 at 14:47
  • \$\begingroup\$ @LorenzoBelfanti it's valid if it works, I guess. Whether it's well-architected is a whole other question. If it's just a throw-away prototype, "Smart UI" is probably fine. The problem is when the prototype ends up being the production app, and then needs to grow features - it tends to grow hair and tentacles as well! If your app is any decent size or of any significant importance, seriously consider looking into UI design patterns suitable for WinForms applications. \$\endgroup\$ – Mathieu Guindon May 16 at 14:53

Not the answer you're looking for? Browse other questions tagged or ask your own question.