1
\$\begingroup\$

This is my below code which accepts the date in specific format and generates the N number of rolling date which consists the first and last date of each month in reverse manner.

import calendar
from datetime import datetime, date


def generate_rolling_date(input_date, number_of_dates):
    """
    Generate Rolling Date
    :param input_date: Input Date (e.g. '01-Feb-21')
    :param number_of_dates: Number of rolling date to generate
    :return:
    """
    # Convert input_date to datetime according to the format
    parsed_date = datetime.strptime(input_date, "%d-%b-%y")

    # Separate Day, Month and Year
    parsed_day = parsed_date.day
    parsed_month = parsed_date.month
    parsed_year = parsed_date.year

    # Initialize an empty output list
    output = []

    # using calendar module, gets the total number of days i.e. last day in parsed_month
    _, num_days = calendar.monthrange(parsed_year, parsed_month)

    # Check if the parsed_day is the last day of the month or not, according to that set the start_date flag
    start_date = False if num_days == parsed_day else True

    # Loop through the number_of_dates to generate the rolling dates
    for i in range(number_of_dates):
        # If the parsed_month value is 0, then decrease the parsed_year by 1 and set the parsed_month value to 12
        if parsed_month == 0:
            parsed_year -= 1
            parsed_month = 12
        # Get the total number of days i.e. last day in parsed_month
        _, num_days = calendar.monthrange(parsed_year, parsed_month)
        # If start_date, then get the first day of the month and set start_date to False,
        # so we can get the last day of month in the next loop
        if start_date:
            # Get the first day of the month
            selected_day = date(parsed_year, parsed_month, 1)
            start_date = False
        else:
            # Get the last day of the month
            selected_day = date(parsed_year, parsed_month, num_days)
            # After getting the last day of the month, decrease the parsed_month by 1 to get the previous month
            parsed_month -= 1
            start_date = True
        # Append selected_day to the output
        output.append(date.strftime(selected_day, "%d-%b-%y"))
    # Return the output
    return output

print(generate_rolling_date("01-Feb-21", 8))

Which gives this output:

['01-Feb-21',
 '28-Feb-21',
 '01-Jan-21',
 '31-Jan-21',
 '01-Dec-20',
 '31-Dec-20',
 '01-Nov-20',
 '30-Nov-20']

Now, my above code works as it should but I think it's a long and can be improved. Kindly guide me on how to improve this. Thank you.

\$\endgroup\$
3
  • \$\begingroup\$ Where does the %d-%b-%y format come from? Do you have control over this? \$\endgroup\$
    – Reinderien
    Jan 12, 2022 at 21:20
  • \$\begingroup\$ And do you really want your dates to be emitted in their current order? It would be less surprising to see last/first/last/first instead of first/last/first/last given that you're iterating in reverse chronological order. \$\endgroup\$
    – Reinderien
    Jan 12, 2022 at 21:25
  • \$\begingroup\$ Finally: how are these dates consumed? In my imagination they would be easier to consume as an iterable of first-last tuples rather than a flat iterable of dates. \$\endgroup\$
    – Reinderien
    Jan 12, 2022 at 21:28

3 Answers 3

2
\$\begingroup\$

Bits and pieces of the other answers are good: divide and conquer; separate formatting from iteration logic.

I think part of the problem with your code is that it's possibly not even doing the "right thing the wrong way", but instead doing the "wrong thing the wrong way". All realistic scenarios I can think of would benefit not from a flat iteration of strings, but a structured iteration of date object tuples. Given what you've shown it is impossible to say whether or not this is compatible with your use case; but I'm going to go ahead and demonstrate it anyway.

The simplest algorithm to accomplish what you're (vaguely) looking for does not rely on calendar at all, and doesn't even limit the number of iterations! This can be done on the calling side via islice. Benefits to this approach include simplicity of implementation, and flexibility: what if the caller doesn't know ahead of time how many months they want?

In my suggested implementation, I show a core generation function that is not length-limited, deals in objects instead of strings, and yields month bounding tuples instead of flat dates. I then show a wrapper function that can approximate your original function by flattening, limiting and formatting the iteration.

For such code it's also important that you unit test.

Suggested

from datetime import datetime, date, timedelta
from itertools import islice
from typing import Iterator

DATE_FORMAT = "%d-%b-%y"
ONE_DAY = timedelta(days=1)

DateRange = tuple[date, date]


def generate_rolling_dates(start_month: date) -> Iterator[DateRange]:
    """
    :param start_month: A day in the month after the iteration is to start. For example, if you
    pass 2022-02-07, pairs will start with (2022-01-01, 2022-01-31).
    :return: Iterator of month first-day last-day pairs, in antichronological order
    """

    start = start_month.replace(day=1)

    while True:
        end = start - ONE_DAY
        start = end.replace(day=1)
        yield start, end


