9
\$\begingroup\$

I'm trying to come up with a more elegant solution for a loop in months that I wrote long time ago in C# 1.1

I use a helper class Month but I'd prefer to get rid of it and make the code shorter and more readable.

I receive a start period in format yyyy-mm and end period yyyy-mm and I need to execute something for every month between those 2 periods.

public void LoopMonths(string startM, string endM)
{
        //startM = "2015-02";
        //endM = "2018-11";

        int startYear = int.Parse(startM.Split('-')[0]);
        int startMonth = int.Parse(startM.Split('-')[1]);
        int endYear = int.Parse(endM.Split('-')[0]);
        int endMonth = int.Parse(endM.Split('-')[1]);

        Month month = new Month(startYear, startMonth);
        Month monthEnd = new Month(endYear, endMonth);

        while (month <= monthEnd)
        {
            // do something ... 
            month = Month.NextMonth(month);
        }      
}


public class Month
{
    public DateTime StartDate { get; set; }
    public DateTime EndDate { get; set; }

    public Month(int year, int month)
    {
        StartDate = new DateTime(year, month, 1);
        EndDate = new DateTime(year, month, DateTime.DaysInMonth(year, month));
    }

    public static Month NextMonth(Month month)
    {
        DateTime next = month.StartDate.AddMonths(1);
        return new Month(next.Year, next.Month);
    }

    public static bool operator <=(Month month1, Month month2)
    {
        return month1.StartDate <= month2.StartDate;
    }

