String concatenation challange to be more efficient with DayOfWeek and Lists

Question

I have this challange that I finished which asked to print out a string according to the provided schedule. Here is an example:

var restaurant = new Restaurant(
            new OpeningHour(8,16), // Sunday
            new OpeningHour(8,17), // Monday
            new OpeningHour(8,17), // Tuesday
            new OpeningHour(8,17), // Wednesday
            new OpeningHour(8,16), // Thursday
            new OpeningHour(8,16), // Friday
            new OpeningHour(8,16)  // Saturday
        );

expected output result = "Sun, Thu - Sat: 8-16, Mon - Wed: 8-17"

What I did was essentially:

• Create a List of Days, OpenHours, and CloseHours
• Create a HashSet of the days so that I can compare the days
• Create a for loop according to HashSet and Days
• Seperate the Start, Middle, and Ending
• Concatenate the result according to the open and close hours as well as the gap between days

I have tried my best but I know for a fact that my code is not efficient at all, instead messy. I am trying to improve my C# skills please help. Here is my messy code:

namespace Livit
{
    using System;
    using System.Collections.Generic;
    using System.Linq;
    
    public class Restaurant
    {
        public WeekCollection<OpeningHour> OpeningHours { get; private set; }

        public Restaurant() {
            // No opening hours available for restaurant
        }

        public Restaurant(OpeningHour monday, OpeningHour tuesday, OpeningHour wednesday, OpeningHour thursday, OpeningHour friday, OpeningHour saturday, OpeningHour sunday)
        {
            OpeningHours = new WeekCollection<OpeningHour>(monday, tuesday, wednesday, thursday, friday, saturday, sunday);
        }
        
        // THE EMPHASIS OF THE CHALLANGE IS THIS FUNCTION RIGHT HERE!!!
        // Parse the date into desired format
        public string DateParser(List<DayOfWeek> days, List<TimeSpan> openHours, List<TimeSpan> closeHours)
        {
          HashSet<string> availableRanges = new HashSet<string>();
          List<string> timeRanges = new List<string>();
          DayOfWeek current = DayOfWeek.Sunday;
          string result = "";
          for (int i = 0 ; i < days.Count; i++){
            string timeRange = openHours[i].ToString().Substring(1,1)+'-'+closeHours[i].ToString().Substring(0,2);
            availableRanges.Add(timeRange);
            timeRanges.Add(timeRange);
          }
          List<string> arToList= availableRanges.ToList();
          for (int i = 0 ; i < arToList.Count; i++)
            {
              for (int j = 0 ; j < timeRanges.Count; j++){
                if(timeRanges[j] == arToList[i]){
                  
                  
                  // First Item
                  if(j==0 ){
                    result += days[j].ToString().Substring(0,3);
                  }
                  // Last Item
                  else if(j==timeRanges.Count-1){
                    char last = result.Last();
                    if(last != ' '){
                        result += " - ";
                      } 
                    result += days[j].ToString().Substring(0,3);
                  }
                  // Everything in the middle
                  else{
                    if(days[j]-current > 1){
                      result += ", ";
                    }
                    if(timeRanges[j] != timeRanges[j-1] ){
                      result += days[j].ToString().Substring(0,3);
                    } else if (timeRanges[j] == timeRanges[j-1]){
                      char last = result.Last();
                      if(last != ' '){
                        result += " - ";
                      } 
                      if(timeRanges[j] != timeRanges[j+1]){
                        result += days[j].ToString().Substring(0,3);
                      }
                    }
                  }
                  
                  current = days[j];
                } 
              }
              
              result += ": " + arToList[i];
              if(i!=arToList.Count-1){
                result += ", ";
              }
            } 
          Console.WriteLine(result);
          return result;
        }

        public string GetOpeningHours()
        {
            // Declare List for each attribute
            List<DayOfWeek> days = new List<DayOfWeek>();
            List<TimeSpan> openHours = new List<TimeSpan>();
            List<TimeSpan> closeHours = new List<TimeSpan>();
            // Call the opening and closing hours from each day and feed into new array
            foreach (DayOfWeek day in Enum.GetValues(typeof(DayOfWeek)).OfType<DayOfWeek>().ToList()) {
              TimeSpan openHour = OpeningHours.Get(day).OpeningTime;
              TimeSpan closeHour = OpeningHours.Get(day).ClosingTime;
              days.Add(day);
              openHours.Add(openHour);
              closeHours.Add(closeHour);
            }
            return DateParser(days,openHours,closeHours);
            
            
            
            throw new NotImplementedException();
        }
    }

