1
\$\begingroup\$

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')
\$\endgroup\$
0
4
\$\begingroup\$

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'
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I suspect that it wouldn't be worthwhile to use maketrans and translate unless the string gets pretty big, or unless all of the tables are made once initially and then forgotten about. I think appending to a string is at best marginally clearer, and although the CPython peephole optimizer handles self-appending well, other Python implementations don't necessarily. PEP8 recommends using ''.join in all situations. You also wouldn't want to do letters = [];... letters.append(char) for similar reasons - a comprehension would be better. \$\endgroup\$ – Dan Oberlam Jun 14 '16 at 13:58
4
\$\begingroup\$

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)
\$\endgroup\$
2
  • \$\begingroup\$ Good points, but as for the solution, this is getting dangerously close to participation in an obfuscated Python context (and that's not easy): there's a local function definition with a one-liner with plenty of parentheses, and the actual function is then also a one-liner, with a double (nested) ternary if statement. While perhaps faster, and in a few ways more Pythonic (generator, ''.join), I need to stare really hard to follow the code logic behind it (thus making it more bug-prone imo). \$\endgroup\$ – user86624 Jun 14 '16 at 4:33
  • 1
    \$\begingroup\$ @Evert I find this version way more readable than a bunch of explicit loops and conditions. The ''.join tells me right away that we are concatenating stuff and the generator expression is pretty clear about where it comes from and how it is transformed. Do not forget that python comes with batteries included so we can write short (understand less boilerplate, not code golf) code that often reads close to plain english. And if they don't exist, you write them. The only thing that I find debatable here is the inner def which trade readability for speed (local symbols are faster to resolve). \$\endgroup\$ – 301_Moved_Permanently Jun 14 '16 at 6:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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