The Breakfast Club attendance

Question

This code checks a string called breakfastHours similar to

1567

which translates to Monday, Friday, Saturday, Sunday.

1 = Monday

2 = Tuesday

....

7 = Sunday

The user then checks the boxes for the days they visited the club, which is all handled within jQuery later on, with the chkB{0} tags.

 #region BuildOpeningHours
private string BuildOpeningHours() {

    StringBuilder sb = new StringBuilder();
    string hoursPrefix = "Opening";

    if (listing.ListingTypeParentName == "restaurants-dining") {
        //display opening hours
        if (!String.IsNullOrEmpty(listing.BreakfastOpeningDays) || !String.IsNullOrEmpty(listing.LunchOpeningDays) || !String.IsNullOrEmpty(listing.DinnerOpeningDays)) {
            sb.Append(OpeningHoursTable(listing.BreakfastOpeningDays, listing.LunchOpeningDays, listing.DinnerOpeningDays, listing.LateOpeningDays));
        } else if (!String.IsNullOrEmpty(listing.OpeningHours)) {
            sb.AppendFormat("<p itemprop=\"openingHours\">{0}</p>", listing.OpeningHours);
        }

    } else if (listing.ListingTypeParentName == "accommodation") {
        //display reception hours
        if (!String.IsNullOrEmpty(listing.OpeningHours)) {
            hoursPrefix = "Reception";
            sb.AppendFormat("<p>{0}</p>", listing.OpeningHours);
        }
    } else {
        //display opening hours
        if (!String.IsNullOrEmpty(listing.OpeningHours)) {
            sb.AppendFormat("<p>{0}</p>", listing.OpeningHours);
        }
    }

    if (sb.Length > 0) {
        return string.Format("<div id=\"opening-hours\"><h3>{2} {0} Hours</h3>{1}</div>", hoursPrefix, sb.ToString(), listing.ListingName);
    } else {
        return "";
    }
}

private string OpeningHoursTable(string breakfastHours, string lunchHours, string dinnerHours, string lateHours) {

    StringBuilder sb = new StringBuilder();
    int mo = 0;
    int tu = 0;
    int we = 0;
    int th = 0;
    int fr = 0;
    int sa = 0;
    int su = 0;

    sb.Append("<table class=\"openinghours\" cellpadding=\"0\" cellspacing=\"0\">");
    sb.Append("<tr class=\"days\">");
    sb.Append("<th class=\"void\"><a href=\"\" class=\"saveEdit\" id=\"save-openingHours\" data=\"hastwo\">Save</a></th>");
    sb.Append("<th><span>M</span></th>");
    sb.Append("<th>T</th>");
    sb.Append("<th><span>W</span></th>");
    sb.Append("<th>T</th>");
    sb.Append("<th>F</th>");
    sb.Append("<th>S</th>");
    sb.Append("<th>S</th>");
    sb.Append("</tr>");

    //breakfast opening hours
    sb.Append("<tr>");
    sb.Append("<th>Bfast</th>");
    char a1 = '1';
    for (char c = '1'; c <= '7'; c++)
    {
        if (breakfastHours.Contains(c))
        {
            sb.AppendFormat("<td class=\"active-box\"><input id=\"chkB{0}\"style=\"margin: 0px;\" type=\"checkbox\" checked></td>",a1);
            switch (c)
            {
                case '1': mo++; break;
                case '2': tu++; break;
                case '3': we++; break;
                case '4': th++; break;
                case '5': fr++; break;
                case '6': sa++; break;
                case '7': su++; break;
            }
        }
        else
        {
            sb.AppendFormat("<td class=\"inactive-box\"><input id=\"chkB{0}\"style=\"margin: 0px;\" type=\"checkbox\"></td>",a1);
        }
        a1++;
    }
    sb.Append("</tr>");
    //lunch opening hours
    sb.Append("<tr>");
    sb.Append("<th>Lunch</th>");
    char a2 = '1';
    for (char c = '1'; c <= '7'; c++)
    {
        if (lunchHours.Contains(c))
        {
            sb.AppendFormat("<td class=\"active-box\"><input id=\"chkL{0}\" style=\"margin: 0px;\" type=\"checkbox\" checked></td>", a2);
            switch (c)
            {
                case '1': mo++; break;
                case '2': tu++; break;
                case '3': we++; break;
                case '4': th++; break;
                case '5': fr++; break;
                case '6': sa++; break;
                case '7': su++; break;
            }
        }
        else
        {
            sb.AppendFormat("<td class=\"inactive-box\"><input id=\"chkL{0}\" style=\"margin: 0px;\" type=\"checkbox\"></td>", a2);
        } 
        a2++;
    } 
    sb.Append("</tr>");

    //dinner opening hours
    sb.Append("<tr>");
    sb.Append("<th class=\"last\">Dinner</th>");
    char a3 = '1';
    for (char c = '1'; c <= '7'; c++)
    {
        if (dinnerHours.Contains(c))
        {
            sb.AppendFormat("<td class=\"active-box\"><input id=\"chkD{0}\" style=\"margin: 0px;\" type=\"checkbox\" checked></td>", a3);
            switch (c)
            {
                case '1': mo++; break;
                case '2': tu++; break;
                case '3': we++; break;
                case '4': th++; break;
                case '5': fr++; break;
                case '6': sa++; break;
                case '7': su++; break;
            }
        }
        else
        {
            sb.AppendFormat("<td class=\"inactive-box\"><input id=\"chkD{0}\" style=\"margin: 0px;\" type=\"checkbox\"></td>", a3);
        }
        a3++;
    } 
    sb.Append("</tr>");

    //late opening hours
    sb.Append("<tr>");
    sb.Append("<th class=\"last\">Late</th>");
    char a4 = '1';
    for (char c = '1'; c <= '7'; c++)
    {
        if (lateHours.Contains(c))
        {
            sb.AppendFormat("<td class=\"active-box\"><input id=\"chkS{0}\" style=\"margin: 0px;\" type=\"checkbox\" checked></td>", a4);
            switch (c)
            {
                case '1': mo++; break;
                case '2': tu++; break;
                case '3': we++; break;
                case '4': th++; break;
                case '5': fr++; break;
                case '6': sa++; break;
                case '7': su++; break;
            }
        }
        else
        {
            sb.AppendFormat("<td class=\"inactive-box\"><input id=\"chkS{0}\" style=\"margin: 0px;\" type=\"checkbox\"></td>",a4);
        }
        a4++;
    }
    sb.Append("</tr>");

    sb.Append("</table>");

    sb.Append("<p>Seasonal variations may occur.</p>");
    sb.Append("<meta itemprop=\"openingHours\" content=\"");
    
    sb.Append("\"/>");
    return sb.ToString();
}
#endregion

