Caesar cipher using Python

Question

I am just beginning to learn Python, this is my own take on a Caesar cipher just looking at Python docs. I would like to know how I could have made it better and how would you rate this. Is it any good?

#caeser cipher
import string
message = str(input("Enter the Message you want to encrypt: "))
shift = int(input("Enter the number of letters for cipher shift: "))
def cipher(message,shift):
 encList = []
 messageLst = []
 alphabet = list(string.ascii_lowercase * 2) 
 punct = list(string.punctuation)
 for i in message:
 messageLst.append(i)
 for i in messageLst :
 for j in alphabet:
 if i == j:
 replaceChar = alphabet[alphabet.index(j)+(shift)]
 encList.append(replaceChar)
 break
 elif i == " ":
 encList.append(i)
 break
 elif i in punct:
 encList.append(i)
 break
 
 encMessage = ""
 encMessage = encMessage.join(encList)
 #print(alphabet)
 #print(messageLst)
 #print(encList)
 print("Encrypted Message is : ", encMessage)
cipher(message,shift)

Answer 1

Alphabet list

Alphabet doesn't need to be a list, you can just use the ascii_lowercase string:

# just a copy
alphabet = string.ascii_lowercase
# two copies
alphabet = string.ascii_lowercase * 2
# alternatively
from string import ascii_lowercase as alphabet

Using the Modulo Operator

Instead of using two copies of ascii_lowercase to cover index overflows, you can use the modulo operation as shown by your linked Wikipedia entry:

alphabet = string.ascii_lowercase
shift = 5
# index is in range(0, 25)
alphabet[(6 + shift) % 26]
'l'
# index would be > 25, so it loops back around
alphabet[(21 + shift) % 26]
'a'

Cipher Lookup with a Dictionary

Now, what you could do is use a dictionary to build the lookups ahead of time so that you don't have to use alphabet.index:

cipher_lookup = {char: alphabet[(i + shift) % 26] for i, char in enumerate(alphabet)}

Now, there's no more need to track indices:

def cipher(message, shift):
 cipher_lookup = {char: alphabet[(i + shift) % 26] for i, char in enumerate(alphabet)}
 encrypted = []
 for letter in message:
 # simply look it up in the dictionary
 encrypted.append(cipher[letter])

Checking for Punctuation

Use a set here, rather than a list. Membership testing for a set/dict is a time-constant operation, rather than O(N), where N is the length of the list:

punct = set(string.punctuation)
'a' in punct
False
'.' in punct
True

However, you don't actually have to test for punctuation. You could instead use dict.get to return the value from the dictionary if it's there, and return the letter if it isn't:

# say shift is 5
cipher_lookup.get('a', 'a')
'f'
# punctuation is not in the cipher
# so we just return that character
cipher_lookup.get('.', '.')
'.'

Predefining encMessage

You can just use:

enc_message = ''.join(enc_list)

Variable Naming

Variable and function names are to be snake_case with all lowercase letters

Function params

Separate your parameters in your function definitions with a space:

def cipher(message, shift):

input

You don't need to cast message to str, since input only outputs strings

if __name__ == "__main__" Guard

This will allow you to import this function without executing the program if you wanted to reuse it:

# This goes at the very bottom
if __name__ == "__main__":
 message = input("Enter the Message you want to encrypt: ")
 shift = int(input("Enter the number of letters for cipher shift: "))
 print(cipher(message, shift))

You can put your message and shift prompts here as well so that they don't execute unless you are running the program.

Refactored

from string import ascii_lowercase as alphabet
def cipher(message, shift):
 cipher_lookup = {char: alphabet[(i + shift) % 26] for i, char in enumerate(alphabet)}
 encrypted_message = ""
 for letter in message:
 encrypted_message += cipher_lookup.get(letter, letter)
 return encrypted_message
if __name__ == "__main__":
 message = input("Enter the Message you want to encrypt: ")
 shift = int(input("Enter the number of letters for cipher shift: "))
 print(cipher(message, shift))

String concatenation

It's not usually best practice to use string concatenation, but I think this is a good start to demonstrate the concepts of what's going on in the program. A better practice than the for loop would be to use a generator expression:

encrypted_message = ''.join((cipher_lookup.get(letter, letter) for letter in message))

Answer 2

C.Nivs has made excellent points about:

• not needing to convert alphabet into a list
• using the Modulo Operator (%)
• storing the translations in a dictionary
• using set/dict and in for membership tests
• naming, spaces, unnecessary casts and initializations
• __main__ guards

