Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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])
share|improve this question
1  
Consider using str.translate‌​. –  Gareth Rees Aug 29 at 20:19

3 Answers 3

up vote 4 down vote accepted
  • 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)
    
share|improve this answer
    
thanks for the reply, i will make those amendments, is there any way I can improve the last bit? It seems long winded. –  Vimal Karsan Aug 29 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
share|improve this answer
    
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. –  Vimal Karsan Aug 29 at 20:36
    
@VimalKarsan While I think the first is less verbose, I gave you an alternative process. –  Batman Aug 29 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 –  Vimal Karsan Aug 29 at 21:24
1  
You can pass the ordering function to max as keyword key: max(LOSS, key=TGS). –  tcarobruce Aug 29 at 21:52
    
if y>90: works better. chr(65+y-91) also helps. –  Florian F Aug 29 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)
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.