Decode function on caesar cypher is weird when using modulo

Question

I'm currently making a caesar function and in the process of optimization, ran across a really weird problem when decrypting an encoded text. For example, encoding civilization with a shift value = 5 results in the output "hnanqnefynts", however upon decoding it results in "csvslszktsox", notice that the odd positions in the word decode correctly, while the even positions are instead shifted by 10, e.g: i -> s, a -> k, n -> x.

I believe the main bug from the code comes from using an if statement to make the shift value negative instead of a minus operator and how it works with the modulo operator, although I don't quite understand why only the even positions in the resulting word is affected.

def caesar(plain_text, shift_amount, cipher_direction):
    text_to_list = list(text)
    output = ''
    operation = 'encoded'
    for letter in text_to_list:
        letter_int = alphabet.index(letter)
        if cipher_direction == 'decode':
            shift_amount *= -1
            operation = 'decoded'
        processed_letter = alphabet[(letter_int + shift_amount) % len(alphabet)]
        output += processed_letter

If looking for optimization, see [str.maketrans](https://docs.python.org/3/library/stdtypes.html#str.maketrans) and [str.translate](https://docs.python.org/3/library/stdtypes.html#str.translate). [This answer](https://stackoverflow.com/a/72874879/235698) has an example. — Mark Tolonen, Jul 18 '22 at 23:35
mb for not clarifying myself, I was trying to figure out why the even positions in the decoded word would get shifted by 10, apparently it was a simple mistake of putting the shift amount inside the for loop. — tumenicooks, Jul 18 '22 at 23:39

score 1 · Accepted Answer · answered Jul 18 '22 at 23:28

Running this function gives NameError: name 'text' is not defined. I assume you meant text_to_list = list(plain_text).

As for your main problem, the odd/even issue, it's because you put shift_amount *= -1 inside your for loop. This causes shift_amount to change sign on alternating decode operations. You only need to check the encode/decode flag once, so move it outside the loop.

Also, you've got two variables whose value isn't used anywhere: output and operation. I assume that output is meant to be the return value of the function, so you need to actually return it. But operation is just a redundant near-copy of cipher_direction, so just get rid of it.

Here's a fixed version:

alphabet = 'abcdefghijklmnopqrstuvwxyz'

def caesar(plain_text, shift_amount, cipher_direction):
    text_to_list = list(plain_text)
    output = ''
    if cipher_direction == 'decode':
        shift_amount *= -1
    for letter in text_to_list:
        letter_int = alphabet.index(letter)
        processed_letter = alphabet[(letter_int + shift_amount) % len(alphabet)]
        output += processed_letter
    return output

Sample input and output:

>>> caesar('civilization', 10, 'encode')
'msfsvsjkdsyx'
>>> caesar('msfsvsjkdsyx', 10, 'decode')
'civilization'

Thank you for the succint explanation! As for the unused variables, I ommitted some parts of the code as I thought it was unnecessary but those two variables are used as part of print function, sorry for the misunderstanding! — tumenicooks, Jul 18 '22 at 23:34

Decode function on caesar cypher is weird when using modulo

1 Answers1