Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

This function produces a decoded string, decoded, for all keyspaces against a given cæsar cipher ciphertext. Punctuation is not changed and string.upper() is used to make working with ord() easier.

I am looking for a more Pythonic way to work with my decoded variable. I keep finding myself assigning empty strings or None or 0 to use as a placeholder and I think it looks ugly. I'd also like to know if there is a better way to avoid writing c = ord(c).

def testCaesar(ciphertext):
    for testkey in range(0,25):
        decoded = ''   # <--- uglyness
        for c in ciphertext.upper():
            if not c.isalpha():
                decoded += c
                continue
            c = ord(c) # this is here to improve readability of line below. Is there a better way?
            decoded += chr(c + testkey if c + testkey < 90 else c - testkey)
        print('{0} : Rot{1} Caesar').format(decoded, str(testkey))
        checkMatch(decoded, testkey, 'Caesar')
share|improve this question
up vote 3 down vote accepted

To be honest I see no problem with using decoded = ''. However I do have a good way to improve the readability of your character decoding. Replace this:

    c = ord(c) # this is here to improve readability of line below. Is there a better way?
    decoded += chr(c + testkey if c + testkey < 90 else c - testkey)

with this:

    decoded += decode(testkey, c)

Now all you need is a decode function, which can do the actual character decoding. You can give decode a good docstring and that'll make it a lot easier to follow what's happening, especially as you could remove the ternary there:

def decode(key, character):
    position = ord(character)
    if position + key < 90:
        position += key
    else:
        position -= key

    return chr(position)

Of course, one other thing you could add is your isalpha test directly in there by starting with:

    if not character.isalpha():
        return character

Now that would change your original loop to this:

decoded = ''
for c in ciphertext.upper():
    decoded += decode(testkey, c)

Well now we can remove your initial empty string. We can just use str.join with a generator expression. Generators are like for loops collapsed into a single expression, and you can make a simple one in this case:

decoded = ''.join(decode(testkey, c) for c in ciphertext.upper())

This makes a single string from each value that decode returns for every character in ciphertext. This would refactor your code like this:

def decode(key, character):
    if not character.isalpha():
        return character
    position = ord(character)
    if position + key < 90:
        position += key
    else:
        position -= key

    return chr(position)

def testCaesar(ciphertext):
    for testkey in range(0,25):
        decoded = ''.join(decode(testkey, c) for c in ciphertext.upper())
        print('{0} : Rot{1} Caesar').format(decoded, str(testkey))
        checkMatch(decoded, testkey, 'Caesar')
share|improve this answer
    
Using a generator looks a lot better to me. Thank you. – cremefraiche Feb 1 at 11:16

You could use else instead of continue to mark the 2 potential results of your test. You get something like :

def testCaesar(ciphertext):
    for testkey in range(0,25):
        decoded = ''   # <--- uglyness
        for c in ciphertext.upper():
            if not c.isalpha():
                decoded += c
            else:
                c = ord(c) # this is here to improve readability of line below. Is there a better way?
                decoded += chr(c + testkey if c + testkey < 90 else c - testkey)
        print('{0} : Rot{1} Caesar').format(decoded, str(testkey))
        checkMatch(decoded, testkey, 'Caesar')

Now, you've got some duplicated logic you could try to remove. You could use a new variable like :

def testCaesar(ciphertext):
    for testkey in range(0,25):
        decoded = ''   # <--- uglyness
        for c in ciphertext.upper():
            if not c.isalpha():
                decoded_c = c
            else:
                c = ord(c) # this is here to improve readability of line below. Is there a better way?
                decoded_c = chr(c + testkey if c + testkey < 90 else c - testkey)
            decoded += decoded_c
        print('{0} : Rot{1} Caesar').format(decoded, str(testkey))
        checkMatch(decoded, testkey, 'Caesar')

Or you could define a function for this :

def decode_char(c, key):
    if not c.isalpha():
        return c
    else:
        c = ord(c) # this is here to improve readability of line below. Is there a better way?
        return chr(c + testkey if c + testkey < 90 else c - testkey)


