Presenter creating Views in c# MVP

Question

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);
        }
    }
}

Answer 1

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;
}

Answer 2

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;

Answer 3

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?