patternpythonMinor
Caesar decipher
Viewed 0 times
caesardecipherstackoverflow
Problem
I have recently started learning Python and one of the assignments I have been working on is a Caesar decipher.
Can you tell me how I can make my code better?
Can you tell me how I can make my code better?
def caesar(code, shift):
''' (str, int) -> str
Shift every letter in the code by the integer 'shift'.
>>> caesar('ABC', 2)
CDE
>>> caesar('a b c', 4)
e f g
'''
decipher = ''
for char in code:
if 65<=ord(char)<=90:
decipher = decipher + chr((ord(char) - 65 + shift)%26+65)
if 97<=ord(char)<=122:
decipher = decipher + chr((ord(char) - 97 + shift)%26+97)
else:
decipher += char
return decipherSolution
As per PEP 8, standard indentation for Python is 4 spaces. In a language where whitespace is syntactically significant, this is an important convention to follow.
The naming of the parameter
Inside the loop, the three branches are mutually exclusive. I suggest writing them as
You have a lot of magic numbers in the code. The first branch could be more clearly written as
Iterating using
The naming of the parameter
code and the variable decipher are slightly off, especially with the latter being a verb. I suggest ciphertext and plaintext.Inside the loop, the three branches are mutually exclusive. I suggest writing them as
if … elif … else …. You used the += concatenation operator in the else branch; I wonder why you didn't do likewise in the other two cases.You have a lot of magic numbers in the code. The first branch could be more clearly written as
if 'A' <= char <= 'Z':
plaintext += chr((ord(char) - ord('A') + shift) % 26 + ord('A'))Iterating using
for char in code is good. Many Python beginners try to do something complicated, like i = 0; while i < len(code): char = code[i]. Even better would have been to use a list comprehension, so as to avoid creating another immutable string every time you append a character to the result. Here is one possible way to write it:def caesar(ciphertext, shift):
def decipher(alpha_start, alpha_size, c, shift):
if alpha_start <= ord(c) < alpha_start + alpha_size:
return chr((ord(c) - alpha_start + shift) % alpha_size + alpha_start)
return ''.join([decipher(ord('A'), 26, c, shift) or
decipher(ord('a'), 26, c, shift) or c
for c in ciphertext])Code Snippets
if 'A' <= char <= 'Z':
plaintext += chr((ord(char) - ord('A') + shift) % 26 + ord('A'))def caesar(ciphertext, shift):
def decipher(alpha_start, alpha_size, c, shift):
if alpha_start <= ord(c) < alpha_start + alpha_size:
return chr((ord(c) - alpha_start + shift) % alpha_size + alpha_start)
return ''.join([decipher(ord('A'), 26, c, shift) or
decipher(ord('a'), 26, c, shift) or c
for c in ciphertext])Context
StackExchange Code Review Q#68693, answer score: 5
Revisions (0)
No revisions yet.