5
\$\begingroup\$

This is code for a caesar cipher that I wrote, I would love for anyone who can to offer any improvements/corrections/more efficient methods they can think of. Thanks.

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"," ","1","2","3","4","5","6","7","8","9","0",",","."]
enc_text=[]
dec_text=[]

def number_check(a):
  valid=False
  while valid==False:
    try:
      number=int(a)
      valid=True
    except ValueError:
      print("Invalid entry, You must enter a number.")
      a=input("please enter a number.\n")
  return number

def encrypt():
  for x in org_text:
    index=alphabet.index(x)
    enc_letter=(index+int(enc_key))%39
    new_let=alphabet[enc_letter]
    enc_text.append(new_let)
  return enc_text

def decipher():
  for x in message:
    index=alphabet.index(x)
    dec_letter=(index-int(enc_key))%39
    new_let=alphabet[dec_letter]
    dec_text.append(new_let)
  return dec_text

enc_key=number_check(input("Please enter your encryption key:\n"))
org_text=input("please enter the message you would like encrypted:\n").upper()
message=encrypt()
message1="".join(message)
print(message1)
decode=decipher()
decode1="".join(decode)
print(decode1)
\$\endgroup\$
6
\$\begingroup\$

PEP-8

White space! Your code needs more white space! The Style Guide for Python Code enumerates many coding conventions Python programs should follow. One is binary operators (like =, ==, +, % should have one space character before and after the operator.

alphabet is a constant. It never changes. PEP-8 would recommend you changing the name to ALPHABET to indicate it is a constant.

List of characters

A string is essentially just a list of characters. Moreover, it will behave exactly as if it is list of characters.

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"," ","1","2","3","4","5","6","7","8","9","0",",","."]

could be replaced with the much less verbose ...

alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ 1234567890,."

... with no other changes required in your program.

One source of truth

How many characters are in your alphabet? Did you count? 39? Are you sure? Great! What if you added an apostrophe to the alphabet. The 39 would become 40, and you'd have to change both occurrences of 39 in your code to 40.

Why are we counting and risking making a mistake? Computers are much better at counting!

ALPHABET_LENGTH = len(alphabet)
...
    enc_letter = (index + int(enc_key)) % ALPHABET_LENGTH
...
    dec_letter = (index - int(enc_key)) % ALPHABET_LENGTH

Now if you decide additional letters should be included in the alphabet, you just add them; no other change would be required.

Global variables

alphabet is a global constant. It never changes. In contrast to that, enc_text, dec_text, org_text and message are global variables. They are mutated by the program.

Avoid global variables. They make the code harder to understand, and harder to maintain.

Consider encrypt(). You call it and org_text is processed, encrypted and added to the enc_text list. What happens if you change org_text and call encrypt() again? The new text gets added to enc_text.

Was that expected?

If enc_text was removed from global scope, and defined inside the encrypt() function, this unexpected behaviour would not happen. Same with dec_text.

Additionally, global variables should not be used to pass the text to be encrypted/decrypted into the functions; these should be passed as parameters.

Bug

If you encrypt the message "This fails!", the program crashes with the exception ValueError: '!' is not in list.

This is another reason to use an alphabet string instead of a list of characters: strings have another method, .find(), which will return -1 if the target isn't found. A list has no such method, forcing you to use try: ... except: ... to catch the exception.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks @AJNeufeld, incredible feedback. \$\endgroup\$
    – chaim8
    May 27 at 18:00
2
\$\begingroup\$

If you look at your encrypt() and decrypt() you might notice that they are mirror images of each other based on key. This might allow us to base them both on a common function that does the rotation for us.

Let's try this via a closure. Since we have a closure, this will also provide us an opportunity to remove most of the global junk.

Remember that there is still the "invalid character" issue to deal with though when using this code.

## --------------------------
## Calling this closure returns a pair of function that can be used
## encrypt and decrypt plain text messages.
## Note that decryption is a mirror of encryption.
## --------------------------
def make_crypt():
    ## remove alphabet from our global context.
    alpha = "ABCDEFGHIJKLMNOPQRSTUVWXYZ 1234567890,."
    len_alpha = len(alpha)

    ## this *might* be faster than using alpha.index(letter)
    alpha_index = {letter: index for index, letter in enumerate(alpha)}

    ## --------------------------
    ## a function to do rotation based on a key with
    ## a direction.
    ## --------------------------
    def crypt(text_in, key):
        return "".join([
            alpha[ (alpha_index[letter] + key) % len_alpha ]
            for letter in text_in
        ])
    ## --------------------------

    ## --------------------------
    ## return a pair of functions that we can use
    ## --------------------------
    return (
        lambda text, key: crypt(text, key),
        lambda text, key: crypt(text, -key)
    )
    ## --------------------------

encrypt, decrypt = make_crypt()
## --------------------------

## --------------------------
## Get our encryption key and ensure that it is an int
## --------------------------
def get_key():
    try:
        return int(input("Please enter your integer encryption key:\n"))
    except ValueError:
        print("Invalid entry, encryption key must enter an integer.")
        return get_key()
## --------------------------

enc_key = get_key()
org_text = input("please enter the message you would like encrypted:\n").upper()

secret_message = encrypt(org_text, enc_key)
print(secret_message)

decoded_message = decrypt(secret_message, enc_key)
print(decoded_message)
\$\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.