Polyalphabetic cipher

Question

I am trying to write the simplest to undestand code possible, each function has documentation and examples and I tried to use the best style possible.

""" This programme implements a polyalphabetic cipher. """

import string


ALPHABET = string.ascii_lowercase
CHARACTERS_THAT_MUST_REMAIN_THE_SAME = string.digits + string.punctuation + string.whitespace

def cycle_get(lst,index):
    """
    If the list ends go back to the start.
    >>> cycle_get(["lorem","ipsum","dolor","sit"],8)
    "lorem"
    """
    new_index = index % len(lst)
    return(lst[new_index])

def cycle_increment_index(index,lst):
    """
    If at the end: go back to the start
    else: increment.
    >>> cycle_increment_index(0,["a","b","c"])
    1
    >>> cycle_increment_index(2,["a","b","c"])
    0
    """
    if index == len(lst) - 1:
        index = 0
    else:
        index += 1
    return(index)

def shift(letter,value):
    """
    Shifts a letter in the alphabet by the value,
    if the alphabet ends go back to the start.
    >>> shift('a',5)
    f
    >>> "".join([shift(i,20) for i in "hello"])
    'byffi'
    """
    current_letter_value = ALPHABET.find(letter)
    end_value = current_letter_value + value
    return(cycle_get(ALPHABET,end_value))

def convert_key_to_numbers(key):
    """
    Uses the alphabetic value of letters to convert a word
    to a list of numbers.
    >>> convert_key_to_numbers("abcde")
    [0,1,2,3,4]
    >>> convert_key_to_numbers("example")
    [4, 23, 0, 12, 15, 11, 4]
    """
    return([ALPHABET.find(i) for i in key])

def encrypt(text,key,reverse_operation=False):
    """
    Encrypts the text with a polyalphabetic cipher.
    >>> encrypt("lorem ipsum dolor sit amet, consectetur adipiscing elit","latine")
    'wokmz masnu qswok avx lmxb, psysxkgieuk iqmailkvrr eeqg'
    >>> encrypt("the quick brown fox jumps over the lazy dog","gvufigfwiufw")
    'zcy vcohg jltst aic rarla iaax obj tgeu lil'
    """
    text = text.lower()
    key = convert_key_to_numbers(key)
    index_of_key = 0
    result = ""
    for char in text:
        if char in CHARACTERS_THAT_MUST_REMAIN_THE_SAME:
            result += char
        else:
            if not reverse_operation:
                result += shift(char,key[index_of_key])
            else:
                result += shift(char,- key[index_of_key])
            index_of_key = cycle_increment_index(index_of_key,key)
    return(result)

def decrypt(text,key):
    """
    Decrypts the text previously encrypted with a polyalphabetic cipher.
    >>> decript('wokmz masnu qswok avx lmxb, psysxkgieuk iqmailkvrr eeqg',"latine")
    'lorem ipsum dolor sit amet, consectetur adipiscing elit'
    >>> decrypt("zcy vcohg jltst aic rarla iaax obj tgeu lil","gvufigfwiufw")
    'the quick brown fox jumps over the lazy dog'
    """
    return(encrypt(text,key,reverse_operation=True))

Answer 1

I am trying to write the simplest to understand code possible, each function has documentation and examples and I tried to use the best style possible.

The individual methods are simple and easy to understand. The docstrings are especially great, they are fantastic help for the reader.

To improve further, try to zoom out. Take a step back and look at the outline of the code, like some advanced editors fold the function bodies and show only the method signatures:

def cycle_get(lst,index): ...

def cycle_increment_index(index,lst): ...

def shift(letter,value): ...

def convert_key_to_numbers(key): ...

def encrypt(text,key,reverse_operation=False): ...

def decrypt(text,key): ...

I don't know if you see it, but things look less clear at this level:

• It's hard to guess what cycle_get(lst,index) and cycle_increment_index(index,lst) would do. Also, curiously, although both seem to take a list and a number parameter, the order of parameters is reversed. This will be difficult to remember for users of the class.

• shift(letter,value) could be clearer: the method shifts letter by some number, but value is too generic, so it doesn't help guessing that.

• convert_key_to_numbers(key) comes quite close, but it would help to be more specific and call "numbers" to "indexes" instead.

• encrypt(text,key,reverse_operation=False) is clear, but the reverse_operation parameter is a bit too long. But the biggest problem is that the parameter doesn't really make sense: what is a reverse operation of encrypt? The reverse of "encrypt" should be "decrypt", which shouldn't belong in this method

• decrypt(text,key) is clear :-)

I would recommend this alternative outline:

def get_circular_index(lst, index):    
    return index % len(lst)

def get_circular_item(lst, index):
    return lst[get_circular_index(lst, index)]    

def get_next_index(lst, index):
    return get_circular_index(lst, index + 1)    

def shift_letter_by(letter, num):
    # ...    

def convert_key_to_indexes(key):
    return [ALPHABET.find(i) for i in key]

def cycle_text(text, key, reverse=False):
    # ...    

def encrypt(text, key):
    return cycle_text(text, key)    

def decrypt(text, key):
    return cycle_text(text, key, reverse=True)

For the shorter ones I included the implementation too. I made some other changes too:

• The original cycle_get and cycle_increment_index were sharing the "index cycling logic". I moved that part of the logic in one place, a new get_circular_index function.

• encrypt is just as clear now as decrypt, and these methods shouldn't call each other, but the common purpose method cycle_text.

• As suggested above, the methods and method parameters are renamed to what I hope would be more natural, and the method parameters are consistently ordered.

---

The CHARACTERS_THAT_MUST_REMAIN_THE_SAME variable is long. I tried to find a better name for it, but it was too hard. When it's too hard to name something, it's often the sign that there's a better way. In this case, is this variable really necessary? It seems the intention is to put characters in it that you don't want to encrypt / decrypt. Its purpose seems sort of the opposite of the ALPHABET variable. But it's not really the opposite of that. And it could be, and it would be simpler. So instead of the condition if char in CHARACTERS_THAT_MUST_REMAIN_THE_SAME: ..., how about using if char not in ALPHABET: ... ?

With this in mind, I would recommend this implementation for cycle_text:

def cycle_text(text, key, reverse=False):
    text = text.lower()
    indexes = convert_key_to_indexes(key)
    index_of_key = 0
    result = ""
    for char in text:
        if char not in ALPHABET:
            result += char
        else:
            if not reverse:
                result += shift_letter_by(char, indexes[index_of_key])
            else:
                result += shift_letter_by(char, - indexes[index_of_key])
            index_of_key = get_next_index(indexes, index_of_key)
    return result

Another improvement I slipped in here is not reassigning to the key parameter inside the method body, as it was in the original code.

---

Why all the redundant parentheses in the return statements?

return(lst[new_index])
return(index)
# ...

All these could be simply like this without parentheses:

return lst[new_index]
return index
# ...

---

You should follow PEP8, the official coding style guide of Python.

For example, instead of this:

return encrypt(text,key,reverse_operation=True)

You should put a space after every comma separating a list of parameters, like this:

return encrypt(text, key, reverse_operation=True)

And put 2 empty lines before function declarations (before every def). This rule is only for functions in the global namespace, for functions inside classes 1 empty line is enough.