4
\$\begingroup\$

I want to get all days of the current week (from Monday to Sunday) with a custom time. To do this, I have written an extension method for TimeSpan:

public static DateTime[] Daily(this TimeSpan ts)
{
    var days = DayOfWeek.Monday - DateTime.UtcNow.DayOfWeek;
    var startDate = DateTime.UtcNow.AddDays(days);
    List<DateTime> dates = new List<DateTime>();

    for(var i = 0; i < 7; i++)
    {
        dates.Add(startDate.Date + ts);
        startDate = startDate.AddDays(1);
    }

    return dates.ToArray();
}

As an example of how it would be used, new TimeSpan(17, 0, 0).Daily() will return:

  • 23/01/2016 17:00:00
  • 24/01/2016 17:00:00
  • 25/01/2016 17:00:00
  • 26/01/2016 17:00:00
  • 27/01/2016 17:00:00
  • 28/01/2016 17:00:00
  • 29/01/2016 17:00:00

Any suggestions on ways to improve my code?

\$\endgroup\$
  • 3
    \$\begingroup\$ I don't know if bug or not because don't know your use case. But if the date is sunday then this will give you week to come and not the week that happened. So if I ran it on Saturday I would be the week the just happened but if I run it on Sunday I will get next week. This seems counter intuitive. I would assume if start of week is Monday then Saturday and Sunday would return the same results. \$\endgroup\$ – CharlesNRice Jan 25 '17 at 16:17
  • \$\begingroup\$ @CharlesNRice you're right this is a bug, if I run it on Sunday I still want to see the current week. \$\endgroup\$ – Almis Jan 25 '17 at 16:26
  • \$\begingroup\$ @Paparazzi I could see it either way. His function acts like Monday is the start of the week. So it seems like it should work as if Monday is day 1 or Zero in C#. It's why I posted as a comment and not an answer. \$\endgroup\$ – CharlesNRice Jan 25 '17 at 16:48
  • \$\begingroup\$ @CharlesNRice OK if you mean bug in the posted solution then I agree. Deleting my comment. \$\endgroup\$ – paparazzo Jan 25 '17 at 16:50
8
\$\begingroup\$

Let's talk about two major things in this: building a modular API, and Unit Testing.

This can be easily unit tested, you should have 7 tests, one for a date in each day of the week. Let's pick a Week (past, present or future, doesn't matter), for this I'm going to pick an easy week. Since DayOfWeek.Monday is the first day of your week, we'll pick the first week of a month that starts on a Monday.

Scrolling backwards in my calendar I found August, 2016. August 1st, 2016 is a Monday, so we'll test that week.

The problem right now with your code is that it cannot be tested. You have to reset the system clock to test it. This is not a hard problem to fix. We'll redefine the API a little bit (add a default parameter, basically):

public static DateTime[] Daily(this TimeSpan ts, DateTime? checkDay = null)

So that's easy, now we can specify a specific day to check. All we have to do in the code is use that variable.

var weekDate = checkDay ?? DateTime.UtcNow;

Bam. That should be the first line of the method. That will make sure we get weekDate set to a valid DateTime.

Alright, good. So we redesigned the API a little and made it support an extra feature with no effort. Now we need to talk about that return.

Generally speaking: robust, modular API's that return a collection should return an IEnumerable<T> of some sort. I may have the user specify a specific DayOfWeek and try to find the first date in a give week that is that DayOfWeek, so I don't need the entire list (potentially). This is because each caller has different requirements (potentially), and to most closely follow SRP we shouldn't care about the resulting collection.

Our API doesn't need to worry about the collection as a whole, only generating each element in it. This may be a bizarre concept to understand at first, but our method should not care what the caller is going to do with the collection itself. We just generate it, so we don't need a list or array, we just need to return each element as it's generated.

We do this by returning an IEnumerable<DateTime> so that we can use the yield return statement instead of return. This will allow us to make a more modular API that doesn't always have to enumerate everything. It's the caller's responsibility to call .ToArray() if they need an array, or .ToList(), or enumerate it, ect.

public static IEnumerable<DateTime> Daily(this TimeSpan ts, DateTime? checkDay = null)

Lastly, different users' may have their week start on a different day. We want to accommodate for that, so we'll create a new default parameter for that.

