3
\$\begingroup\$

I have a c# WinForms application in which I have tried to implement the Model View Presenter pattern with Passive View. I create new Views in the Presenter of my 'home screen'. I was wondering if this is bad practice, and if it is, what an alternative might be to resolve this. Also, general improvements are more than welcome.

I have asked a question before about my MVP implementation here (you can check it out if you want more of an idea about how I have implemented the pattern in other parts of my application.

You can find my code below.

View

public partial class HomeScreenView : Form, IHomeScreenView
{
    private HomeScreenPresenter _presenter;

    // Public members.
    public List<string> ExistingAssessments
    {
        get
        {
            var assessments = new List<string>();
            for(int i = 0; i < recentAssessments.Lines.Count(); i++)
            {
                assessments.Add(recentAssessments.Lines[i]);
            }
            return assessments;
        }
        set
        {
            recentAssessments.Text = "";
            foreach (var assessment in value)
            {
                recentAssessments.Text =  assessment + Environment.NewLine + recentAssessments.Text;
            }
        }
    }

    // Public events.
    public event EventHandler InitializingAssessments;
    public event EventHandler CreatingNewAssessment;
    public event EventHandler AddingNewStandard;
    public event EventHandler OpeningAssessment;
    public event EventHandler GeneratingReport;

    // Initialize homescreen. Create an instance of the presenter with a reference the view itsself.
    public HomeScreenView(IAssessmentsModel model)
    {
        InitializeComponent();
        _presenter = new HomeScreenPresenter(this, model);
    }

    // Fires the initializing assessments event.
    private void HomeScreenView_Activated(object sender, EventArgs e)
    {
        InitializingAssessments?.Invoke(this, EventArgs.Empty);
    }

    // Fires the creating new assessment event.
    private void newAssessmentButton_Click(object sender, EventArgs e)
    {
        CreatingNewAssessment?.Invoke(this, EventArgs.Empty);
    }

    // Fires the adding new standard event.
    private void addStandardButton_Click(object sender, EventArgs e)
    {
        AddingNewStandard?.Invoke(this, EventArgs.Empty);
    }

    // Fires the opening assessment event.
    private void openAssessmentButton_Click(object sender, EventArgs e)
    {
        OpeningAssessment?.Invoke(this, EventArgs.Empty);
    }

    // Fires the generating report event.
    private void generateReportButton_Click(object sender, EventArgs e)
    {
        GeneratingReport?.Invoke(this, EventArgs.Empty);
    }

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

Presenter

public class HomeScreenPresenter
{
    // Private view and model.
    private readonly IHomeScreenView _view;
    private IAssessmentsModel _model;
    private DetailScreenView detailScreenView;
    private StandardScreenView standardScreenView;
    private OpenAssessmentScreenView openAssessmentScreenView;
    private ReOpenAssessmentScreenView reOpenAssessmentScreenView;

    // Initialize the homescreen presenter with an IHomeScreenView and a new assessments model. Subsribe to necessary events.
    public HomeScreenPresenter(IHomeScreenView view, IAssessmentsModel model)
    {
        _view = view;
        _model = model;
        _view.InitializingAssessments += InitializeAssessments;
        _view.CreatingNewAssessment += CreateNewAssessment;
        _view.AddingNewStandard += AddNewStandard;
        _view.OpeningAssessment += OpenAssessment;
        _view.GeneratingReport += GenerateReport;
    }

    // Fills the list of the view with all existing assessments.
    public void InitializeAssessments(object sender, EventArgs e)
    {
        try
        {
            var assessments = _model.GetDataList("Assessments", "assessment_name");

            if (assessments.Any())
            {
                HashSet<string> items = new HashSet<string>(assessments);
                assessments = items.ToList();

                for (int i = 0; i < assessments.Count; i++)
                {
                    assessments[i] = (assessments[i] + ": " + _model.CheckStatus(assessments[i]));
                }

                _view.ExistingAssessments = assessments;
            }
            else _view.ExistingAssessments = new List<string>();
        }
        catch(Exception ex)
        {
            _view.Message("Error - Initializing assessments has failed.\n"
                + "Error message - " + ex.Message);
        }
    }

    // Creates a detail screen view.
    public void CreateNewAssessment(object sender, EventArgs e)
    {
        if (detailScreenView == null || detailScreenView.IsDisposed)
        {
            detailScreenView = new DetailScreenView(_model);
        }
    }

    // Creates a standard screen view.
    public void AddNewStandard(object sender, EventArgs e)
    {
        if(standardScreenView == null || standardScreenView.IsDisposed)
        {
            standardScreenView = new StandardScreenView(new StandardsModel(new DatabaseController()));
        }
    }

    // Creates an open assessment screen view.
    public void OpenAssessment(object sender, EventArgs e)
    {
        if(openAssessmentScreenView == null || openAssessmentScreenView.IsDisposed)
        {
            openAssessmentScreenView = new OpenAssessmentScreenView(_model);
        }
    }

    // Creates a re open assessment screen view.
    public void GenerateReport(object sender, EventArgs e)
    {
        if(reOpenAssessmentScreenView == null || reOpenAssessmentScreenView.IsDisposed)
        {
            reOpenAssessmentScreenView = new ReOpenAssessmentScreenView(_model);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ What's the point of doing this? HashSet<string> items = new HashSet<string>(assessments); assessments = items.ToList(); \$\endgroup\$ – Innat3 Nov 18 '16 at 15:03
  • \$\begingroup\$ I create a HashSet to remove the duplicates, and then convert it back to a List so that I can use it as a DataSource for a Control on my form. \$\endgroup\$ – Marthe Veldhuis Nov 18 '16 at 15:08
  • \$\begingroup\$ @MartheVeldhuis in that case, you may want to go with a more declarative LINQ expression: assessments = assessments.Distinct().ToList(); IIRC, it does roughly the same thing under the covers, but it's a lot more obvious what the intent is. \$\endgroup\$ – Dan Lyons Nov 22 '16 at 19:04
  • \$\begingroup\$ yea I got that from the other's answers as well. thank you. \$\endgroup\$ – Marthe Veldhuis Nov 23 '16 at 7:04
2
\$\begingroup\$

imo, you can simplify this bit

var assessments = _model.GetDataList("Assessments", "assessment_name");

if (assessments.Any())
{
  HashSet<string> items = new HashSet<string>(assessments);
  assessments = items.ToList();

  for (int i = 0; i < assessments.Count; i++)
  {
      assessments[i] = (assessments[i] + ": " + _model.CheckStatus(assessments[i]));
  }

  _view.ExistingAssessments = assessments;
}

to this

var assessments = _model.GetDataList("Assessments", "assessment_name").Distinct();
if (assessments.Any())
{
    //assessments.ForEach(a => a += $":{_model.CheckStatus(a)}"); -> wont update values
    assessments = assessments.Select(a => a += $":{_model.CheckStatus(a)}").ToList();
    _view.ExistingAssessments = assessments;
}
\$\endgroup\$
  • \$\begingroup\$ Hi, I have tried to use your solution. But I am still very new to LINQ and I have a problem because the status is not added to the assessment string. I have tried debugging and I can see that the correct status is collected, but it is not added somehow. Do you know why? \$\endgroup\$ – Marthe Veldhuis Nov 22 '16 at 11:32
  • 1
    \$\begingroup\$ @MartheVeldhuis ahh you are absolutely right, foreach will not update the original list! I have edited my answer (notice you don't need .ToList() in the first line anymore, since we will convert it to list later) \$\endgroup\$ – Innat3 Nov 22 '16 at 11:55
1
\$\begingroup\$

Separation of concerns

public List<string> ExistingAssessments
{
  get
  {
      var assessments = new List<string>();
      for (int i = 0; i < recentAssessments.Lines.Count(); i++)
      {
          assessments.Add(recentAssessments.Lines[i]);
      }
      return assessments;
  }
  set
  {
      recentAssessments.Text = "";
      foreach (var assessment in value)
      {
          recentAssessments.Text = assessment + Environment.NewLine + recentAssessments.Text;
      }
  }
}

This is an interesing property that desperately needs improvement.

It stores the data lines as as string that is apparently being splitted internally and you get back the lines via the Lines property. A lot of work to just store a few lines of text.

You should drop the string concatenation here which is with the + operator - that is slow by the way - and store the lines raw in a list. It's the job of the Text or some other property/method to build a full/formatted-string.

This property should ideally do only this:

public IEnumerable<string> ExistingAssessments
{
    get
    {
        return recentAssessments.Lines.ToList();            
    }
    set
    {
        recentAssessments.Lines = value.ToList();
    }
}

You can ToList the return value and the new value so that you cannot by accident modify it from outside.

Everything else is beyond its responsibility scope.


I'm also not sure about its name. You are not filtering there so the prefix Existing is misleading. One could there are other assesments and the private filed is even called recent assesments. Try to be consistent. Don't change the meaning without a good reason. Why don't simply name it Assessments or like the field RecentAssesments?


Other suggestions

var assessments = new List<string>();
for (int i = 0; i < recentAssessments.Lines.Count(); i++)
{
  assessments.Add(recentAssessments.Lines[i]);
}
return assessments;

This is much less code when done with LINQ:

return recentAssessments.Lines.ToList();

if (assessments.Any())
{
  HashSet<string> items = new HashSet<string>(assessments);
  assessments = items.ToList();

  for (int i = 0; i < assessments.Count; i++)
  {
      assessments[i] = (assessments[i] + ": " + _model.CheckStatus(assessments[i]));
  }

  _view.ExistingAssessments = assessments;
}
else _view.ExistingAssessments = new List<string>();

There's no need for the HashSet, for the Any and the else part. Just create the new list right away and use Distinct. If there are no assessments it will remain simply empty. With C# 6 it's really short:

var assessments = 
    _model.GetDataList("Assessments", "assessment_name")
    .Distinct()
    .Select(x => $"{x}: {_model.CheckStatus(x)}")
    .ToList();

_view.ExistingAssessments = assessments;
\$\endgroup\$
  • \$\begingroup\$ HI, thanks for your feedback. You gave the example to set my ExistingAssessments property by assigning a list to my recentAssessments control. However, this gives me a syntax error because recentAssessments.Lines is an array and you are trying to assign a list to it. Thought i would mention it. \$\endgroup\$ – Marthe Veldhuis Nov 22 '16 at 12:24
  • \$\begingroup\$ @MartheVeldhuis This is just an example. I cannot know which type it really has because it isn't included in your question (or I've overseen it). I'm sure you know what I mean and I see you can fix it ;-) \$\endgroup\$ – t3chb0t Nov 22 '16 at 12:27
  • \$\begingroup\$ Not trying to blame you, just for the clarity of your example ;p. The .Lines property is a string array, which basically means removing the .ToList() part in the setter. \$\endgroup\$ – Marthe Veldhuis Nov 22 '16 at 13:02
0
\$\begingroup\$

It look OK to me except that

private HomeScreenPresenter _presenter;

on the view (form) maybe should be an interface which HomeScreenPresenter implements.

Further: Instead of the presenter reacts on events on the view the view should call "passive" methods on the presenter - but it's a matter of taste maybe.


You are firing the InitializingAssessments event on the form each time it is activated. Shouldn't that be in the load-event?

\$\endgroup\$
  • \$\begingroup\$ I think indeed those first parts are a matter of taste. It is my intention to fire the event every time it is activated, because actions from my other forms often update the assessments and their names. \$\endgroup\$ – Marthe Veldhuis Nov 21 '16 at 16:57

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.