HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

The Breakfast Club attendance

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
breakfasttheclubattendance

Problem

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("{0}", listing.OpeningHours);
}

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

if (sb.Length > 0) {
return string.Format("{2} {0} Hours{1}", 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("");
sb.Append("");
sb.Append("Save");
sb.Append("M");
sb.Append("T");
sb.A

Solution

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("{2} {0} Hours{1}", 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 `` 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.

Code Snippets

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

Context

StackExchange Code Review Q#75868, answer score: 16

Revisions (0)

No revisions yet.