4
\$\begingroup\$

I have a c# WinForms application in which I have tried to implement the Model View Presenter pattern with Passive View. The View derives from a parent IView which propagates the public members and events of the View to its Presenter. In this way, the Presenter can utilize these field and react to the events without relying on the implementation of the View (forms library and such). The View only fires events and performs input validation logic.

I have included one of my Views below. I was wondering if this View is 'Passive' enough, so that my program is compliant with the pattern and if my general idea of the pattern is correct. Any other improvements are welcome as well. This is my first ever application, c# experiece, and implementation of the MVP pattern so bear with me.

View

public partial class StandardScreenView : Form, IStandardScreenView
{
    // Public members.
    public List<string> QuestionStandards
    {
        get { return currentQuestionStandards.Items.Cast<string>().ToList(); }
        set
        {
            currentQuestionStandards.DataSource = value;
            currentQuestionStandards.SelectedItem = null;
        }
    }

    public List<string> MaturityStandards
    {
        get { return currentMaturityStandards.Items.Cast<string>().ToList(); }
        set
        {
            currentMaturityStandards.DataSource = value;
            currentMaturityStandards.SelectedItem = null;
        }
    }
    public List<string> ComplianceStandards
    {
        get { return currentComplianceStandards.Items.Cast<string>().ToList(); }
        set
        {
            currentComplianceStandards.DataSource = value;
            currentComplianceStandards.SelectedItem = null;
        }
    }
    public string SelectedQuestionStandard
    {
        get
        {
            if(currentQuestionStandards != null)
            {
                return currentQuestionStandards.SelectedItem.ToString();
            }
            return null;
        }
        set { currentQuestionStandards.SelectedItem = value; }
    }
    public string SelectedMaturityStandard
    {
        get
        {
            if(currentMaturityStandards != null)
            {
                return currentMaturityStandards.SelectedItem.ToString();
            }
            return null;
        }
        set { currentMaturityStandards.SelectedItem = value; }
    }
    public string SelectedComplianceStandard
    {
        get
        {
            if(currentComplianceStandards != null)
            {
                return currentComplianceStandards.SelectedItem.ToString();
            }
            return null;
        }
        set { currentComplianceStandards.SelectedItem = value; }
    }

    // Public events.
    public event EventHandler<EventArgs> InitializingStandards;
    public event EventHandler<string> AddingQuestionStandard;
    public event EventHandler<string> AddingMaturityStandard;
    public event EventHandler<string> AddingComplianceStandard;
    public event EventHandler<EventArgs> DeletingQuestionStandard;
    public event EventHandler<EventArgs> DeletingMaturityStandard;
    public event EventHandler<EventArgs> DeletingComplianceStandard;

    // Initialize standardscreen. Create an instance of the presenter with a reference the view itsself.
    public StandardScreenView(IStandardsModel model)
    {
        InitializeComponent();
        var standardScreenPresenter = new StandardScreenPresenter(this, model);
        Show();
        this.ActiveControl = questionStandardLabel;
    }

    // Initializes the standards.
    private void StandardScreenView_Activated(object sender, EventArgs e)
    {
        InitializingStandards?.Invoke(this, EventArgs.Empty);
    }

    // Gets the filename and fires the question standard adding event.
    private void newQuestionStandardButton_Click(object sender, EventArgs e)
    {
        Cursor.Current = Cursors.WaitCursor;
        string fileName = GetFileName();

        if (fileName != null)
        {
            AddingQuestionStandard?.Invoke(this, fileName);
        }
    }

    // Gets the filename and fires the maturity standard adding event.
    private void newMaturityStandardButton_Click(object sender, EventArgs e)
    {
        Cursor.Current = Cursors.WaitCursor;
        string fileName = GetFileName();

        if (fileName != null)
        {
            AddingMaturityStandard?.Invoke(this, fileName);
        }
    }

