6
\$\begingroup\$

I am a bit new to Python. This is my first code review, and I am excited to get a critique on how to make this (very simple) shift cipher more Pythonic, cleaner, and more organized.

alphabet = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j','k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

def shift(offset):

    message = input("Input Message You Would Like Encrypted:\n")
    new_message = ''

    for letter in message:

        letter = letter.lower() #doesn't handle upper-case yet

        if letter.isalpha():
            shift_pos = alphabet.index(letter) + offset
            new_pos = alphabet[shift_pos]
            new_message += new_pos

        #these will not be shifted

        elif ' ' or '/t' or '/n' in letter: 
            new_message += letter

        elif letter.isnumeric(): 
            new_message += letter

        else:
            print("An error took place in recording the message. Check input.\n")

    print(new_message)


shift(-1)
\$\endgroup\$
0
13
\$\begingroup\$

Bug

elif ' ' or '/t' or '/n' in letter: 
    new_message += letter

Provided that execution reaches that point (i.e. letter.isalpha() is false), this condition always evaluates to True, because the space character is a non-empty string. The rest of the expression doesn't matter due to short-circuit evaluation of or. For the record, the string consisting of the two characters / and t is always True, and the two-character string '/n' can never appear within a one-character string.

Of course, that means that the elif letter.isnumeric() and the else branches are unreachable.

Did you mean to write this instead?

elif letter in ' \t\n':
    new_message += letter

Naming

letter might not be a letter of the alphabet. A better name would be character, char, or just c.

Design

shift would be a better as a pure function: it should accept the message as one of its parameters (instead of prompting for it) and return the enciphered result (instead of printing it). Then, you would call

print(shift(input("Input message you would like encrypted:\n"), -1))

Why do you want to disallow certain characters? If digits and spaces are to be passed through unmodified, why shouldn't the same be true of punctuation?

Efficiency

Strings in Python are immutable. Every time you call += on a string, it allocates a new string and fills it with the old content and the new ending. Therefore, += is a relatively expensive operation and should be avoided.

A better alternative would be to build a list (by appending or using a list comprehension). Then, you construct the result by calling ''.join(…).

Pythonic cleverness

Your alphabet could just be string.ascii_lowercase instead of that list.

The classic way to implement a Caesar Cipher in Python is using str.translate().

from string import ascii_lowercase as ALPHABET

def shift(message, offset):
    trans = str.maketrans(ALPHABET, ALPHABET[offset:] + ALPHABET[:offset])
    return message.lower().translate(trans)

print(shift(input("Input message you would like encrypted:\n"), -1))
\$\endgroup\$
1
  • \$\begingroup\$ I appreciate this response. There is a TON of valuable information to dig through, and I will be doing so, shortly. With regard to disallowing certain characters. I moreso did not want to manually account for all possible inputs before I got feedback on direction, but I can see that the string.ascii_lowercase is a superior option to doing that. :) I will refactor to include that, and I will test it out. Thanks again. \$\endgroup\$
    – j-grimwood
    Jun 28 '18 at 19:42
3
\$\begingroup\$

Non ascii characters

You should check out what will happen if your string to cypher would include non ascii character. For example polish letter 'ą'. It is alpha character and it will go into check of index, but as this value is not included in the list of characters trying to get it's index will result with Value Error.

Tests

Code is easier to understand and be checked if there are tests included. Write up some tests to check if the algorithm actually works.

\$\endgroup\$

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.