I won't repeat those here, but please study those and follow their advice on those points.

---

Additional Code Review Points

Loop Invariants

You wrote this double for loop:

 for i in messageLst :
 for j in alphabet:
 if i == j:
 replaceChar = alphabet[alphabet.index(j)+(shift)]
 encList.append(replaceChar)
 break
 elif i == " ":
 encList.append(i)
 break
 elif i in punct:
 encList.append(i)
 break

The tests i == " " and i in punct are nested inside the inner loop, which loops for j in alphabet. Notice that neither those tests nor the code executed based on passing either of those tests depends on j. This is inefficient.

Consider a messageLst which contains ['z', 'e', 'b', 'r', 'a']. The first iteration of the outer loop

• assigns 'z' to the variable i, and then
• executes the body of that loop, which is the inner loop. That loop:
  • assign j to the first letter of alphabet (an 'a'), and since i == j is false, goes on to check if i is a space, and if not, if i is a punctuation character. Since both were false, the loop continues and,
  • assign j to the second letter of alphabet (a 'b'), and since i == j is false, goes on to check if i is a space, and if not, if i is a punctuation character. Since both were false, the loop continues and,
  • assign j to the third letter of alphabet (a 'c'), and since i == j is false, goes on to check if i is a space, and if not, if i is a punctuation character. Since both were false, the loop continues and,

Notice those checks for i == ' ' and i in punct are redundantly executed ... 26 times in case of the letter 'z'!

If those tests are moved out of the loop, we can get a more efficient implementation:

 for i in messageLst :
 if i == " ":
 encList.append(i)
 elif i in punct:
 encList.append(i)
 else:
 for j in alphabet:
 if i == j:
 replaceChar = alphabet[alphabet.index(j)+(shift)]
 encList.append(replaceChar)
 break

Unnecessary looping

Consider just the inner loop:

 for j in alphabet:
 if i == j:
 ...
 break

This searches alphabet for values j that equals i, and (due to the break) stops at the first occurrence. When that occurrence is found, j will equal i.

This is a complicated way of writing:

 if i in alphabet:
 j = i
 ...

In fact, you don't even need the extra j variable. You can simply use i in the ... code.

 if i in alphabet:
 replaceChar = alphabet[alphabet.index(i) + shift]
 encList.append(replaceChar)

Improved code

Fixing the loop invariant and removing the unnecessary looping (but without applying improvements from the other answer), we get:

 for i in messageLst :
 if i in alphabet:
 replaceChar = alphabet[alphabet.index(i) + shift]
 encList.append(replaceChar)
 elif i == " ":
 encList.append(i)
 elif i in punct:
 encList.append(i)

... which is certainly an improvement over the original code.

Batteries Included

C.Nivs also gave you improvements on string concatenation, culminating with a generator expression to produce the encrypted message. We can do even better...

Python comes with a str.translate function, which does a letter-by-letter substitution on a string ... which is exactly what the Caesar cipher is doing.

from string import ascii_lowercase as alphabet
def cipher(message: str, shift: int) -> str:
 """
 Encode a message using a Caesar Cipher with a user-defined shift on
 a 26 letter lowercase alphabet.
 """
 shift %= len(alphabet)
 code = str.maketrans(alphabet, alphabet[shift:] + alphabet[:shift])
 return message.translate(code)
if __name__ == "__main__":
 message = input("Enter the Message you want to encrypt: ")
 shift = int(input("Enter the number of letters for cipher shift: "))
 print(cipher(message, shift))

Note: I've added a """docstring""" and type-hints to the cipher function. Both are extremely useful in Python development. Make sure you study them.

• N3buchadnezzar: Great answer! Minor nitpicks, just reading the code the part code = ... is a bit terse, adding shifted_alphabet = alphabet[shift:] + alphabet[:shift] above would make it clearer. len of alphabet is called each time a new cipher is called. Maybe make it a constant ALPHABET_LEN = len(alphabet). Some minor checking on the inputs would be nice. What happens if shift is not an int? Also some very brief doctests would make cipher clearer as it is a bit terse. code is better named translation_table or cipher =).
• N3buchadnezzar: As a last comment =) str is imho not helping much. Typing hints should ideally hint at what is going on. I find it clearer adding from typing import Annotated and then do something like Plaintext = Annotated[str, "a plaintext to be ciphered"], Ciphertext = Annotated[str, "an encrypted plaintext using a cipher"] and then def cipher(message: Plaintext, shift: int) -> Ciphertext:.