Improvements with caesar cipher encryption

Question

I have wanna learn more of ways to improve my code and also, make it look different. I want to see different approach but have the same result.

def text():
 print('What message(s) are you trying to encrypt?')
 return input()
def shift():
 key = 0;
 while True:
 print('Enter the key number (1-%s)' % (keymax))
 key = int(input())
 if (key >= 1 and key <= keymax):
 return key
def encrypt(string, shift):
 hidden = ''
 for char in string:
 if char.isalpha() == False:
 hidden = hidden + char
 elif char.isupper():
 hidden = hidden + chr((ord(char) + shift - 65) % 26 + 65)
 else:
 hidden = hidden + chr((ord(char) + shift - 97) % 26 + 97)
 return hidden
text = text()
s = shift()
print("original string: ", text)
print("after encryption: ", encrypt(text, s))

It would be gladly appreciated if anyone could lend a hand or help out! :)

• Jaideep Shekhar: text = text() be careful! Try to call text again. ;-)
• David G.: keymax is missing.

Answer 1

Just a couple of small things you could improve/change on your code:

1. Unnecessary function/print statement. You can just do text = input('What message ... encrypt?\n') instead of making an extra function for it an using a seperate print statement. The same goes for the input statement in the shift function, you can put the text of the print statement inside of the input.

2. Unnecessary default value for variable. In the shift function you assign the value 0 to the variable key on forehand. But as you never use it outside the while True statement this is unnecessary. This line can be removed.

3. 'Magic numbers'. Inside the encrypt function you use the ascii codes 65, 26 and 97 to quickly convert the characters. While their meaning might be obvious to you (and in this trivial case to most people) it is never a good idea to do this. It is cleaner to make a variable on top of your program in uppercase defining the value. So something like: UPPER_ASCII_SHIFT = 65 and then using that variable in the rest of the code. This way it is much easier to read for someone else. This is also a big theme in the cs50 course where this programming problem is originally from.

4. Try choosing clearer names for variables that immediately clarify their use and content. Something like hidden is a bit ambiguous, maybe encrypted_string is better? Also try to avoid single letter variable names like s, but rather call it something like shift_distance.

Answer 2

I spent a little time doing some cleanup and here's what I ended up with:

NUM_LETTERS = 26
def get_text() -> str:
 return input('What message(s) are you trying to encrypt? ')
def get_shift(shift_max: int) -> int:
 try:
 key = int(input('Enter the key number (1-%s) ' % (shift_max)))
 assert 1 <= key <= shift_max
 except (AssertionError, ValueError):
 return get_shift(shift_max)
 else:
 return key
def encrypt(text: str, shift: int) -> str:
 def encrypt_char(char: str) -> str:
 if not char.isalpha():
 return char
 alpha = ord('A') if char.isupper() else ord('a')
 return chr((ord(char) + shift - alpha) % NUM_LETTERS + alpha)
 return ''.join([encrypt_char(char) for char in text])
text = get_text()
shift = get_shift(NUM_LETTERS)
print("original string: ", text)
print("after encryption: ", encrypt(text, shift))

Notable changes:

1. No magic numbers. I used ord('A') for the ASCII codes and defined NUM_LETTERS for the shift operation.

2. Changed function names to match what they're doing (e.g. your shift function didn't actually shift anything, it just got the shift value from the user, so I renamed it get_shift).

3. Your shift function didn't handle non-int inputs, so I fixed that, and since that's driven by a raised exception I made the bounds check work that way too. Just for fun and to demonstrate other ways of doing the same thing, I made it loop via recursion since that lets you get rid of the while construct and reduces the indentation (this isn't necessarily always a good idea in Python because if you recurse enough times it'll blow the stack, but for this case if the user enters garbage ten thousand times it's probably fine to have the script exit, lol).

4. I broke the character encryption logic into its own mini-function, which makes it easy to do the full string encryption as a list comprehension statement.

5. DRY (Don't Repeat Yourself) -- since the character shifting logic is identical except for what the start of the alphabet is, I combined those two lines into one.

6. Type annotations!

• Yonlif: The recursion in get_shift does not look that good, I would replace it with a while loop. Also you missed the return type annotation of encrypt. Except for that - great.
• sorasu: I see, this was very helpful. I really appreciate your time!
• Surya Prasad Khanal: can you add number also?