    // Gets the filename and fires the compliance standard adding event.
    private void newComplianceStandardButton_Click(object sender, EventArgs e)
    {
        Cursor.Current = Cursors.WaitCursor;
        string fileName = GetFileName();

        if (fileName != null)
        {
            AddingComplianceStandard?.Invoke(this, fileName);
        }
    }

    // Opens the openfile dialog and checks the result. If the result is OK, the form is closed and the filename is returned.
    public string GetFileName()
    {
        DialogResult result = openFileDialog1.ShowDialog();
        string fileName = openFileDialog1.FileName;

        if (result == DialogResult.OK && (Path.GetExtension(openFileDialog1.FileName) == ".txt"))
        {
            return fileName;
        }
        else if (result == DialogResult.Cancel)
        {
            return null;
        }
        else
        {
            Message("You have selected a file with an illegal extension. Please try again and select a *.txt file.");
            return null;
        }
    }

    // Fires the deleting question standards event.
    private void deleteQuestionStandardButton_Click(object sender, EventArgs e)
    {
        string temp = SelectedQuestionStandard;
        if (temp != null && MessageBox.Show("Are you sure you want to delete this standard from the database?", ""
            , MessageBoxButtons.YesNo) == DialogResult.Yes)
        {
            SelectedQuestionStandard = temp;
            DeletingQuestionStandard?.Invoke(this, EventArgs.Empty);
        }
    }

    // Fires the deleting maturity standards event.
    private void deleteMaturityStandardButton_Click(object sender, EventArgs e)
    {
        string temp = SelectedMaturityStandard;
        if (temp != null && MessageBox.Show("Are you sure you want to delete this standard from the database?", ""
            , MessageBoxButtons.YesNo) == DialogResult.Yes)
        {
            SelectedMaturityStandard = temp;
            DeletingMaturityStandard?.Invoke(this, EventArgs.Empty);
        }
    }

    // Fires the deleting compliance standards event.
    private void deleteComplianceStandardButton_Click(object sender, EventArgs e)
    {
        string temp = SelectedComplianceStandard;
        if (temp != null && MessageBox.Show("Are you sure you want to delete this standard from the database?", ""
            , MessageBoxButtons.YesNo) == DialogResult.Yes)
        {
            SelectedComplianceStandard = temp;
            DeletingComplianceStandard?.Invoke(this, EventArgs.Empty);
        }
    }

    // Displays a message.
    public void Message(string message)
    {
        MessageBox.Show(message);
    }

    // Closes the view.
    public void CloseView()
    {
        Close();
    }

}

IView

public interface IStandardScreenView
{
    // Public members.
    List<string> QuestionStandards { get; set; }
    List<string> MaturityStandards { get; set; }
    List<string> ComplianceStandards { get; set; }
    string SelectedQuestionStandard { get; set; }
    string SelectedMaturityStandard { get; set; }
    string SelectedComplianceStandard { get; set; }

    // Public events.
    event EventHandler<EventArgs> InitializingStandards;
    event EventHandler<string> AddingQuestionStandard;
    event EventHandler<string> AddingMaturityStandard;
    event EventHandler<string> AddingComplianceStandard;
    event EventHandler<EventArgs> DeletingQuestionStandard;
    event EventHandler<EventArgs> DeletingMaturityStandard;
    event EventHandler<EventArgs> DeletingComplianceStandard;

    // Public functions.
    void Message(string message);
    void CloseView();
}

Presenter

public class StandardScreenPresenter
{
    // Private view and model.
    private readonly IStandardScreenView _view;
    private IStandardsModel _model;

