CodeWars "Dbftbs Djqifs" kata code

Question

I have been working on the following kata from CodeWars. It's basically a Caesar Cipher — see the test cases below my solution code.

Any comments/feedback appreciated!

def encryptor(key, message):
    new = ""
    for letter in message:
        if letter.isalpha():
            id = ord(letter)
            shift = id+key 
            if 97 <= id <= 122: 
                while shift>122:
                    shift -= 26
                while shift<97:
                    shift += 26    
                letter = chr(shift)
            if 65 <= id <= 90: 
                while shift>90:
                    shift -= 26
                while shift<65:
                    shift += 26    
                letter = chr(shift)
        new = new + letter
    return new 

And here are test cases that give you an idea of what is being looked for:

test.assert_equals(encryptor(13, ''), '')
test.assert_equals(encryptor(13, 'Caesar Cipher'), 'Pnrfne Pvcure')
test.assert_equals(encryptor(-5, 'Hello World!'), 'Czggj Rjmgy!')
test.assert_equals(encryptor(27, 'Whoopi Goldberg'), 'Xippqj Hpmecfsh')

Answer 1

There is not too much to be said about the code as it is. A few (very minor) things I can point out as suggestions:

• Make if 97 <= id <= 122: followed by if 65 <= id <= 90: an if-elif clause. That makes it clearer the code path can only follow one of these (or even neither).

• Dedent the bottom letter = chr(shift) by one indentation and remove the other one; avoids duplication

• There is inconsistent white space around operators (sometimes one space each side, sometimes no space). I think PEP 8 recommends a single space on each side.

• Some people will argue that you shouldn't do new = ""; ... ; new = new + letter. But instead, something like letters = []; ...; letters.append(letter); ...; new = "".join(letters).
  That would gain you some speed. But only if your string becomes (very) long. For small strings, I think the current way (appending to a string) is clearer.
  Just so you know.

---

I can, however, suggest to use str.maketrans and str.translate. You'll need to set up the translation table in a smart way (the algorithm), but the actually encoding is then very clear and straightforward. Left as an exercise to the reader.

Which leads to the fun fact: rot13 is available as a standard encoding. In Python 2:

>>> 'Caesar Cipher'.encode('rot13')
'Pnrfne Pvcure'

In Python 3 is slight more effort:

>>> import codecs
>>> codecs.encode('Caesar Cipher', 'rot13')
'Pnrfne Pvcure'

Answer 2

I object mildly to these two lines:

for letter in message:
    if letter.isalpha():

You're calling the character a letter before you have ascertained that it is in fact a letter. Then, you test letter.isalpha(), but that test is somewhat redundant with the … <= id <= … tests further down.

There are too many magic numbers (97, 122, 65, 90). It would be nicer to see 'a', 'z', 'A', and 'Z', respectively.

The while loops could be replaced by % 26 calculations.

The majuscule and miniscule cases are basically copy-and-pasted code.

Code that follows this pattern…

result = ""
for thing in things:
    …
    result = result + blah

… is often better written using a generator expression.

Suggested solution

def encryptor(key, message):
    def shift(c, alpha_start):
        return chr((ord(c) - ord(alpha_start) + key) % 26 + ord(alpha_start))
    return ''.join(shift(c, 'A') if 'A' <= c <= 'Z' else 
                   shift(c, 'a') if 'a' <= c <= 'z' else c
                   for c in message)