public static IEnumerable<DateTime> Daily(this TimeSpan ts, DayOfWeek startDayOfWeek = DayOfWeek.Monday, DateTime? checkDay = null)

Bam. Now we've built a much more robust API that can be tested more easily as well. It should be trivial to implement this, in fact, we only need to change a few parts of your original code.

public static IEnumerable<DateTime> Daily(this TimeSpan ts, DayOfWeek startDayOfWeek = DayOfWeek.Monday, DateTime? checkDay = null)
{
    var compDate = checkDay ?? DateTime.UtcNow;
    var days = startDayOfWeek - compDate.DayOfWeek;
    var startDate = compDate.AddDays(days);

    for (var i = 0; i < 7; i++)
    {
        yield return startDate.AddDays(i).Date + ts;
    }
}

It's even shorter now, supports your original use-case, and add's many more supported use-cases.

Then we build Unit-tests, they'll all have mostly the same body:

var expected = new DateTime[]
{
    new DateTime(2016, 8, 1, 15, 0, 0),
    new DateTime(2016, 8, 2, 15, 0, 0),
    new DateTime(2016, 8, 3, 15, 0, 0),
    new DateTime(2016, 8, 4, 15, 0, 0),
    new DateTime(2016, 8, 5, 15, 0, 0),
    new DateTime(2016, 8, 6, 15, 0, 0),
    new DateTime(2016, 8, 7, 15, 0, 0)
};
var inputTimeSpan = new TimeSpan(15, 0, 0);
var inputDate = new DateTime(2016, 8, 1);

var actual = Daily(inputTimeSpan, checkDay: inputDate).ToArray();

CollectionAssert.AreEquivalent(expected, actual);

Just change inputDate. Once we do this we'll see that on 2016-08-01 we get a failed result. No big deal, we just need to make sure we fix our method. (Easy fix too.)

Our bug is the offset, on Sunday you'll get Monday-Sunday for the upcoming week. (So on 7 Aug 2016 you get 8 Aug 2016 - 14 Aug 2016.) That's fine, to fix it we'll just check if days > 0 and if so we'll subtract from it.

days = days > 0 ? days - 7 : days;

We'll just add that after we assign days the first time, and bam. We're done.


Final Code

public static IEnumerable<DateTime> Daily(this TimeSpan ts, DayOfWeek startDayOfWeek = DayOfWeek.Monday, DateTime? checkDay = null)
{
    var compDate = checkDay ?? DateTime.UtcNow;
    var days = startDayOfWeek - compDate.DayOfWeek;
    days = days > 0 ? days - 7 : days;
    var startDate = compDate.AddDays(days);

    for (var i = 0; i < 7; i++)
    {
        yield return startDate.AddDays(i).Date + ts;
    }
}

As you can see, it's still nice and short, and it's more robust than the original. :) This was an excellent start, and hopefully you take more out of this than just a bug fix. :)

\$\endgroup\$
  • \$\begingroup\$ Very good step-by-step answer, I understood everything without any trouble. I definitely need to improve my test-driven development :) \$\endgroup\$ – Almis Jan 26 '17 at 16:54
  • \$\begingroup\$ Since this function will return at most 7 value-type values, is there any value in returning IEnumerable<T> over T[]? \$\endgroup\$ – Zev Spitz Jan 29 '17 at 6:46
7
\$\begingroup\$

You already know that you cannot test this extension because Now is hardcoded so tests will yield different results each time. To overcome this problem you can make it a DateTime extension instead an all problems with testing will disappear because the only calculation that you need is to add the number of days and the time to the date.

Then you take the advantage of the Take method for counting how many dates you'll get. With this you only need a single test that checks if the two additions work correctly.

public static IEnumerable<DateTime> DailyAt(this DateTime date, TimeSpan time)
{
    for (int offset = 0;; offset++)
    {
        yield return date.Date.AddDays(offset).Add(time);
    }
}

You can use and/or test it with any date and any time and for as many days as you want:

var dates = DateTime.UtcNow.DailyAt(new TimeSpan(17, 20,0)).Take(3);

Do you want it to start tomorrow? Then just Skip today:

