Skip to main content
added 347 characters in body
Source Link
t3chb0t
  • 44.3k
  • 9
  • 84
  • 190

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?


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?


Source Link
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;