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

"Day-of-the-week-finder"

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

Problem

I'm new to Python/Programming and wrote this simple "Day of the week/Holiday finder" script. Ideally I'd use something so I can have a little widget on my desktop, but right now I'm looking for any and all suggestions to improve my code?

I'm importing this holiday module

import datetime
import holidays

today = datetime.date.today()
today = today.strftime("%m/%d/%Y")
today = datetime.datetime.strptime(today, "%m/%d/%Y")

def days_between(d1):
    d2 = today
    return (d2 - d1).days

def find_dt(input_dt):
    print(input_dt)
    dt1 = datetime.datetime.strptime(input_dt, "%m/%d/%Y")
    dt2 = dt1.strftime('%A')

    if days_between(dt1) == 0:
        print("Today IS {d}.".format(d=dt2))
    elif days_between(dt1) = 1:
        print("It wasn't a holiday.")
    elif days_between(dt1) == 0:
        print("Today is {h}.".format(h=us_h))
    elif days_between(dt1) <= 1:
        print("It'll be {h}.".format(h=us_h))
    else:
        print("It was {h}.".format(h=us_h))

def main():
    d = input('Type a date using MM/DD/YYYY: ')
    # d = '12/25/2015'
    find_dt(d)

main()

Solution

today

First of all, I'm super confused as to why you're doing

today = datetime.date.today()
today = today.strftime("%m/%d/%Y")
today = datetime.datetime.strptime(today, "%m/%d/%Y")


It seems like you'd be fine if you just did

today = datetime.datetime.today()


Additionally, constants (such as today) are traditionally named in ALL_CAPS.

days_between

It is very counter-intuitive to me that this only takes a single argument. I think writing it this way

def days_between(d1, d2):
    return (d2 - d1).days

def days_from_today(date):
    return days_between(date, today)


makes much more sense.

find_dt

I don't like everything that is going on here. First of all, I would argue that it shouldn't be the job of find_dt to convert an input string to a datetime object - the caller should be responsible for that.

Next, you're doing two distinct things. One is displaying the day of the week for the given date, and the other is displaying information about that day being a US holiday. These should probably be distinct.

def day_of_week(date):
    date_string = date.strftime("%A")

    from_today = days_from_today(date)
    if from_today == 0:
        info_string = "Today IS {}"
    elif from_today <= 1:
        info_string = "It will be a {}"
    else:
        info_string = "It was a {}"
    print(info_string.format(date_string))


A few notes about this. I didn't call days_from-today every time - that's pretty wasteful. Additionally, because each one does basically the same thing, instead of repeating the print I create the appropriate string and then just print into that at the end.

Next, holidays.

def holiday_status(date):
    us_holidays = holidays.UnitedStates()
    us_h = us_holidays.get(date)

    from_today = days_from_today(date)

    if us_h is None:
        if from_today == 0:
            print("Today is not a holiday")
        elif from_today <= 1:
            print("It won't be a holiday")
        else:
            print("It wasn't a holiday")
    else:
        if from_today == 0:
            info_string = "Today is {}"
        elif from_today <= 1:
            info_string = "It'll be {}"
        else:
            info_string = "It was {}"
        print(info_string.format(us_h))


Notice that I used some similar strategies as with day_of_week. I also broke up your if statements a little more - while some people think nesting is bad, I think in this case it makes it much more clear what the conditions are.

__main__

You should have a block like this

if __name__ == '__main__':
    main()


at the bottom of your file. Then it is import safe.

Locale

While right now you assume the US, it is conceivable that your users may be from other locales. You probably want something like this (the specifics of this depend on your Python version, but this is a rough approximation)

LOCALE_ENUM = enum("US UK FR JP ...")
LOCALES = {
    LOCALE_ENUM.US: holidays.UnitedStates(),
    LOCALE_ENUM.UK: holidays.UnitedKingdom(),
    ...
}


and then to define both find_dt and holiday_status to take a LOCALE_ENUM instance and use it to determine what locale's holidays they should use.

Code Snippets

today = datetime.date.today()
today = today.strftime("%m/%d/%Y")
today = datetime.datetime.strptime(today, "%m/%d/%Y")
today = datetime.datetime.today()
def days_between(d1, d2):
    return (d2 - d1).days

def days_from_today(date):
    return days_between(date, today)
def day_of_week(date):
    date_string = date.strftime("%A")

    from_today = days_from_today(date)
    if from_today == 0:
        info_string = "Today IS {}"
    elif from_today <= 1:
        info_string = "It will be a {}"
    else:
        info_string = "It was a {}"
    print(info_string.format(date_string))
def holiday_status(date):
    us_holidays = holidays.UnitedStates()
    us_h = us_holidays.get(date)

    from_today = days_from_today(date)

    if us_h is None:
        if from_today == 0:
            print("Today is not a holiday")
        elif from_today <= 1:
            print("It won't be a holiday")
        else:
            print("It wasn't a holiday")
    else:
        if from_today == 0:
            info_string = "Today is {}"
        elif from_today <= 1:
            info_string = "It'll be {}"
        else:
            info_string = "It was {}"
        print(info_string.format(us_h))

Context

StackExchange Code Review Q#109546, answer score: 2

Revisions (0)

No revisions yet.