16
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Is sb a StringBuilder? Why not just include the entire method? Is it the shown snippet that's duplicated within the same method, or you have 4 almost-identical methods? What is (are) the method(s)'s name(s)? Is this ASP.NET? Which page event runs this code? With a little bit more [code] context, you could get some very meaty answers here! \$\endgroup\$ Jan 7, 2015 at 2:43
  • \$\begingroup\$ a1 appears to be the same thing as c. You have 3 times the same code, factorize. breakfastHours appears to be days, not hours. Your switch case is not very pretty, a dict or an array would look better \$\endgroup\$
    – njzk2
    Jan 7, 2015 at 16:46
  • \$\begingroup\$ Does the value have to be a string concatenation of the individual int values? Ideally, this would instead be a bitmask. That would greatly simplify how you work with the values. \$\endgroup\$
    – Dan Lyons
    Jan 7, 2015 at 18:27

2 Answers 2

16
\$\begingroup\$

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.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ string isn't just sexier: it's a language alias for the System.String class, like int is a language alias for the System.Int32 type. For consistency's sake, if you're using the int alias there's no reason not to use the string alias as well. \$\endgroup\$ Jan 7, 2015 at 3:14
  • 3
    \$\begingroup\$ Right now you're using 7 variables to distinguish between the 1st, 2nd, ... 7th day in the week. You might as well use an array of integers and use indices 1 to 7 to signify this. Then you wouldn't need 7 different variables but instead one array where you increment the appropriate indice. \$\endgroup\$ Jan 7, 2015 at 3:34
6
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Personally, I don't think "mondayCount" is necessary. It's not a bad thing, but personally I think it would make the code look a little ugly. My only alternative comment is that if you were going to abbreviate names of days of the week that you should use 3 letters instead of 2, simply because that is a conventional notation for days of the week. "Sun" "Mon" "Tue" "Wed" "Thu" "Fri" "Sat" "Sun" are found commonly throughout calendars. \$\endgroup\$
    – corsiKa
    Jan 7, 2015 at 16:39
  • \$\begingroup\$ I'd agree, if the variable referred to "Monday" but it actually refers to how many Mondays have been encountered in the string, and so appending "Count" makes this more clear to the glancing eye of a maintenance programmer. \$\endgroup\$
    – Nick Udell
    Jan 7, 2015 at 16:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.