# Caesar cipher exercise

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

-
Consider using str.translate‌​. – Gareth Rees Aug 29 '14 at 20:19

## 3 Answers

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

-
thanks for the reply, i will make those amendments, is there any way I can improve the last bit? It seems long winded. – MathsIsHard Aug 29 '14 at 20:05

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

-
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. – MathsIsHard Aug 29 '14 at 20:36
@VimalKarsan While I think the first is less verbose, I gave you an alternative process. – Batman Aug 29 '14 at 21:21
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 – MathsIsHard Aug 29 '14 at 21:24
You can pass the ordering function to max as keyword key: max(LOSS, key=TGS). – tcarobruce Aug 29 '14 at 21:52
if y>90: works better. chr(65+y-91) also helps. – Florian F Aug 29 '14 at 22:17

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)

-