HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Caesar decipher

Submitted by: @import:stackexchange-codereview··
0
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?

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 decipher

Solution

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 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.