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])

• Gareth Rees: Consider using str.translate.

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)
  

• MathsIsHard: thanks for the reply, i will make those amendments, is there any way I can improve the last bit? It seems long winded.

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

• MathsIsHard: Hey, thanks for the review, do you think there's a simpler way of doing the last bit? Creating two lists seems a bit long winded.
• RageCage: @VimalKarsan While I think the first is less verbose, I gave you an alternative process.
• MathsIsHard: thank you but I was referring to the very last bit with the LOSS and LOTG where I'm finding the string with the highest 'goodness'. I know you can implement the max function with a key (using methods), is there a similar way to use max with a function key? Many thanks
• tcarobruce: You can pass the ordering function to max as keyword key: max(LOSS, key=TGS).
• Florian F: if y>90: works better. chr(65+y-91) also helps.

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)