    public class OpeningHour
    {
        public TimeSpan OpeningTime { get; private set; }
        public TimeSpan ClosingTime { get; private set; }

        public OpeningHour(TimeSpan openingTime, TimeSpan closingTime)
        {
            OpeningTime = openingTime;
            ClosingTime = closingTime;
        }

        public OpeningHour(int openingHour, int closingHour)
        {
            OpeningTime = TimeSpan.FromHours(openingHour);
            ClosingTime = TimeSpan.FromHours(closingHour);
        }

    }

    public class WeekCollection<T>
    {
        private Dictionary<DayOfWeek, T> _collection;

        public WeekCollection(T sunday, T monday, T tuesday, T wednesday, T thursday, T friday, T saturday)
        {
            _collection = new Dictionary<DayOfWeek, T>();
            _collection.Add(DayOfWeek.Sunday, sunday);
            _collection.Add(DayOfWeek.Monday, monday);
            _collection.Add(DayOfWeek.Tuesday, tuesday);
            _collection.Add(DayOfWeek.Wednesday, wednesday);
            _collection.Add(DayOfWeek.Thursday, thursday);
            _collection.Add(DayOfWeek.Friday, friday);
            _collection.Add(DayOfWeek.Saturday, saturday);
        }

        public T Get(DayOfWeek dayOfWeek)
        {
            return _collection[dayOfWeek];
        }

    }
}

Currently, I am still trying to find a better way in doing this challange. Any help would be appriciated.

P.S. I highlighted the part where my concatenation is occuring, this part is basically the emphasis of the whole challange

Answer 1

Notes

• constructor:

  • Not fully utilizing the WeekCollection<T>, which is why it has too many arguments for one constructor.
  • no validations
  • default constructor is not private to enforce using the custom constructor, which adds more unneeded validation requirements to the OpeningHours.
  • OpeningHours is not fully immutable, you can still modify the collection.

• The implementation of WeekCollection<T> is not needed, since it only restricted the underlying dictionary to 7 elements in week day order, which you can do in one line of code!.

• The implementation of OpeningHour is good, but short! it would be better if you add DayOfWeek property to it, and use it in any array or collection as an object-model would be better.

• DateParser:

  • arguments are un-chained, which either adds more complexity or invalidate the results.
  • misleading method name, Parser supposed to parse it to some system or custom type, not getting the string representation of the date.
  • Not utilizing or using the OpeningHours, which means, you're throwing away your efforts of implementing OpeningHours including WeekCollection<T>!

Moreover

because of your enormous constructor, it would be a good practice to consider using object modeling technique to minimize the arguments, and have a better control on it. So, you can modify your OpeningHour to something like this :

public class WorkDay
{
    public DayOfWeek Day { get; set; }

    public TimeSpan OpenHour { get; set; }

    public TimeSpan CloseHour { get; set; }
}

public class Restaurant
{
    public List<WorkDay> WorkDays { get; }

    public Restaurant(List<WorkDay> workDays)
    {
        WorkDays = workDays;
    }
}

Now, it's much simplified and easier to handle, as we avoided extra work, which disposed the need of WeekCollection<T>. The rest can be handled from the constructor (like validating the object, order of elements ..etc.).

For the DateParser

Almost all collections types have a constructor that takes IEnumerable<T>, even if the collection doesn't have that constructor, it would have a method that takes a collection to be added like AddRange for instance. So, this :

HashSet<string> availableRanges = new HashSet<string>();
List<string> timeRanges = new List<string>();
for (int i = 0 ; i < days.Count; i++){
   string timeRange = openHours[i].ToString().Substring(1,1)+'-' + closeHours[i].ToString().Substring(0,2);
   availableRanges.Add(timeRange);
   timeRanges.Add(timeRange);
}

is not necessary, as you can do this :

var timeRanges = new List<string>();

for (int i = 0 ; i < days.Count; i++){  
   timeRanges.Add($"{{openHours[i]:%h}-{closeHours[i]:%h}");
}

