Collecting 3 years of data and building a string

Question

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();
    DateTime aDate = DateTime.Now;
    for (int x = 1; x <= totalYearCount; x++)
    {

        var list = ms.GetListOfMeetingBygroupId(groupId, aDate.Year);

        sbMeeting.AppendFormat("<p><b>{0}</p></b>",aDate.ToString("yyyy"));

        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>");
       }
        aDate = aDate.AddYears(-1);
    }

Answer 1

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.

General readability

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.

Answer 2

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