def generate_flat_dates(start_month: str, n_days: int) -> Iterator[str]:
    months = generate_rolling_dates(
        datetime.strptime(start_month, DATE_FORMAT).date()
    )

    for pair in islice(months, n_days // 2):
        for day in pair:
            yield day.strftime(DATE_FORMAT)


def test() -> None:
    actual = tuple(islice(generate_rolling_dates(date(2022, 2, 7)), 3))
    expected = (
        (date(2022,  1, 1), date(2022,  1, 31)),
        (date(2021, 12, 1), date(2021, 12, 31)),
        (date(2021, 11, 1), date(2021, 11, 30)),
    )
    assert actual == expected

    actual = tuple(generate_flat_dates(start_month='07-Feb-22', n_days=6))
    expected = (
        '01-Jan-22',
        '31-Jan-22',
        '01-Dec-21',
        '31-Dec-21',
        '01-Nov-21',
        '30-Nov-21',
    )
    assert actual == expected


if __name__ == '__main__':
    test()
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Unless I've misunderstood the question, this answer doesn't handle odd n_days correctly: generate_flat_dates always returns an even number of dates. And I suspect that odd-even issue why I don't understand your suggested change to my answer. \$\endgroup\$
    – FMc
    Jan 13, 2022 at 1:32
  • \$\begingroup\$ @FMc - Thanks for reading, and you're right; but the difference is deliberate - I have yet to hear from OP, but I don't currently have any reason to believe that odd date counts need to be supported, and if they do, why. \$\endgroup\$
    – Reinderien
    Jan 13, 2022 at 1:35
  • \$\begingroup\$ Makes sense. I do agree that the desired output raises questions. \$\endgroup\$
    – FMc
    Jan 13, 2022 at 1:45
  • \$\begingroup\$ @Reinderien Your code is incorrect. It does not produce the same results as OP's code. A start date in 02-2021 should produce month ranges beginning at 01-02-2021. \$\endgroup\$ Jan 13, 2022 at 8:46
1
\$\begingroup\$

Your current code is well organized and easy to follow. You might consider decomposing it to separate the phases more clearly into their own functions: (1) parse input date, (2) generate dates needed, (3) convert dates to strings. That said, the task at hand is fairly simple, so leaving it all in one function could make sense in many contexts. The primary problem with the current code is that it doesn't take advantage of simpler strategies. Here's one to consider:

from datetime import datetime, date, timedelta
from calendar import monthrange

def generate_rolling_date(input_date, number_of_dates):
    # Parse input date and change it to the 15th.
    FMT = '%d-%b-%y'
    MID = 15
    d = datetime.strptime(input_date, FMT).replace(day = MID)

    # Yield the desired dates as strings.
    for i in range(number_of_dates):
        # Select 1st or last day of month.
        ndays = monthrange(d.year, d.month)[1] if i % 2 else 1
        yield date.strftime(d.replace(day = ndays), FMT)

        # Advance to prior month if we yielded last day of month.
        if ndays > 1:
            d = (d - timedelta(days = 30)).replace(day = MID)
\$\endgroup\$
3
  • \$\begingroup\$ It should be possible to restructure your loop so that you don't need to do an even/odd check on monthrange and can just iterate directly. \$\endgroup\$
    – Reinderien
    Jan 12, 2022 at 21:55
  • \$\begingroup\$ @Reinderien Perhaps so. I assume you're referring to the odd/even check on i since monthrange isn't checked for anything. I did play with a few alternatives, and am open to evaluating specific suggestions. \$\endgroup\$
    – FMc
    Jan 13, 2022 at 0:27
  • \$\begingroup\$ How about making a pre-loop generator expression that does your for i iteration, with an inner for day in monthrange - this will flatten, and then you can iterate over it. \$\endgroup\$
    – Reinderien
    Jan 13, 2022 at 0:53
1
\$\begingroup\$

Documentation

Your function generate_rolling_date() does something somewhat special and unusual. Its name does not convey, what exactly it is doing. But, to be honest, I cannot think of a better name at the moment, that fits in <80 characters of line length. In any case you should document in the docstring what the function does and returns, since it is anything but trivial.

Divide and conquer

Your function generate_rolling_date() does far too much at the moment. It parses dates from strings, interates, increments and decrements dates. Split those actions into different functions that are less complex.

Get your types as early as possible

You obviously want to handle dates, so your main function generate_rolling_date() should take in and return datetime.date objects, not strings. Stringly typed is an anti-pattern and code-smell.

Comment what needs clarification

Comments should be used to clarify why your code does something, if and only if it is not obvious. Comments should never explain what the code does. This is obvious by reading the code itself:

# Initialize an empty output list
output = []

...

# Append selected_day to the output
output.append(date.strftime(selected_day, "%d-%b-%y"))

All your comments are examples on how not to use comments.

Suggested change

from calendar import monthrange
from datetime import date
from typing import Iterator


def get_first_of_month(date_: date) -> date:
    """Returns the first day of the given month."""

    return date_.replace(day=1)


def get_last_of_month(date_: date) -> date:
    """Returns the last day of the given month."""

    _, days = monthrange(date_.year, date_.month)
    return date_.replace(day=days)


def get_previous_month(date_: date) -> date:
    """Returns the previous month."""

    try:
        return date_.replace(month=date_.month-1, day=1)
    except ValueError:
        return date_.replace(year=date_.year-1, month=12, day=1)


def generate_rolling_dates(start: date, amount: int) -> Iterator[date]:
    """Generates rolling dates.
    For the given amout of turns, return the first of the month
    and on the next iteration the last of the month.
    If the last value was the last of the month,
    return the first of the previous month.
    """

    for index in range(amount):
        if index % 2:
            yield get_last_of_month(start)
            start = get_previous_month(start)
        else:
            yield get_first_of_month(start)


def main() -> None:
    """Test the code with the example date."""

    start = date.fromisoformat('2021-02-01')

    for date_ in generate_rolling_dates(start, 8):
        print(date_)


if __name__ == '__main__':
    main()
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I really don't recommend your algorithm in get_last_of_month - it's not necessary to iterate through every single day in the month to get the last one. \$\endgroup\$
    – Reinderien
    Jan 12, 2022 at 21:53
  • 1
    \$\begingroup\$ Yeah, why not to use the calendar module? \$\endgroup\$ Jan 13, 2022 at 7:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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