4
\$\begingroup\$

I am relatively new to python and am trying to teach myself.

I have created a python program that will take the current month's first three letters, and add one letter to the first, two to the second and three to the third.

It does this by converting the month's characters to an ASCII number, then adding X to that.

Please see the below code, which is what I wrote, and if you can, please provide me with feedback, how you feel it can be shortened etc.

import datetime

myDate = datetime.datetime.now()
month = myDate.strftime("%b")
month_list = list(month)
#below line is for testing other months or words. To use, please comment out the above three lines (myDate, month and month_list)
#month_list = ["Z", "z", "Z", ""]

letter_one = month_list[0]
letter_one_ascii = ord(letter_one)
letter_one_ascii += 1

if letter_one_ascii == 91 or letter_one_ascii == 123:
    letter_one_ascii = 65
else:
    letter_one = chr(letter_one_ascii)

letter_two = month_list[1]
letter_two_ascii = ord(letter_two)
letter_two_ascii += 2

if letter_two_ascii == 91 or letter_two_ascii == 123:
    letter_two_ascii = 65
elif letter_two_ascii == 92 or letter_two_ascii == 124:
    letter_two_ascii = 66
else:
    letter_two = chr(letter_two_ascii)

letter_three = month_list[2]
letter_three_ascii = ord(letter_three)
letter_three_ascii += 3

if letter_three_ascii == 91 or letter_three_ascii == 123:
    letter_three_ascii = 65
elif letter_three_ascii == 92 or letter_three_ascii == 124:
    letter_three_ascii = 66
elif letter_three_ascii == 93 or letter_three_ascii == 125:
    letter_three_ascii = 67
else:
    letter_three == chr(letter_three_ascii)

letter_one = chr(letter_one_ascii)
letter_two = chr(letter_two_ascii)
letter_three = chr(letter_three_ascii)

print "Your Password is: " + letter_one.upper(), letter_two.upper(), letter_three.upper()
\$\endgroup\$
12
\$\begingroup\$

Independent of the actual programming language you break a lot of rules and recommendations.

First - you shall not repeat yourself. The snippet

if letter_one_ascii == 91 or letter_one_ascii == 123:
    letter_one_ascii = 65

is repeated with slightly changed names

if letter_three_ascii == 91 or letter_three_ascii == 123:
    letter_three_ascii = 65

or slightly changed values

elif letter_three_ascii == 92 or letter_three_ascii == 124:
    letter_three_ascii = 66

Most probably you did not type that but you did copy-paste and edited names and/or values. This is very error prone as you could easily forget to change a name or value. Such errors are very hard to find. What would you do if you had to extend this generator to handle 5 characters? or 6? Typical solution: write a function that shifts by n letters and another one shifting by one letter

def shift_by_one(letter):
    # some magic here
    return new_letter

def shift_by_n(letter, n):
    # some magic here using previously defined function 'shift_by_one'
    return new_letter

you are not repeating yourself any more and extending comes easy. finally you should implement another function shifting a word according to your rules

def shift_word(word):
    # some magic here using previously defined function 'shift_by_n'
    return new_word

Second - you shall name your magic numbers, especially if they are appearing more than once. but even if appearing only once, what does 124 stand for? or 66? This is hard to maintain for you and even harder for a different person. Instead of 65 you could have something like

first_letter_ascii = ord('A')

Third - boundary checking. you can do that after incrementing like you did, but you can also do it before. The latter is preferred if outside values do not exist or do not make sense. Instead of

letter_one_ascii = ord(letter_one)
letter_one_ascii += 1

if letter_one_ascii == 91 or letter_one_ascii == 123:
    letter_one_ascii = 65
else:
    letter_one = chr(letter_one_ascii)

you could easily write

if letter_one == 'Z' or letter_one == 'z':
    letter_one = 'A'
else:
    letter_one_ascii = ord(letter_one)
    letter_one_ascii += 1
    letter_one = chr(letter_one_ascii)

That is much more readable and by the way this would make the body for shift_by_one.

Other options for boundary checking are modulo operations

arr = 'ABCD'
print(arr[4%len(arr)])

and deliberatly extending arrays outside

arr = 'ABCDA'
print(arr[4])

Fourth - you handle a lot of special cases as you process uppercase and lowercase. however in your final output you are interested in uppercase only. convert month to uppercase immediately and eliminate all 90+ numbers respectively letter 'z'.

month = myDate.strftime("%b").upper()

this also enables you to use a modulo boundary check and we can eliminate the loop in shift_by_n

def shift_by_n(letter, n):
    index = ord(letter) - ord('A')
    index = (index + n) % 26
    letter = chr(index + ord('A'))
    return letter

Finally some python stuff: you do not have to convert a string into a list to access characters, string provides slicing and iterability.

word = myDate.strftime("%b").upper()[:3]

if you end up in a list of characers and want to have a string you can join all characters by

''.join(mylist)

another nice feature in python is list comprehension which we use in shift_word

letters = [shift_by_n(letter,n+1) for n, letter in enumerate(word)]

we end up in

import datetime

def shift_by_n(letter, n):
    index = ord(letter) - ord('A')
    index = (index + n) % 26
    letter = chr(index + ord('A'))
    return letter

def shift_word(word):
    letters = [shift_by_n(letter,n+1) for n, letter in enumerate(word)]
    return ''.join(letters)

myDate = datetime.datetime.now()
word = myDate.strftime("%b").upper()[:3]
print "Your Password is: " + shift_word(word)
\$\endgroup\$
1
  • \$\begingroup\$ Very well stated. I totally missed he was .upper() at the end, would have made my answer a lot simpler too. \$\endgroup\$ – corn3lius May 18 '17 at 20:16
2
\$\begingroup\$

Why not pull all the repeated code into a function?

import datetime

myDate = datetime.datetime.now()
month = myDate.strftime("%b")
month_list = list(month)
#below line is for testing other months or words. To use, please comment out the above three lines (myDate, month and month_list)
#month_list = ["Z", "z", "Z", ""]

def mapLetters(inputLetters):
    retLetters =[]
    for i, l in enumerate(inputLetters):
        letter = ord(l) + (i + 1)
        # REPLACES 
        #letter_one = month_list[0]
        #letter_one_ascii = ord(letter_one)
        #letter_one_ascii += 1
        if letter > ord('Z') and letter < ord('a'): 
            letter = ord('A') + (letter - (1 + ord('Z')))
        elif letter > ord('z'):
            letter = ord('A') + (letter - (1 + ord('z')))
            ##  REPLACES 
            #if letter_three_ascii == 91 or letter_three_ascii == 123:
            #    letter_three_ascii = 65
            #elif letter_three_ascii == 92 or letter_three_ascii == 124:
            #    letter_three_ascii = 66
            #elif letter_three_ascii == 93 or letter_three_ascii == 125:
            #    letter_three_ascii = 67
            #else:
            #    letter_three == chr(letter_three_ascii)
        retLetters.append(chr(letter))
    return retLetters


print("{} -> {}".format(month_list, mapLetters(month_list)))
# ['M', 'a', 'y'] -> ['N', 'c', 'B']

Updated:

Changes based on @Graipher suggestions. and @Stefan's

Also removed 'magic numbers' for ascii.

\$\endgroup\$
1
  • \$\begingroup\$ You should try to adhere to best practices in your answer, specifically PEP8 (you use non-standard two spaces as indentation, have some spurious whitespace in some parenthesis and are not using lower_case for variables and shadow the built-in input). Otherwise a very nice first answer here! \$\endgroup\$ – Graipher May 18 '17 at 14:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy