Critique my simple MVP Winforms app

Answer 1

Everything looks good the only possible level I would go further is to abstract away the logic for saving the file and have that handled by providers so later you could easily flex in alternate saving methods such as database, email, cloud storage.

IMO anytime you deal with touching the file system it's always better to abstract it away a level, also makes mocking and testing alot easier.

Answer 2

The only way to get any closer to a perfect MVP passive view pattern would be to write your own MVP triads for the dialogs instead of using the WinForms dialogs. Then you could move the dialog creation logic from the view to the presenter.

This gets into the topic of communication between mvp triads, a topic which is usually glossed over when examining this pattern. What I've found works for me is to connect triads at their presenters.

public class PersistenceStatePresenter
{
    ...
    public Save
    {
        string sFilename;

        if (_model.Filename == _model.DefaultName || _model.Filename == null)
        {
            var openDialogPresenter = new OpenDialogPresenter();
            openDialogPresenter.Show();
            if(!openDialogPresenter.Cancel)
            {
                return; // user canceled the save request.
            }
            else
                sFilename = openDialogPresenter.FileName;

        ...

The Show() method, of course, is responsible for showing an unmentioned OpenDialogView, which would accept the users input and pass it along to the OpenDialogPresenter. In any case, it should be starting to become clear that a presenter is an elaborate middleman. Under different circumstances, you might be tempted to refactor a middleman out but here its is intentional to:

• Keep logic out of the view, where it is harder to test
• Avoid direct dependencies between the view and the model

At times I've also seen the model used for MVP triad communication. The benefit of this is the presenter's don't need to know each other exist. Its usually accomplished by setting a state in the model, which triggers an event, which another presenter then listens for. An interesting idea. One I've not used personally.

Here's a few links with some of the techniques others have used to deal with triad communication:

• http://www.rickardnilsson.net/post/The-Humble-dialog-v2.aspx
• http://spin.atomicobject.com/2008/01/30/presenter-first-get-your-triads-talking/

Answer 3

One thing I like to do is get rid of direct View to Presenter communication. The reason for this is the view is at the UI level and the presenter is at the business layer. I don't like my layers to have inherent knowledge of each other, and I seek to limit the direct communication as much as possible. Typically, my model is the only thing that transcends layers. Thus the presenter manipulates the view through the interface, but the view doesn't take much direct action against the presenter. I like the Presenter to be able to listen to and manipulate my view based on reaction, but I also like to limit the knowledge my view has of its presenter.

I'd add some events to my IPersistenceStateView:

event EventHandler Save;
event EventHandler Open;
// etc.

Then have my Presenter listen to those events:

public PersistenceStatePresenter(IPersistenceStateView view)
{
    _view = view;

    _view.Save += (sender, e) => this.Save();
    _view.Open += (sender, e) => this.Open();
   // etc.

   InitializeModel();
   InitializeView();
}

Then change the view implementation to have the button clicks fire the events.

This makes the presenter act more like a puppetmaster, reacting to the view and pulling its strings; therein, removing the direct calls on the presenter's methods. You'll still have to instantiate the presenter in the view, but that's about the only direct work you'll do on it.