var availableRanges = new HashSet<string>(timeRanges);

though, when converting TimeSpan or DateTime you should always use IFormatProvider instead of Substring to specifiy the format you need (like {openHours[i]:%h} I specified the hour part only. The other bad practice is that you already have TimeSpan and you then converted to string, then used string comparison. This is bad, because you're changing the object type which loses its mutability, and preferences. Plus, it's another extra work. Why not just work the current object as is, and take advantage of its its benefits?.

I think the reason behind the conversion to string is that you needed to put both TimeSpan to be compared, as the current solution is dealing with them separately. So, converting it to string would solve this issue, which is a ghetto solution.

A proper solution is to use either Object Modeling (like WorkDay example above) or Tuple or Dictionary or a KeyPairValue or even creating anonymous type. Which would chain both objects. This way, you will still keep the object to its original state, and can be managed properly.

Also, string is immutable so, when concatenating string, it would recreate a new string, and not appending to the current one. This would be really bad to the memory with large strings. Instead, use StringBuilder, as*@Heslacher mentioned in his answer.

The loop itself can be simplified though, however, you've missed the order of days! because you're not using WeekCollection<OpeningHour> which you've already ordered!

P.S. DayOfWeek it's enum it can be cast to int value, which is an int representation of that day (0 = Sunday, and 6 = Saturday). So, why not just use it instead ?.

Example :

public class WorkDay
{
    public DayOfWeek Day { get; set; }

    public TimeSpan OpenHour { get; set; }

    public TimeSpan CloseHour { get; set; }

    public WorkDay(DayOfWeek day , TimeSpan openTime , TimeSpan closeTime)
    {
        Day = day;
        OpenHour = openTime;
        CloseHour = closeTime;
    }

    public WorkDay(DayOfWeek day , int openHour , int closeHour)
    {
        if(openHour > 24 || openHour < 0) { throw new ArgumentOutOfRangeException(nameof(openHour)); }

        if(closeHour > 24 || closeHour < 0) { throw new ArgumentOutOfRangeException(nameof(closeHour)); }

        OpenHour = new TimeSpan(openHour , 0 , 0);
        
        CloseHour = new TimeSpan(closeHour , 0 , 0);

        Day = day;
    }
    
    public string GetOpenCloseTimeAsString()
    {
        return $"{OpenHour:%h}-{CloseHour:%h}";
    }
    
    public override string ToString()
    {
        return $"{Day.ToString().Substring(0,3)}: {OpenHour:%h}-{CloseHour:%h}";
    }

}
    
    
public class Restaurant
{
    /// <summary>
    /// Enforced immutability, no changes on the list once the list is assigned. 
    /// </summary>
    public IReadOnlyList<WorkDay> WorkDays { get; }

    /// <summary>
    /// Enforced constructor initiailization, always initiate with a IEnumerable<WorkDay>
    /// </summary>
    /// <param name="openHours"></param>
    public Restaurant(IEnumerable<WorkDay> workDays)
    {
        if(workDays == null)
        { throw new ArgumentNullException(nameof(workDays)); }

        // only seven days are allowed. 
        var count = workDays.Count();
        
        if(count == 0 || count > 7) 
        { throw new ArgumentOutOfRangeException(nameof(workDays)); }
    
        // forget the user order, just reorder the list before assign.
        // to ensure it'll be always in the given order
        WorkDays = workDays
                        .OrderBy(x => x.OpenHour)
                        .ThenBy(x => x.CloseHour)
                        .ThenBy(x => (int) x.Day)
                        .ToList();
    }


    /// <summary>
    /// If you need to return a HashSet or the list.
    /// </summary>
    /// <returns></returns>
    public HashSet<string> GetUniqueTimes()
    {
        return new HashSet<string>(WorkDays.Select(x => x.GetOpenCloseTimeAsString()));
    }

    public string GetWorkdaysAsString()
    {
        // The expected results can be acheived using Linq 

        var result = WorkDays.GroupBy(x => new { x.OpenHour , x.CloseHour }
            , (key , values) => new
            {
                Time = key ,
                Days = values.Select(x => x.Day)
                                 .OrderBy(x => (int) x) // re-enforces the order by week days
                                 .Select(d => d.ToString().Substring(0 , 3))
                                 .ToList()
            })
            .Select(x =>
            {
                if (x.Days.Count == 1)
                {
                    return new
                    {
                        StartDay = x.Days.First() ,
                        Schedule = $"{x.Days.First()} : {x.Time.OpenHour:%h}-{x.Time.CloseHour:%h}"
                    };
                }

                return new
                {
                    StartDay = x.Days.First() ,
                    Schedule = $"{x.Days.Skip(1).DefaultIfEmpty().First()} - {x.Days.Skip(1).DefaultIfEmpty().Last()}: {x.Time.OpenHour:%h}-{x.Time.CloseHour:%h}"
                };

            })
            .ToList();

        var open = result.First();
        var close = result.Last();

        return result.Count > 2 ? $"{open.StartDay}, {string.Join(", " , result.Select(x => x.Schedule))}"  : $"{open.StartDay}, {open.Schedule}, {close.Schedule}";
    }
}

Usage :

 // since the constructor accepts IEnumerable<WorkDay>
 // I can pass List, Array, or any other collection that implements `IEnumerable`
var resturant = new Restaurant(new List<WorkDay>
{
    new WorkDay(DayOfWeek.Sunday, 8, 16),
    new WorkDay(DayOfWeek.Monday, 8, 17),
    new WorkDay(DayOfWeek.Tuesday, 8, 17),
    new WorkDay(DayOfWeek.Wednesday, 8, 17),
    new WorkDay(DayOfWeek.Thursday, 8, 16),
    new WorkDay(DayOfWeek.Friday, 8, 16),
    new WorkDay(DayOfWeek.Saturday, 8, 16)
});


var results = resturant.GetWorkdaysAsString();

// in case you need to return the hashset of the times
var unique = resturant.GetUniqueTimes();

Answer 2

The biggest improvement regarding performance would be to use a StringBuilder instead of concatinating strings by the meaning of result += ... because the later will create a new string each time.

Hi I am new to this StringBuilder, could you show me the right way to do it?

As an example based on your code:

StringBuilder stringBuilder = new StringBuilder();
for (int i = 0; i < arToList.Count; i++)
{
    for (int j = 0; j < timeRanges.Count; j++)
    {
        if (timeRanges[j] == arToList[i])
        {
            // First Item
            if (j == 0)
            {
                stringBuilder.Append(days[j].ToString().Substring(0, 3));
            }
            // Last Item
            else if (j == timeRanges.Count - 1)
            {
                char last = result.Last();
                if (last != ' ')
                {
                    stringBuilder.Append(" - ");
                }
                stringBuilder.Append(days[j].ToString().Substring(0, 3));
            }

When you need the content of the StringBuilder you just use the ToString() method.

---

• To show correct results you will need to force the restaurant to open before 10. Thats because for openinghours you take the second char. A better way would be to correctly use the TimeSpan objects by accessing the Hours property.

• You could use String interpolation instead of doing e.g string + " - " + string which would look for the first loop like so

  string timeRange = $"{openHours[i].Hours}-{closeHours[i].Hours}";
  

• The most inner loop doesn't need to iterate each time over all the timeRanges. Each seen item can be skipped. This can be done by using (int)current as the initial value for j.

• There is some discrepancy regarding the constructor of the Restaurant and the constructor of the WeekCollection. The first parameter of Restaurant's constructor is monday which is passed to the WeekCollection<>'s constructor as parameter sunday. If one sees only the constructor of Restaurant the expected results won't match.

• "//Everything in the middle": at least with the provided test-data you can safely remove this:

  if (days[j] - current > 1)
  {
      stringBuilder.Append(", ");
  }
  

  and change the else if to an else.

Summing up the DateParser() method, which by the way shouldn't be named like a noun but rather like a verb or verb-phrase would look like this:

public string DateParser(List<DayOfWeek> days, List<TimeSpan> openHours, List<TimeSpan> closeHours)
{
    HashSet<string> availableRanges = new HashSet<string>();
    List<string> timeRanges = new List<string>();
    DayOfWeek current = DayOfWeek.Sunday;

    for (int i = 0; i < days.Count; i++)
    {
        string timeRange = $"{openHours[i].Hours}-{closeHours[i].Hours}";
        availableRanges.Add(timeRange);
        timeRanges.Add(timeRange);
    }
    StringBuilder stringBuilder = new StringBuilder();
    
    List<string> arToList = availableRanges.ToList();
    for (int i = 0; i < arToList.Count; i++)
    {
        for (int j = (int)current; j < timeRanges.Count; j++)
        {
            if (timeRanges[j] == arToList[i])
            {
                // First Item
                if (j == 0)
                {
                    stringBuilder.Append(days[j].ToString().Substring(0, 3));
                }
                // Last Item
                else if (j == timeRanges.Count - 1)
                {
                    char last = stringBuilder[stringBuilder.Length - 1];
                    if (last != ' ')
                    {
                        stringBuilder.Append(" - ");
                    }
                    stringBuilder.Append(days[j].ToString().Substring(0, 3));
                }
                // Everything in the middle
                else
                {
                    if (timeRanges[j] != timeRanges[j - 1])
                    {
                        stringBuilder.Append(days[j].ToString().Substring(0, 3));
                    }
                    else
                    {
                        char last = stringBuilder[stringBuilder.Length - 1];
                        if (last != ' ')
                        {
                            stringBuilder.Append(" - ");
                        }
                        if (timeRanges[j] != timeRanges[j + 1])
                        {
                            stringBuilder.Append(days[j].ToString().Substring(0, 3));
                        }
                    }
                }
                current = days[j];
            }
        }

        stringBuilder.Append(": " + arToList[i]);
        if (i != arToList.Count - 1)
        {
            stringBuilder.Append(", ");
        }
    }
    return stringBuilder.ToString();
}

Answer 3

There is a bug in your code

Your example and your code do not correspond. Your example starts with Sunday, yet the first parameter of your constructor is Monday, which then uses WeekCollection which starts with... Sunday!

A suggestion

IMHO OpeningHour should contain a parameter to indicate the day of the week, so you can pass a collection of OpeningHour to the constructor which should then check for duplicates. That way you can also have days where the restaurant is closed.

Answer 4

When you initialize the dictionary in WeekCollection<T> you could/should do it like this:

    _collection = new Dictionary<DayOfWeek, T>
    {
      { DayOfWeek.Sunday, sunday },
      { DayOfWeek.Monday, monday },
      { DayOfWeek.Tuesday, tuesday },
      { DayOfWeek.Wednesday, wednesday },
      { DayOfWeek.Thursday, thursday },
      { DayOfWeek.Friday, friday },
      { DayOfWeek.Saturday, saturday }
    };

Personally I think that WeekCollection<T> is superfluous. It doesn't really help to solve the problem, nor does it make the code more clear. So I would use its incapsulated dictionary directly in Restaurant:

public class Restaurant
{
  public Dictionary<DayOfWeek, OpeningHour> OpeningHours { get; }

  public Restaurant(OpeningHour sunday, OpeningHour monday, OpeningHour tuesday, OpeningHour wednesday, OpeningHour thursday, OpeningHour friday, OpeningHour saturday)
  {
    OpeningHours = new Dictionary<DayOfWeek, OpeningHour>
    {
      { DayOfWeek.Sunday, sunday },
      { DayOfWeek.Monday, monday },
      { DayOfWeek.Tuesday, tuesday },
      { DayOfWeek.Wednesday, wednesday },
      { DayOfWeek.Thursday, thursday },
      { DayOfWeek.Friday, friday },
      { DayOfWeek.Saturday, saturday }
    };
  }

  ...
}

---

A little about naming:

public class OpeningHour
{
  public TimeSpan OpeningTime { get; private set; }
  public TimeSpan ClosingTime { get; private set; }

Descriptive names is a must and good, but IMO they should be as short as possible:

public class OpeningHour
{
  public TimeSpan Open { get; }
  public TimeSpan Close { get; }

It's obvious from the context, that OpeningTime is a time object, so Open is precise and descriptive enough.

---

But the major problem with the code in DateParser() is that you do many of the "computation" on the output strings. You compose the output and select from the days/opening hours via the built strings. This is error prone and makes the code unnecessary complex and hard to read.

Instead you should group and select by the numeric values of the OpeningHour and DayOfWeek objects and from that result format the resulting string output.

Below is a complete take on that - with some inline comments, that you maybe can find inspiration in:

public class Restaurant
{
  // The WeekCollection is replaced with a plain dictionary
  private readonly Dictionary<DayOfWeek, OpeningHour> openingHours;
  public IReadOnlyDictionary<DayOfWeek, OpeningHour> OpeningHours => openingHours;

  public Restaurant(OpeningHour sunday, OpeningHour monday, OpeningHour tuesday, OpeningHour wednesday, OpeningHour thursday, OpeningHour friday, OpeningHour saturday)
  {
    openingHours = new Dictionary<DayOfWeek, OpeningHour>
    {
      { DayOfWeek.Sunday, sunday },
      { DayOfWeek.Monday, monday },
      { DayOfWeek.Tuesday, tuesday },
      { DayOfWeek.Wednesday, wednesday },
      { DayOfWeek.Thursday, thursday },
      { DayOfWeek.Friday, friday },
      { DayOfWeek.Saturday, saturday }
    };
  }

  public string GetOpeningHours()
  {
    var intervals = GroupByOpeningIntervals();

    StringBuilder builder = new StringBuilder();

    foreach (((var open, var close), var days) in intervals)
    {
      FormatInterval(builder, open, close, days);
    }

    builder.Length -= 2; // The last comma and space (", ")

    return builder.ToString();
  }

  // Format the result string according to your requirements
  private void FormatInterval(StringBuilder builder, TimeSpan open, TimeSpan close, List<DayOfWeek> days)
  {
    days.Sort();

    DayOfWeek firstDay = days[0];
    DayOfWeek lastDay = days[0];

    for (int i = 1; i < days.Count; i++)
    {
      DayOfWeek currDay = days[i];
      // If the current day isn't the next day from lastDay, then the current day interval ends, and a new begins with currDay as firstDay
      if (currDay != lastDay + 1)
      {
        AppendDayInterval(builder, firstDay, lastDay);
        firstDay = lastDay = currDay;
      }
      else
      {
        lastDay = currDay;
      }
    }

    AppendDayInterval(builder, firstDay, lastDay);
    builder.Length -= 2; // The last comma and space (", ")

    builder.AppendFormat(@": {0:hh\:mm}-{1:hh\:mm}, ", open, close);
  }

  // A helper function that format a DayOfWeekInterval
  private void AppendDayInterval(StringBuilder builder, DayOfWeek first, DayOfWeek last)
  {
    if (first == last)
      builder.AppendFormat("{0}, ", first.ToString().Substring(0, 3));
    else
      builder.AppendFormat("{0} - {1}, ", first.ToString().Substring(0, 3), last.ToString().Substring(0, 3));
  }

  // Grouping the OpeningHours into a dictionary having the open/close interval as key and a list of DayOfWeek values as value
  private Dictionary<(TimeSpan open, TimeSpan close), List<DayOfWeek>> GroupByOpeningIntervals()
  {
    Dictionary<(TimeSpan open, TimeSpan close), List<DayOfWeek>> intervals = new Dictionary<(TimeSpan open, TimeSpan close), List<DayOfWeek>>();

    // Each entry in OpeningHours is a KeyValuePair<K, V> which implements a Deconstruct(...) method and can therefore be decomposed into a tuple:
    foreach ((var day, var openingHour) in openingHours)
    {
      if (!intervals.TryGetValue(openingHour.Interval, out var set))
        intervals[openingHour.Interval] = set = new List<DayOfWeek>();
      set.Add(day);
    }

    return intervals;
  }
}

public class OpeningHour
{
  public TimeSpan Open { get; }
  public TimeSpan Close { get; }

  public OpeningHour(TimeSpan open, TimeSpan close)
  {
    Open = open;
    Close = close;
  }

  public OpeningHour(int open, int close) :
    this(TimeSpan.FromHours(open), TimeSpan.FromHours(close))
  {
  }

  // Just a convenient property.
  public (TimeSpan open, TimeSpan close) Interval => (Open, Close);

  public override string ToString()
  {
    return $"{Open.TotalHours} - {Close.TotalHours}";
  }
}

As seen, there is very few comparison statements and all comparison is done on numeric values instead of on strings.