2
\$\begingroup\$

I have implemented Zeller's algorithm in Python to calculate the day of the week for some given date. I would like to get some feedback on how I can make the code shorter and more elegant:

def input_value(input_name, prompt, min_num, max_num):
    while True:
        data = raw_input(prompt)
        if data:
            try:
                input_name = int(data)
            except ValueError:
                print 'Invalid input...'
            else:
                if input_name >= min_num and input_name <= max_num:
                    return input_name
                    break
                input_name = 'Please try again: '
        else:
            print 'Goodbye!'
            break


month = 0
day = 0
year = 0
century = 0

month = input_value(month, "Please enter the month (from 1-12, where March is 1 and February is 12): ", 1, 12)
day = input_value(day, "Please enter the day (from 1-31): ", 1, 31)
year = input_value(year, "Please enter the year (from 0 - 99, eg. 88 in 1988): ", 0, 99)
century = input_value(century, "Please enter the century (from 0 - 99, eg. 19 in 1988): ", 0, 99)

A = month
B = day
C = year
D = century

W = (13*A - 1) / 5
X = C / 4
Y = D / 4
Z = W + X + Y + B + C - 2*D
R = Z % 7

birthday = "You were born on " + str(B) + "/" + str(A+2) + "/" + str(D) + str(C) + " which was a "

if R == 0:
    print birthday + 'Sunday'
elif R == 1:
    print birthday + 'Monday'
elif R == 2:
    print birthday + 'Tuesday'
elif R == 3:
    print birthday + 'Wednesday'
elif R == 4:
    print birthday + 'Thursday'
elif R == 5:
    print birthday + 'Friday'
elif R == 6:
    print birthday + 'Saturday'

Thanks in advance for taking the time to look at this.

\$\endgroup\$
5
\$\begingroup\$
def input_value(input_name, prompt, min_num, max_num):

input_name is always a number - why are you calling it a 'name'? Also, there's no need to take it as an argument, since you never use its initial value in your function.

    while True:
        data = raw_input(prompt)
        if data:
            try:
                input_name = int(data)
            except ValueError:
                print 'Invalid input...'

Consider putting a continue here, for clarity. It isn't necessary, though.

            else:
                if input_name >= min_num and input_name <= max_num:

Python understands if min_num <= input_name <= max_num:

                    return input_name
                    break

You don't need break here, the return makes it unreachable.

                input_name = 'Please try again: '

Assigning a string to something that's otherwise always a number is odd, and this value is never used anyway. Nix this line.

        else:
            print 'Goodbye!'
            break

This will end up falling off the end of the function, and so returning None. The rest of your script will continue, but eventually fail horribly. This probably isn't what you want.

month = 0
day = 0
year = 0
century = 0

Only reason to initialise these to 0 is to pass them into input_value, but, as I said above, it never uses the zero values. So, you don't need this.

month = input_value(month, "Please enter the month (from 1-12, where March is 1 and February is 12): ", 1, 12)

March = 1 is an odd system. Consider inputting it as January=1 and then transforming it with:

# Weirdify month counting
month = month - 2 if month < 3 else month + 10

To not surprise your users.

day = input_value(day, "Please enter the day (from 1-31): ", 1, 31)
year = input_value(year, "Please enter the year (from 0 - 99, eg. 88 in 1988): ", 0, 99)
century = input_value(century, "Please enter the century (from 0 - 99, eg. 19 in 1988): ", 0, 99)

These lines are very long. You should wrap them per PEP8:

 year = input_value("Please enter the century (from 0 - 99, eg. 19 in 1988): ",
                     0, 99)

Also, you might consider inputting year and century as a single value ('1988') and separating it in the script.

A = month
B = day
C = year
D = century

Just use the long names throughout your formulae, eg X = year / 4.

W = (13*A - 1) / 5
X = C / 4
Y = D / 4
Z = W + X + Y + B + C - 2*D
R = Z % 7

birthday = "You were born on " + str(B) + "/" + str(A+2) + "/" + str(D) + str(C) + " which was a "

Use string formatting instead of concatenation: birthday = "You were born on {}/{}/{}, which was a {}{}".format(day, month+2, century, year)

if R == 0:
    print birthday + 'Sunday'
elif R == 1:
    print birthday + 'Monday'
elif R == 2:
    print birthday + 'Tuesday'
elif R == 3:
    print birthday + 'Wednesday'
elif R == 4:
    print birthday + 'Thursday'
elif R == 5:
    print birthday + 'Friday'
elif R == 6:
    print birthday + 'Saturday'

This can go into a list:

 days = ['Sunday', 'Monday', 'Tuesday', 'Wednesday', 
         'Thursday', 'Friday', 'Saturday']
 day_of_week = days[R]

And then that can go in the above string formatting instead of being concatenated on later.

\$\endgroup\$
  • \$\begingroup\$ @Ivc thanks, this is really great advice. There are two points which I don't fully understand. I don't know how to use continue, even after looking it up on the python docs. I also don't fully understand the 'string formatting' and 'listing days of the week' point. Can you offer any more advice here? \$\endgroup\$ – Tom Kadwill Feb 12 '12 at 15:44
  • \$\begingroup\$ continue is sortof like break, except that it goes back to the top of the loop instead of falling out the bottom. For string formatting, look up the documentation for str.format(), and the "String Formatting mini language". Basically it achieves the same thing as concatenating all your variables into the string, except its easier to read, and potentially more efficient (especially if your code ever gets run on, say, PyPy). For the days of the week, days[0] is 'Sunday' - it avoids the chain of if/elif, and will error instead of failing silently if your R is >6 somehow. \$\endgroup\$ – lvc Feb 13 '12 at 3:51

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.