# Collecting 3 years of data and building a string

I am collecting 3 years of data and building a string that will be used with an ASP Literal control. The method GetListOfMeetingBygroupId takes in a groupID and year and will return meeting for that group and year.

    MeetingService ms = new MeetingService();
int groupId = 574;

//Meeting

int totalYearCount = 3;
StringBuilder sbMeeting = new StringBuilder();
for (int x = 1; x <= totalYearCount; x++)
{

foreach (var meeting in list)
{
sbMeeting.Append("<p>");

DateTime meetingendDate = Convert.ToDateTime(meeting.MeetingDateList.Select(c => c.StartDate).Max());
DateTime meetingstartDate = Convert.ToDateTime(meeting.MeetingDateList.Select(c => c.StartDate).Min());

int totalMeetingPresentation = ms.GetListOfPresentationsBymeetingId(meeting.MeetingId).Count();

sbMeeting.Append(meetingstartDate.ToString("dddd, MMMM dd  yyyy"));
sbMeeting.Append("<br>");
sbMeeting.Append(meeting.Type.Name);
sbMeeting.Append("<br>");

if (totalMeetingPresentation > 0)
{
sbMeeting.AppendFormat("<a class='body'href=meetingdetails.aspx?meetingId={0}>{1}</a>", meeting.MeetingId, "Minutes and Presentations");
}
else
{

var meetingMinFile = ms.GetMeetingFile(meeting.MeetingId);
sbMeeting.AppendFormat("<td><a href=../../../Common/FileManager.ashx?FileManagerId={0}>{1}</a></td>", meetingMinFile.FileManagerId, "Minutes");

}
sbMeeting.Append("<p>");
sbMeeting.Append("<br>");
sbMeeting.Append("</p>");
}
}

• Have you tested the output of this code? I can see multiple broken HTML tags in there. – Jeff E Dec 9 '19 at 20:45

I'll focus on the C# part of the code, but please run the output HTML through a validator because at first glance I count no fewer than five HTML tags that are mismatched, misplaced, or simply broken.

# Outer loop

You're not using the loop variable x anywhere. This suggests that the loop should be refactored around the thing that you're actually iterating over, which seems to be a range of years.

Each year is just an integer; you can get a range of integers using Enumerable.Range. You want the most recent totalYearCount years (inclusive of this year), so start the range at nowYear - totalYearCount + 1. You want this year first, so reverse the sequence too.

foreach (var year in Enumerable
.Range(aDate.Year - totalYearCount + 1, totalYearCount)
.Reverse())
{
var list = ms.GetListOfMeetingBygroupId(groupId, year);
// ...
}


You can replace aDate.ToString("yyyy") accordingly.

# Naming

When using camelCase and PascalCase, each distinct word should start with a capital letter. GetListOfMeetingByGroupId, GetListOfPresentationsByMeetingId, meetingStartDate, meetingEndDate, and so on.

Type prefixes like sb don't help much, especially when abbreviated. Rename that variable to better reflect what it actually is: it's a builder for the output, so maybe outputBuilder instead of sbMeeting.

list is a really generic name for something we know to be a collection of meetings, so call it meetings or meetingList.

# Code we haven't seen

It's advised to post or link to all supporting code. It's at least relevant because of this here:

Convert.ToDateTime(meeting.MeetingDateList.Select(c => c.StartDate).Max());


What's the type of c.StartDate? You're converting it to a DateTime after getting the maximum date -- so it's probably not a DateTime already -- which begs the question how are you sorting the dates? I suspect you might've wanted the order switched:

meeting.MeetingDateList.Max(c => Convert.ToDateTime(c.StartDate));


Note that I've used the overload for Max that takes a selector so that I can avoid the unnecessary Select.

Your spacing is inconsistent. Consistent spacing would make for better readability, so I offer these suggestions:

• Use exactly one indent per {} block -- your inner foreach loop is double-indented.
• Avoid empty lines of padding within {} blocks -- see your for loop and the else block.
• Parameters should be separated by a comma and one space -- see your first sbMeeting.AppendFormat(...).

All of those hard-coded HTML strings should be refactored out into some kind of template. They have no business being encoded inline in the method like that.

• What do you mean template? – Jefferson Dec 10 '19 at 20:57

These lines look nice and readable

   sbMeeting.Append("<p>");
sbMeeting.Append("<br>");
sbMeeting.Append("</p>");


but if you want to have a little more performance, just use one string.

   sbMeeting.Append("<p><br></p>");