    // Initialize the standard screen presenter with an IStandardScreenView and a new assessments model. Subsribe to necessary events.
    public StandardScreenPresenter(IStandardScreenView view, IStandardsModel model)
    {
        _view = view;
        _model = model;
        _view.InitializingStandards += InitializeStandards;
        _view.AddingQuestionStandard += AddQuestionStandard;
        _view.AddingMaturityStandard += AddMaturityStandard;
        _view.AddingComplianceStandard += AddComplianceStandard;
        _view.DeletingQuestionStandard += DeleteQuestionStandard;
        _view.DeletingMaturityStandard += DeleteMaturityStandard;
        _view.DeletingComplianceStandard += DeleteComplianceStandard;
    }

    // Initializes the standards.
    public void InitializeStandards(object sender, EventArgs e)
    {
        _view.QuestionStandards = null;
        _view.MaturityStandards = null;
        _view.ComplianceStandards = null;

        try
        {
            var questionStandards = _model.GetStandards("Questions", "question_standard");
            var maturityStandards = _model.GetStandards("MaturityAnswers", "m_answer_standard");
            var complianceStandards = _model.GetStandards("ComplianceAnswers", "c_answer_standard");

            if (questionStandards != null && questionStandards.Any())
            {
                HashSet<string> items = new HashSet<string>(questionStandards);
                questionStandards = items.ToList();

                _view.QuestionStandards = questionStandards;
            }

            if (maturityStandards != null && maturityStandards.Any())
            {
                HashSet<string> items = new HashSet<string>(maturityStandards);
                maturityStandards = items.ToList();

                _view.MaturityStandards = maturityStandards;
            }

            if (complianceStandards != null && complianceStandards.Any())
            {
                HashSet<string> items = new HashSet<string>(complianceStandards);
                complianceStandards = items.ToList();

                _view.ComplianceStandards = complianceStandards;
            }
        }
        catch(Exception ex)
        {
            _view.Message("Error - initializing standards has failed.\n" +
                "Error message - " + ex.Message);
        }
    }

    // Adds the new question standard file.
    public void AddQuestionStandard(object sender, string standardName)
    {
        try
        {
            _model.AddNewStandard("Questions", "question_id, question_value, question_standard", standardName);
            _view.Message("The new question standard has been added to the database.");
        }
        catch(Exception ex)
        {
            _view.Message("Error - Adding question standard to database has failed, please make sure the question id's are unique and the filepath is valid."
                + Environment.NewLine + "Error message - " + ex.Message);
        }
    }

    // Adds the new maturity standard file.  
    public void AddMaturityStandard(object sender, string standardName)
    {
        try
        {
            _model.AddNewStandard("MaturityAnswers", "m_answer_id, m_answer_value, m_answer_standard", standardName);
            _view.Message("The new maturity standard has been added to the database.");
        }
        catch (Exception ex)
        {
            _view.Message("Error - Adding maturity standard to database has failed, please make sure the answer id's are unique and the filepath is valid."
                + Environment.NewLine + "Error message - " + ex.Message);
        }
    }

    // Adds the new compliance standard file.
    public void AddComplianceStandard(object sender, string standardName)
    {
        try
        {
            _model.AddNewStandard("ComplianceAnswers", "c_answer_id, c_answer_value, c_answer_standard", standardName);
            _view.Message("The new compliance standard has been added to the database.");
        }
        catch (Exception ex)
        {
            _view.Message("Error - Adding compliance standard to database has failed, please make sure the asnwer id's are unique and the filepath is valid."
                + Environment.NewLine + "Error message - " + ex.Message);
        }
    }

    // Deletes the question standard.
    public void DeleteQuestionStandard(object sender, EventArgs e)
    {
        try
        {
            _model.DeleteStandard("Questions", "question_standard", _view.SelectedQuestionStandard);
            _view.Message("The standard has been deleted.");

        }
        catch(Exception ex)
        {
            _view.Message("Error - Deleting standard has failed.\n" +
                "Error message - " + ex.Message);
        }
    }