def testCaesar(ciphertext):
    for testkey in range(0,25):
        decoded = ''   # <--- uglyness
        for c in ciphertext.upper():
            decoded += decode_char(c, testkey)
        print('{0} : Rot{1} Caesar').format(decoded, str(testkey))
        checkMatch(decoded, testkey, 'Caesar')

Now, Python has a style guide called PEP8 and it says :

Do not rely on CPython's efficient implementation of in-place string concatenation for statements in the form a += b or a = a + b . This optimization is fragile even in CPython (it only works for some types) and isn't present at all in implementations that don't use refcounting. In performance sensitive parts of the library, the ''.join() form should be used instead. This will ensure that concatenation occurs in linear time across various implementations.

This can be easily done now :

decoded = ''.join(decode_char(c, testkey) for c in ciphertext.upper())  

Also, a tiny optimisation could be to call upper() once and only once.

share|improve this answer

Format

There is one line that bothers me a lot:

print('{0} : Rot{1} Caesar').format(decoded, str(testkey))

First of, you don't need str when dealing with format. This is the whole point of using format: you don't have to deal with converting stuff into strings first, you let format handle it for you.

Second, the call is very weird. You usually call format directly on the string, not on enclosing parenthesis. This is even more confusing as print was changed from a keyword to a function in Python 3. So the meaning of this line is changed between Python 2 and Python 3.

In Python 2, format is applied on the string.

In Python 3, format is applied on the returned value of the print function. Meaning you try to call None.format(...) which raise an AttributeError.

Ideally, you should write this line as:

print('{0} : Rot{1} Caesar'.format(decoded, testkey))
share|improve this answer
    
Thank you for pointing these out, both late-night SNAFUs that I'm not experienced enough to have jump out at me. – cremefraiche Feb 1 at 11:07
decoded = ''   # <--- ugliness
for c in ciphertext.upper():
    if not c.isalpha():
        decoded += c
        continue
    c = ord(c)  # Is there a better way?
    decoded += chr(c + testkey if c + testkey < 90 else c - testkey)

Yes, there are better ways. They come from functional programming.

Try to adhere to the idea of "one concept, one name" as much as possible. For example, on the penultimate line above, you use the name c to mean both "the ciphertext character" and "the ord of the ciphertext character". Don't do that. Instead, use a new name for the new concept "ord of the ciphertext character"; for example, ord_c.

    ord_c = ord(c)
    decoded += chr(ord_c + testkey if ord_c + testkey < 90 else ord_c - testkey)

In the above code, ord_c = ord(c) is basically a definition of what ord_c means in the code. You could go replace every instance of ord_c in the program with ord(c), and the program would still work fine. This is a nice property for a program to have; it's known as referential transparency. A perfect program won't have any places where you say "Hey, you know that variable foo from earlier? Well, now it means something totally different." If the variable means something different, you should give it a different name.

Next, decompose your program into functions. For example, take that messy expression you're trying to get rid of, and stick it in a function:

def rotate(c, key):
    ord_c = ord(c)
    if ord_c + key < 90:
        return chr(ord_c + key)
    else:
        return chr(ord_c - key)

decoded = ''   # <--- ugliness
for c in ciphertext.upper():
    if not c.isalpha():
        decoded += c
        continue
    decoded += rotate(c, testkey)  # less ugliness

(Incidentally, you probably meant chr(ord_c + key - 26), not chr(ord_c - key).)

Next, use structured programming to get rid of the goto-style jumps in your code. Yes, continue and break count as jumps.

decoded = ''   # <--- ugliness
for c in ciphertext.upper():
    if c.isalpha():
        decoded += rotate(c, testkey)
    else:
        decoded += c

Finally, the code is clean enough to notice that what we're really doing here is transforming a string character-by-character: for each character in the input ciphertext, we produce a corresponding character in the output decoded. So we can use a comprehension to get the same result directly, with no intermediate state:

def decode(c, key):
    if not c.isalpha():
        return c
    elif ord(c) + key < 90:
        return chr(ord(c) + key)
    else:
        return chr(ord(c) - key)

decoded = ''.join(decode(c, testkey) for c in ciphertext.upper())
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.