mo,tu,we,th,fr,sa&su are all integers to be sent back to the server later on.

This code is duplicated four times for Breakfast hours, Lunch hours, Dinner hours, Late hours.

Answer 1

First a general remark: EWWW. You're creating HTML as a string and returning that to the client. That's not how we do it in a backend (at least, that what I assume I'm looking at). The proper approach would be to send data back (so in your case: an array of integers) and inject that clientside inside HTML.

---

The Build prefix is typically only used in the Builder pattern, which you don't use here. GetOpeningHours() would be just fine since you're not actually building instances.

---

I personally prefer string over String because it is sexier but I understand if not everyone has seen the light yet. However, do keep the consistency and either stick to string or String but not a combination of both.

---

You're not using any form of globalization. Intended? Adding a resourcebundle isn't much work and you'll finally be able to support Swahili.

---

Return string.Empty instead of "". It tells whoever reads your code that you really did intend to return an empty string and it wasn't just a typo.

---

string.Format("<div id=\"opening-hours\"><h3>{2} {0} Hours</h3>{1}</div>", hoursPrefix, sb.ToString(), listing.ListingName);

No. No no no no no. We count from 0 to 2, and that is the order I expect the format to adhere. Don't introduce subtle bugs by switching them around.

---

I'm okay with you using 7 separate ints but at least spell their entire names. Nbdy lks abbrevs, yh?

---

I have no idea what a1 is. Give it a meaningful name. And make it an int if you're going to count with it.

---

If you don't want to use multiple variables, may I suggest an array? Nothing fancy, just int[8] and fill it from 1 to 7. Then you can switch your switch to days[i]++.

Note however that this requires a different loop or usage of char.GetNumericValue().

---

Your code generates multiple <th> elements. I'm not sure if that's correct HTML (I couldn't find something that discourages it though).

---

Lastly: you never use your daily values, after going through all this trouble to calculate them.

Answer 2

Jeroen has got design pretty much covered in their answer, so I shall cover style.

Var

Use the var keyword when defining local variables where the right hand side of the definition makes the type obvious. This looks cleaner and saves time when it comes to changing types during refactoring.

e.g.

StringBuilder sb = new StringBuilder();
int mo = 0;
int tu = 0;
int we = 0;
int th = 0;
int fr = 0;
int sa = 0;
int su = 0;

should be

var sb = new StringBuilder();
var mo = 0;
var tu = 0;
var we = 0;
var th = 0;
var fr = 0;
var sa = 0;
var su = 0;

You should also use var when declaring foreach and for loop iterators.

e.g.

for (char c = '1'; c <= '7'; c++)

should be:

for (var c = '1'; c <= '7'; c++)

Naming

As Jeroen said, you should avoid shortened form variable names. Extra characters on a variable's name are free, and work wonders for a maintenance programmer down the line.

e.g.

int mo = 0;

would better be:

int mondayCount = 0;

You can probably get away with naming your StringBuilder sb since it's quite a common naming scenario, although there's no harm in calling it stringBuilder.

Commenting

Your comments appear to explain what the code does. That is the job of the code itself. Instead, use your comments to explain why you made the code the way it is, for example any quirks you had to work around, deviations from the expected norm, etc.