    // Deletes the maturity standard.
    public void DeleteMaturityStandard(object sender, EventArgs e)
    {
        try
        {
            _model.DeleteStandard("MaturityAnswers", "m_answer_standard", _view.SelectedMaturityStandard);
            _view.Message("The standard has been deleted.");
        }
        catch (Exception ex)
        {
            _view.Message("Error - Deleting standard has failed.\n" +
                "Error message - " + ex.Message);
        }
    }

    // Deletes the compliance standard.
    public void DeleteComplianceStandard(object sender, EventArgs e)
    {
        try
        {
            _model.DeleteStandard("ComplianceAnswers", "c_answer_standard", _view.SelectedComplianceStandard);
            _view.Message("The standard has been deleted.");
        }
        catch (Exception ex)
        {
            _view.Message("Error - Deleting standard has failed.\n" +
                "Error message - " + ex.Message);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I'm not an expert on MVP pattern but it looks nice and clean to me. I have one question: If something goes wrong in the Add/Delete...Standard() methods, then you send a message to the view. Is that enough for the view to know, or should it be notified about which error and state? \$\endgroup\$ – Henrik Hansen Nov 3 '16 at 19:20
  • \$\begingroup\$ I try doing that with the ex.Message property from Exception. That gives some information about what the error is. Thanks for your input \$\endgroup\$ – Marthe Veldhuis Nov 4 '16 at 7:48
  • 1
    \$\begingroup\$ nice, the only thing i would change is remove the redundant null checks on the lists. instead i'd make the interface members IEnumerables and init them to` Enumberable.Empty<string>()` or if you want to stick with list, then just initialize an empty list. then you just need to check for .Any() , cleaner. \$\endgroup\$ – apieceoffruit Nov 4 '16 at 9:38
  • \$\begingroup\$ FWIW I think you nailed the pattern. Well done. \$\endgroup\$ – RubberDuck Nov 4 '16 at 16:27
  • \$\begingroup\$ @RubberDuck That's nice to hear :) \$\endgroup\$ – Marthe Veldhuis Nov 5 '16 at 17:07
4
\$\begingroup\$
event EventHandler<EventArgs> InitializingStandards;
event EventHandler<string> AddingQuestionStandard;

This is not exactly how we implement events.

The EventHandler Delegate already works with EventArgs so it's redundant to use the generic one:

public delegate void EventHandler(object sender, EventArgs e)

it's the same as if you wrote:

event EventHandler InitializingStandards;

but if you use the generic overload then the argument should be derived from EventArgs:

public delegate void EventHandler<TEventArgs>(object sender, TEventArgs e)

The standard signature of an event handler delegate defines a method that does not return a value. This method's first parameter is of type Object and refers to the instance that raises the event. Its second parameter is derived from type EventArgs and holds the event data. If the event does not generate event data, the second parameter is simply the value of the EventArgs.Empty field. Otherwise, the second parameter is a type derived from EventArgs and supplies any fields or properties needed to hold the event data.

EventHandler Delegate

(Emphasis mine)

For some strange reason the delegate does not enforce that and has no where TEventArgs : EventArgs constraint. But if you want to stick to the convention then you should create new types like:

class AddingQuestionStandardEventArgs : EventArgs
{
    AddingQuestionStandardEventArgs(string standardName)
    {
         StandardName = standardName;
    }
    public string StandardName { get; }
}

to use them instead of the string:

event EventHandler<AddingQuestionStandardEventArgs> AddingQuestionStandard;
\$\endgroup\$
  • 3
    \$\begingroup\$ I would add that encapsulating event args in a dedicated class shields the delegate signature from changes, which would otherwise ripple to every handler when a new member needs to be added... although there has to be a way to reuse that EventArgs class for all string events. \$\endgroup\$ – Mathieu Guindon Nov 3 '16 at 15:24
  • \$\begingroup\$ Thank you both for your input! @Mat'sMug It took me a while to decipher what you are trying to say since im just a newbie :p. I'll try to implement your suggestions. \$\endgroup\$ – Marthe Veldhuis Nov 4 '16 at 7:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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