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

Extracting opening and closing times for each day of the week

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

Problem

I have these two methods:

private static OpeningDay GetUsallyOpeningHour(List openingHoursDaily, DayOfWeek dayOfWeek)
{
    var listOfSpecificDayOfWeek = openingHoursDaily.Where(x => x.DateTime.DayOfWeek == dayOfWeek);

    if(listOfSpecificDayOfWeek.Any())
        return listOfSpecificDayOfWeek.GroupBy(x => x.From).OrderByDescending(x => x.Count()).ThenByDescending(x => x.Key).First().First();

    return new OpeningDay { From = new TimeSpan(0, 0, 0), To = new TimeSpan(0, 0, 0), DateTime = DateTime.Now };
}

private static OpeningDay GetUsallyClosingHour(List openingHoursDaily, DayOfWeek dayOfWeek)
{
    var listOfSpecificDayOfWeek = openingHoursDaily.Where(x => x.DateTime.DayOfWeek == dayOfWeek);

    if (listOfSpecificDayOfWeek.Any())
        return listOfSpecificDayOfWeek.GroupBy(x => x.To).OrderByDescending(x => x.Count()).ThenByDescending(x => x.Key).First().First();

    return new OpeningDay { From = new TimeSpan(0, 0, 0), To = new TimeSpan(0, 0, 0), DateTime = DateTime.Now };
}


The only difference between them is that one is grouping by x.From and other by x.To, and I was wondering how I can improve this code.

Basically, I can a create method which takes lambda, but how long? At first I was thinking to pass this:

GroupBy(x => x.To).OrderByDescending(x => x.Count()).ThenByDescending(x => x.Key)


as a lambda parameter. But this would take me ending with function that takes a lambda and then returns sth like lambda.First().First().

What do you recommend? Are there some other improvements?

Solution

Your thinking is good: it's a small difference but... there is a difference. And you have to find a way to extract the difference out of the methods and try create a parameter from this. Small example:

public class User
{
    public int Id { get; set; }
    public string Name { get; set; }
    public int Value { get; set; }
}


We could then group them by Id or Value for example:

public Item GetItemGroupedByName(List items)
{
    return items.GroupBy(x => x.Id).First().First();
}

public Item GetItemGroupedByValue(List items)
{
    return items.GroupBy(x => x.Value).First().First();
}


This gives the same kind of situation like yours. Luckily there's the Func Delegate. The method can now look like this:

public Item GetItemGrouped(List items, Func rule)
{
    return items.GroupBy(rule).First().First();
}


And you call it like this:

GetItemGrouped(list, x => x.Value);
//or:
GetItemGrouped(list, x => x.Id);


Of course you can't group by Name in this case as it is a string property and the Func expects an integer. You might change the int to object, but please don't as that is hacky code. Since the purpose of your method is to group by a TimeSpan, you can implement this easily:

private static OpeningDay GetUsallyOpeningHour(List openingHoursDaily,
                                               DayOfWeek dayOfWeek,
                                               Func groupByRule)
{
    var listOfSpecificDayOfWeek = openingHoursDaily.Where(x => x.DateTime.DayOfWeek == dayOfWeek);

    if(listOfSpecificDayOfWeek.Any())
        return listOfSpecificDayOfWeek.GroupBy(groupByRule)
                                      .OrderByDescending(x => x.Count())
                                      .ThenByDescending(x => x.Key)
                                      .First().First();

    return new OpeningDay { From = new TimeSpan(0, 0, 0), To = new TimeSpan(0, 0, 0), DateTime = DateTime.Now };
}


And now you can call the method like:

var openingDay = GetUsallyOpeningHour(openingHours, DayOfWeek.Thursday, x => x.From);
//or:
var openingDay = GetUsallyOpeningHour(openingHours, DayOfWeek.Thursday, x => x.To);

Code Snippets

public class User
{
    public int Id { get; set; }
    public string Name { get; set; }
    public int Value { get; set; }
}
public Item GetItemGroupedByName(List<Item> items)
{
    return items.GroupBy(x => x.Id).First().First();
}

public Item GetItemGroupedByValue(List<Item> items)
{
    return items.GroupBy(x => x.Value).First().First();
}
public Item GetItemGrouped(List<Item> items, Func<Item, int> rule)
{
    return items.GroupBy(rule).First().First();
}
GetItemGrouped(list, x => x.Value);
//or:
GetItemGrouped(list, x => x.Id);
private static OpeningDay GetUsallyOpeningHour(List<OpeningDay> openingHoursDaily,
                                               DayOfWeek dayOfWeek,
                                               Func<OpeningDay, TimeSpan> groupByRule)
{
    var listOfSpecificDayOfWeek = openingHoursDaily.Where(x => x.DateTime.DayOfWeek == dayOfWeek);

    if(listOfSpecificDayOfWeek.Any())
        return listOfSpecificDayOfWeek.GroupBy(groupByRule)
                                      .OrderByDescending(x => x.Count())
                                      .ThenByDescending(x => x.Key)
                                      .First().First();

    return new OpeningDay { From = new TimeSpan(0, 0, 0), To = new TimeSpan(0, 0, 0), DateTime = DateTime.Now };
}

Context

StackExchange Code Review Q#91754, answer score: 2

Revisions (0)

No revisions yet.