Caesar cipher exercise

Question

I was doing this exercise from here. I made a solution work but can't help feel it could have been done in fewer lines. I'm quite new to python so I'm not too sure of the little shortcuts.

def shift(S, n):
   word = ''
   for i in S:
      if ord(i) == 32:
         word += ' '
      elif ord(i)+n > 90:
         a = n-(90-ord(i))+64
         word += chr(a)
      else:
         word +=chr(ord(i)+n)
   return word
def TGS(x): # Total goodness of string
   TGC = 0 # Total goodness of character
   for i in x:
      if ord(i) == 32:
         continue
      TGC += letterGoodness[ord(i)-65]
   return TGC
A = input()
LOSS = [shift(A,n) for n in range (1,27)] # List of shifted strings
LOTG = [TGS(x) for x in LOSS]
a = LOTG.index(max(LOTG))
print(LOSS[a])

Answer 1

• Magic numbers.

  Try to avoid them. 65 is really ord('A'), 90 is ord('Z'); so say that explicitly. Along the same line,

  if i == ' ':
  

  looks better than

  if ord(i) == 32:
  

• Naming

  Try to use descriptive names. I'd prefer caesar_decrypt(ciphertext, shift) to shift(S, n). Same goes for A, a, etc.

• Streamlining

  I was really stumbled upon an asymmetry of elif and else cases of shift:

       a = n-(90-ord(i))+64
       word += chr(a)
  

  is quite counter-intuitive. I'd suggest

  for i in S:
    if ord(i) == 32:
        word += ' '
        continue
    a = ord(i) + n
    if a > ord('Z'):
        a -= ord('Z') - ord('A')
    word +=chr(a)
  

Answer 2

While you are by no means obligated to use it, here is a shorter version of the shift function.

With comments:

def shift(s,n):
    word=''
    for i in s:
        x=ord(i)
        y=x+n
        if 64<x<91:
            word+=chr(y) if y<91 else chr(65+y-90) 
        else:
            word+=' ' 
    return word

Explanation:

y is the ascii value plus the shift value.

64<x<91 checks if the character is a character we are looking for.

word+=chr(y) if y<91 else chr(65+y-90) The shifted character will be added if it does not exceed 'Z', otherwise will equal the wraparound value.

EDIT

If you would prefer not to use a conditional expression, you could alternatively do this:

def shift(s,n):
    word=''
    for i in s:
        x=ord(i)
        y=x+n
        if 64<x<91:
            if y<91:
                word+=chr(65+y-90)
            else:
                word+=chr(y) 
        else:
            word+=' ' 
    return word

Answer 3

Let's see.

1. I think s is a bit short. I would use a longer word.
2. It is more readable to compare chars than the numeric equivalent
3. You could take care of negative values of n by adding if n<0: n = n%26+26
4. You could ignore any non-alphabetic letters and preserve punctuation.
5. You could also shift lowercase letters in a separate test.
6. Modular arithmetic can be used to shift a value in a circular way. But you need to transform your letter into an integer in range 0..25. And back.

Result:

def shift(string,n):
    if n<0:
        n = n%26 + 26
    word = ''
    for c in string:
        if 'A' <= c <= 'Z':
            c = chr((ord(c)-ord('A')+n)%26 + ord('A'));
        if 'a' <= c <= 'z':
            c = chr((ord(c)-ord('a')+n)%26 + ord('a'));
        word += c
    return word

print shift("Hello, World!", 3)
print shift("Khoor, Zruog!", -3)