MVP win forms application

Question

In a module of a win forms application I have a hierarchy of classes like BaseRecovery, LoanRecovery and FineRecovery. Both LoanRecovery and FineRecovery inherits from BaseRecovery. All these models have one view called RecoveryForm where you can enter/view loans and fines of employees. and I'm planing to use single Presenter class called RecoveryPresenter which inherits from BasePresenter. We have a DataService Class for db transactions

LoanRecovery class looks like this...

Class LoanRecovery : BaseRecovery
{
    public string LoanID {get;set;}
    public DateTime IssueDate {get;set;}
    public Decimal Amount {get;set;}
    .
    .
    .
}

So I'd do the following in Programe.cs.

IBaseRcovery recovery=null;
IRecoveryForm recoveryForm = new RecoveryForm();
IDataService dataService = new DataService();

BasePresenter presenter = new RecoveryPresenter( recovery, recoveryForm, dataService );
presenter.Show(); // This will display the form recoveryForm

In the presenter I would say

public RecoveryPresenter( IBaseRecover model, IRecoveryForm view, IDataService dataService )
{
    this._Model = model;
    this._View = view;
    this._DataService = dataService;            
    WireUpViewEvents();

}

Now lets say if I need to give a loan, I would run SetModelPropertisFromView() method in the BasePresenter class which use reflection. But before that I should pass an instance of LoanRecovery class (i.e. _Model) to that method together with _View. Like wise we can do same thing for all children classes as follows...

    public void Issue()
    {
        if (_View.Type == "Loan")
        {
            _Model = new LoanRecovery();
            SetModelPropertiesFromView(_Model, _View, _DataService);
            _dataService.InsertLoan(_Model); //Error
        }

        if (_View.Type == "Fine")
        {
            _Model = new FineRecovery();
            SetModelPropertiesFromView(_Model, _View, _DataService);
            _DataService.InsertFine(_Model); //Error
        }

        if (_View.Type == "Insurance")
        {
            _Model = new InsuranceRecovery();
            SetModelPropertiesFromView(_Model, _View, _DataService);
            _DataService.InsertFine(_Model); //Error
        }


    } 

Everything is fine until the last line of the above if blocks. The issue is the data access method in the DataService class requires a child instance not a base class instance.

  public void InsertLoan( LoanRecovery loan)
    {
        using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
        {
            SqlCommand sqlCommand = new SqlCommand("BEGIN INSERT INTO recovery ;
            sqlCommand.Parameters.Add("@empID", SqlDbType.Char).Value = loan.EmployeeID;
            sqlCommand.Parameters.Add("@loanType", SqlDbType.VarChar).Value = loan.Type;
            sqlCommand.Parameters.Add("@loanAmount", SqlDbType.Decimal).Value = loan.FullAmount;
            sqlCommand.Parameters.Add("@loanDuration", SqlDbType.Int).Value = loan.Duration;
            sqlConnection.Open();
            sqlCommand.ExecuteNonQuery();
        }
    }

So I solved the problem like this

    public void Issue()
    {
        if (_View.Type == "Loan")
        {
            LoanRecovery loanModel = new LoanRecovery(); //Creates a child instance
            SetModelPropertiesFromView(loanModel, _View, _DataService);
            _DataService.InsertLoan(loanModel);
        }
    }

Now it's working but my worry is that I'm not using the injected instance through the constructor rather newing a child object which creates a dependency. Can anybody propose a better design to solve this issue please?

Answer 1

There are a number of typos in your code blocks, that lead me to believe the code wasn't taken from the working code in your IDE. I will assume the following:

• IBaseRcovery is really IBaseRecovery
• RecoveryPresenter( IBaseRecover model, ... is really RecoveryPresenter(IBaseRecovery model, ...
• SqlCommand sqlCommand = new SqlCommand("BEGIN INSERT INTO recovery ; is really SqlCommand sqlCommand = new SqlCommand("BEGIN INSERT INTO recovery ...");

That said, the naming feels awkward with an underscore followed by a capital letter:

this._Model = model;
this._View = view;
this._DataService = dataService;

I would rather see this:

_model = model;
_view = view;
_dataService = dataService; 

Or that:

this.model = model;
this.view = view;
this.dataService = dataService; 

But not a mixture of both.

The raison d'être of the underscore prefix is to avoid having to use the this qualifier; if you prefix a field name with an underscore and use this, you're cluttering up your code.

Also convention is camelCase (i.e. lowercase first letter) for private fields, local variables and parameters. PascalCase (i.e. uppercase first letter) is for type and member names (classes, interfaces, delegates, enums, events, methods, etc.).

---

You can declare a variable without initializing it. When it's a reference type, it's set to null until you assign it a reference. Hence, this is perfectly redundant:

IBaseRecovery recovery = null;

You could just do this:

IBaseRecovery recovery;

---

Looks like generics could come in handy here:

public RecoveryPresenter(IBaseRecovery model, IRecoveryForm view, IDataService dataService)

Can turn into a constructor for a generic class:

public class Presenter<TView, TModel> 
    where TView : IView
    where TModel : class
{
    private TModel _model;
    private TView _view;
    private IRepository<TModel> _repository;

    public Presenter(TModel model, TView view, IRepository<TModel> repository)
    {
        _model = model;
        _view = view;
        _repository = repository;
    }

    /* ... */
}

If IBaseRecovery is only a marker interface with no members, get rid of it, it smells.

The key here, is IRepository<TModel>: your DataService has too many responsibilities, its interface will keep getting modified as you add more features to your application (i.e. as you need to deal with more database tables), and that's not very cleanly extensible.

The IRepository<TModel> interface could look something like this:

public interface IRepository<TModel> where TModel : class
{
    void SelectAll();
    void GetById(int id);
    void Insert(TModel model);
    void Update(TModel model);
    void Delete(TModel model);
}

Look into the repository pattern for more information. The idea is that you'll have a family of specialized classes, all presenting the same interface, that perform similar operations on different parts of the underlying database:

public class LoanRepository : IRepository<Loan>

public class FineRepository : IRepository<Fine>

...

You can even derive all these classes from an abstract ReposityBase class (non-generic), and implement the common code only once (i.e. connection set-up & disposal, for example).

Now, if your presenter is going to be dealing with more than a single database table / repository, you have a problem; that problem is solved with a unit of work, which the presenter can depend on:

public class Presenter<TView> 
    where TView : IView
{
    private IUnifOfWork _uow;
    private TView _view;

    public Presenter(IUnitOfWork uow, TView view)
    {
        _uow = uow;
        _view = view;
    }

    /* ... */
}

Repository and Unit-of-Work patterns usually go hand-in-hand; here is an example of an implementation I've written in VBA recently. It's another language and an earlier data access technology, but you'll find many similarities - look at the pattern, not the code itself.

And/or read up on repository+UoW design patterns; adding these to your arsenal should greatly simplify your life.