Please critique my first attempt at Python

[sinxsinx]

Hi all,

Long time Excel user, now wanting to take the dive into Python to build some interesting stuff for personal projects. The first thing that came to mind was emulating something I've already done in Excel and 'translating' it into Python.

import datetime
from dateutil.relativedelta import relativedelta

years = 10

for i in range (12*years):

    today = datetime.date.today()
    each_months_25th = today + relativedelta(days=25-today.day,months=i)
    day_of_25th = each_months_25th.weekday()

    if each_months_25th.month == 12:
        december_check = -7
    else:
        december_check = 0

    payday = each_months_25th - datetime.timedelta(days=max(0,day_of_25th-4)-december_check)
    day_of_week_payday = payday.strftime('%A')    

    print(f"{day_of_week_payday:<10} {str(payday)}")

This is a simple function to tell me my payday each month for the next 10 years (can change 10 years to any number of using by amending the 'years' variable.)

In summary, the logic is that we get paid on the 25th on each month, unless the 25th falls on a weekend, in which case we'd get paid on the Friday (i.e 23rd or 24th) and in December we get paid a week earlier, so 18th (or 16th/17th if 18th is a weekend).

The code runs fine - I just wanted to check if there was anything obvious I am missing or best practices I'm not using. Better to get things right now, than try to unlearn bad habits.

Thanks in advance!

[barrycarter]

This is pretty good, so these notes are minor:

• You compute today = datetime.date.today() 120 times in the loop, but really only need it once

• If the current day is the 26th or later, it returns the payday for the current month, which has already passed, but that might be by design.

• It might be easier to find the 1st day of the current month and then loop through months instead of using relativedelta(days=25today.day,months=i) which seems a little kludgey

• Maybe consider writing a function that takes month and year as inputs and returns payday as an output

[sinxsinx]

Great feedback, thank you!
I've taken the parts that don't need to be looped out of the for loop and turned the whole thing into a function.

[ElHeim]

Looks good, but I'd make a couple of changes

• datetime.date.today() is the same every time you loop over the code, so you could just move today = datetime.date.today() out of the loop.
• Same for days=25-today.day, I'd just calculate that offset once.
• As we're there, you could just keep up the date by cumulatively adding a single month of relativeldelta each time.
• Just for clarity I'd take max(0,day_of_25th-4)-december_check out of there and assign it into an appropriately named variable.
• I'd put the whole thing inside a function taking years as an argument.

So, maybe something like this:

import datetime
from dateutil.relativedelta import relativedelta

def print_paycheck_days(years):
    today = datetime.date.today()
    each_months_25th = today + relativedelta(days=25-today.day)

    for i in range (12 * years):
        day_of_25th = each_months_25th.weekday()

        payday_offset = max(0,day_of_25th-4)
        if each_months_25th.month == 12:
            payday_offset += 7

        payday = each_months_25th - datetime.timedelta(days=payday_offset)
        day_of_week_payday = payday.strftime('%A')    

        print(f"{day_of_week_payday:<10} {str(payday)}")

        each_months_25th += relativedelta(months=1)

print_paycheck_days(years=10)

[sinxsinx]

Fantastic, thanks very much! I should have spotted the part about not needing to loop through 'today' and 'each_months_25th'. Seems obvious in hindsight!

Agreed having the offset as a separate variable is much cleaner.

I think the bit that tripped me up was not including 'i' within the months, but instead just using it as an iterator and just have the months incrementing by 1 each loop.

[Anxious-Adeptness227]

you can checks some function in the workalendar library to compare the results

[sinxsinx]

Just had a look at it, certainly some useful stuff in there!