Skip to main content
1 of 2
t3chb0t
  • 44.3k
  • 9
  • 84
  • 190

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.


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;
t3chb0t
  • 44.3k
  • 9
  • 84
  • 190