    public static bool operator >=(Month month1, Month month2)
    {
        return month1.StartDate >= month2.StartDate;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Don't like the name month for what is year and month. \$\endgroup\$ – paparazzo Jun 20 '18 at 15:11
  • 1
    \$\begingroup\$ @paparazzo: Fair point, but that's subjective. For example "day" is ambiguous between a "day of week" enum (Mon, Tue, Wed, ...), or part of a date value (which belongs to a specific month of a specific year). The same applies here. You're thinking of Month as an enum (Jan, Feb, ...), but OP is thinking of month as part of a date value. Either can be correct. I'd expect your interpretation to be an enum, not a class. \$\endgroup\$ – Flater Jun 22 '18 at 9:59
  • 1
    \$\begingroup\$ @paparazzo: No it doesn't. One Month object refers to one month (of a particular year). In order to define a range (since you claim "Month spans years"), Month would have to have two year values (begin and end of the range), which it simply doesn't. It only has one month value, one year value. I think you're getting confused by the StartDate/EndDate properties: the properties could technically span years; but the existing logic simply doesn't do that (it sets both properties to the same month, same year, but a different day). \$\endgroup\$ – Flater Jun 22 '18 at 11:00
  • 1
    \$\begingroup\$ @paparazzo: I give up. Read the code. The functionality you claim exists simply doesn't emerge in the code. At no point can a single Month object contain more than the defined month. At no point is a Month object ever defined to refer to more than one month. If you want to claim that Month can span years, I'm interested in hearing your proof of that. Substantiate your claims. \$\endgroup\$ – Flater Jun 22 '18 at 11:06
  • 1
    \$\begingroup\$ @paparazzo: You still haven't actually understood my initial comment. It's pointless to respond to something you don't read. \$\endgroup\$ – Flater Jun 22 '18 at 11:32
8
\$\begingroup\$

Improving on an already provided answer.

Since the date format is known, the date can be parsed using that format

This reduces having to split the provided strings and then parse the parts to construct the DateTime.

Everything else from @paparazzo's answer applies about naming and repeated code.

public static IEnumerable<DateTime> LoopYearMonths(string startYearMonth, string endYearMonth) {
    string format = "yyyy-MM";
    DateTime startDT = DateTime.ParseExact(startYearMonth, format, CultureInfo.InvariantCulture);
    DateTime endDT = DateTime.ParseExact(endYearMonth, format, CultureInfo.InvariantCulture);
    while (startDT <= endDT) {
        yield return startDT;
        startDT = startDT.AddMonths(1);
    }
}

Alternatively the function can also be refactored to include the desired action as a delegate

public static void LoopMonths(string startYearMonth, string endYearMonth, Action<DateTime> action) {
    string format = "yyyy-MM";
    DateTime startDT = DateTime.ParseExact(startYearMonth, format, CultureInfo.InvariantCulture);
    DateTime endDT = DateTime.ParseExact(endYearMonth, format, CultureInfo.InvariantCulture);
    while (startDT <= endDT) {
        action(startDT);
        startDT = startDT.AddMonths(1);
    }
}

and called

LoopMonths("2015-02", "2018-11", date => {
    //Do something with date
});
\$\endgroup\$
11
\$\begingroup\$

These are all good sugestions but to my taste not linq-ish enough and not really reusable. Since I don't like writing the same code serval times you might want to encapsulate some of this logic for later use.


You can extract two pieces. First you have a date-time range so create a Range struct. Here's a very simple one (as an example without any parameter validation - actually T should be IComparable so you can assure the range is valid).

readonly struct Range<T>
{
    public Range(T min, T max)
    {
        Min = min;
        Max = max;
    }

    public T Min { get; }

    public T Max { get; }
}

Then you are interpolating between min and max. Put this in an extension that will consume a Range and do the rest:

public static IEnumerable<T> Interpolate<T>(this Range<T> range, Func<T, T> next) where T : IComparable
{
    var current = range.Min;
    while (current.CompareTo(range.Max) <= 0)
    {
        yield return (current = next(current));
    }
}

Now all you need is to parse the date so how about one more helper for that too?

static class Range
{
    public static Range<DateTime> FromDateTime(string min, string max, string format)
    {
        return new Range<DateTime>(
            DateTime.ParseExact(min, format, CultureInfo.InvariantCulture),
            DateTime.ParseExact(max, format, CultureInfo.InvariantCulture)
        );
    }
}

And we're ready to go. Create a range and let the extension work.

var range = Range.FromDateTime("2015-02", "2018-11", "yyyy-MM");
range.Interpolate(d => d.AddMonths(1)).Dump();

Now you have nice components that you can use in many more places and maybe in several projects.

\$\endgroup\$
  • \$\begingroup\$ I had a similar thought. This screamed a DateTime Range. \$\endgroup\$ – Rick Davin Jun 20 '18 at 20:14
  • \$\begingroup\$ liking this structure a lot \$\endgroup\$ – Nkosi Jun 20 '18 at 21:04
5
\$\begingroup\$

Don't like the name month for what is year and month.

Recommend you return iEnumerable. This separates out what you want to do with YearMonth.

public iEnumerable<DateTime> LoopMonths(string startM, string endM)
{

Do the split once is a little more efficient and cleaner.

I don't see the need for a class month. NextMonth(Month month) is nothing but .AddMonths(1).

This trims it down.

public static IEnumerable<DateTime> LoopYearMonths(string startYearMonth, string endYearMonth)
{
    string[] start = startYearMonth.Split('-');
    string[] end = endYearMonth.Split('-');

    DateTime startDT = new DateTime(int.Parse(start[0]), int.Parse(start[1]), 1);
    DateTime endDT   = new DateTime(int.Parse(end[0]), int.Parse(end[1]), 1);

    while (startDT <= endDT)
    {
        yield return startDT;
        startDT = startDT.AddMonths(1);
    }
}
\$\endgroup\$
4
\$\begingroup\$

You have several good answers about the solution as a whole. I am going to focus on one small aspect: the EndDate.

public Month(int year, int month)
{
    StartDate = new DateTime(year, month, 1);
    EndDate = new DateTime(year, month, DateTime.DaysInMonth(year, month));
}

I work with a time-series database where we have a custom class for a time range, and frequently users want to work with time intervals, say get an hourly, daily, or monthly average.

My main point I want to bring up is that your EndDate is not really covering to the end of the month. Rather it ends at the start of the last day of the month. I think the end is actually:

EndDate = StartDate.AddMonths(1);

Though that is an implied exclusive end. If it made you feel better, you can subtract 1 tick from that. Where I work, we can't subtract just 1 second because we have customers that have sub-second data. But for all the fuss and fury over that 1 tick, just using the beginning of the next month as the end is sufficient. In many cases, someone wanting hourly averages is going to break it up as:

01:00 - 02:00
02:00 - 03:00
03:00 - 04:00
etc

Rather than

01:00 - 01:59.59.99999
02:00 - 02:59.59.99999
03:00 - 03:59.59.99999
etc

It shouldn't be as complicated or as ugly for days or months either. Or to put it more simply: June ends where July starts.

\$\endgroup\$
  • \$\begingroup\$ I have extensions for DateTime that do exactly that, this is adjust some parts of it like end-of-day or noon etc :-) \$\endgroup\$ – t3chb0t Jun 21 '18 at 4:11
2
\$\begingroup\$

Another option without any loops:

var startMonth = DateTime.Parse("yyyy-mm", m1)
var endMonth = DateTime.Parse("yyyy-mm", m2)

var dates = Enumerable
    .Range(0, int.MaxValue)
    .Select(i => startMonth.AddMonths(i))
    .TakeWhile(d => d <= endMonth);

dates.ToList().ForEach(...) //or foreach(var date in dates) {...}, that loop is more ok in my book

Edited due to comments: Made a mistake, need ToList() for ForEach to work.

Why would it be good to use Linq instead of having loops as in the original example (and all the other suggestions right now)? Some of the points below is true of many of the other answers as well.

  1. Separating "getting the months between month A and month B" from "making an action for each month" is basically single responsibility principle (easier to understand, easier to reuse and test each part separately and so on).
  2. It's lazy until ToList(). If using foreach, it's lazy all the time.
  3. Using built-in functionality is usually safer and faster than custom code.
  4. Less code.
  5. The exact condition for where to stop is very obvious at d <= endMonth. For exclusive end, just modify that. This is tightly connected with next point.
  6. No off by one errors, that otherwise are likely when handwriting loops.
  7. Comparing with other answers, we don't need any custom classes or any very specific "LoopMonths" functions that deals with low level details.

The main downside of my suggestion is that you need to understand linq. You can, of course, also wrap each part, say GetMonthsBetweenDates(d1,d2) for the linq part.

\$\endgroup\$
  • 2
    \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. \$\endgroup\$ – Dannnno Jun 21 '18 at 2:24
  • \$\begingroup\$ There is no ForEach method on IEnumerable<T> in the BCL. As Dannnno says, if you wish to suggest a method without any loops, then you should justify why you think this is a good idea. \$\endgroup\$ – VisualMelon Jun 21 '18 at 12:57
2
\$\begingroup\$

I personally would go with a more mathematical and less abstract approach:

public void LoopMonths(string startM, string endM)
    {
        //startM = "2015-02";
        //endM = "2018-11";

        int year = int.Parse(startM.Split('-')[0]);
        int month = int.Parse(startM.Split('-')[1]);
        int endYear = int.Parse(endM.Split('-')[0]);
        int endMonth = int.Parse(endM.Split('-')[1]);

        while (year < endYear || (year == endYear && month < endMonth))
        {
            // do something ... 
            month++;
            if (month == 13)
            {
                year++;
                month = 1;
            }
        }
    }

You still get one iteration step for every month between your start- and your end-month. Which for me seems to be the only requirement you have. Using the above solution you get rid of the additional class without changing your code a lot. Since you wanted to simplify your code my thought was to propose a solution that follows the KISS principle. Here you only have the most basic operations: one loop, one if, some math and the parsing you had before.

\$\endgroup\$
  • \$\begingroup\$ I actually understood this one. Not highest fruit on the tree, which I like. \$\endgroup\$ – JustJohn Jun 25 '18 at 19:20

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.