var dates = DateTime.UtcNow.DailyAt(new TimeSpan(17, 20,0)).Skip(1).Take(3);

Ok, we can generate the dates and time but what's with the monday? You write

I want to get all days of the current week (from Monday to Sunday) with a custom time.

this means we need another extension that gets the monday:

public static DateTime LastMonday(this DateTime value)
{
    return value.AddDays(DayOfWeek.Monday - value.DayOfWeek);
}

and now you can chain them:

var dates = 
    DateTime.UtcNow
    .LastMonday()
    .DailyAt(new TimeSpan(17, 20,0))
    .Take(3);

but you want an entire week? Ok, let's create another extension:

public static IEnumerable<DateTime> TakeWeek(this IEnumerable<DateTime> values)
{
    return values.Take(7);
}

and chain this one too:

var dates = 
    DateTime.UtcNow
    .LastMonday()
    .DailyAt(new TimeSpan(17, 20,0))
    .TakeWeek();
\$\endgroup\$
  • \$\begingroup\$ I struggled a bit to understand your code but it's really good and I definitely gonna use this code style. You have the bug with the Sunday where last Monday will return the next Monday instead but overall it was very good, it will change the way I code from now on. I accepted the other answer because it improved my code without changing it completely, it was explained very well and because it didn't have the bug but I still like your elegant code more :) \$\endgroup\$ – Almis Jan 26 '17 at 16:51
  • \$\begingroup\$ @Almis This is not a bug ;-) let's say Monday = 3 and DayOfWeek = 3 (Wednesday) then 1 - 3 = -2 so AddDays(-2); (I've tested it and it returns the correct value) \$\endgroup\$ – t3chb0t Jan 26 '17 at 16:59
  • \$\begingroup\$ Crap, I mean Monday = 1 of course \$\endgroup\$ – t3chb0t Jan 26 '17 at 17:09
  • \$\begingroup\$ yes when DayOfWeek = 0 (Sunday) then it's 1-0 = 1 so AddDays(+1) which is the next Monday \$\endgroup\$ – Almis Jan 26 '17 at 18:29
2
\$\begingroup\$

I don't know that this is much better, but you can use an array directly (instead of a List<T>). This gets rid of one line and may slightly improve performance:

public static DateTime[] Daily(this TimeSpan ts)
{
    var days = DayOfWeek.Monday - DateTime.UtcNow.DayOfWeek;
    var startDate = DateTime.UtcNow.AddDays(days);
    DateTime[] dates = new DateTime[7];

    for(var i = 0; i < 7; i++)
    {
        dates[i] = startDate.DateAddDays(i).date + ts;
    }

    return dates;
}

Quick fix for week starts on Sunday (not tested)

public static DateTime[] Daily(this TimeSpan ts)
{
    var days = DayOfWeek.Sunday - DateTime.UtcNow.DayOfWeek;
    var startDate = DateTime.UtcNow.AddDays(days);
    DateTime[] dates = new DateTime[7];

    for(var i = 0; i < 7; i++)
    {
        dates[i] = startDate.DateAddDays(i+1).date + ts;
    }

    return dates;
}
\$\endgroup\$
  • \$\begingroup\$ Yes I agree with you it's better to use directly an array than list, this answer is almost complete (excluding the bug that @CharlesNRice found). You also have syntax error it should be startDate.AddDays(i).Date \$\endgroup\$ – Almis Jan 25 '17 at 16:35
  • \$\begingroup\$ I agree with CharlesNRice that it is a bug. I would just start on Sunday. \$\endgroup\$ – paparazzo Jan 25 '17 at 16:51
0
\$\begingroup\$

In addition to (most of) the points in EBrown's excellent answer, I would suggest using Enumerable.Range as a replacement for the for loop:

public static DateTime[] Daily(this TimeSpan ts, DayOfWeek startDayOfWeek = DayOfWeek.Monday, DateTime? checkDay = null)
{
    var compDate = checkDay ?? DateTime.UtcNow;
    var days = startDayOfWeek - compDate.DayOfWeek;
    days = days > 0 ? days - 7 : days;
    var startDate = compDate.AddDays(days);

    return Enumerable.Range(0,7).Select(i => startDate.AddDays(i) + ts).ToArray();
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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