patternpythonMinor
Polyalphabetic cipher
Viewed 0 times
polyalphabeticcipherstackoverflow
Problem
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_numbe
```
""" 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_numbe
Solution
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:
I don't know if you see it, but things look less clear at this level:
-
It's hard to guess what
-
-
-
-
I would recommend this alternative outline:
For the shorter ones I included the implementation too.
I made some other changes too:
-
The original
-
-
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
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
But it's not really the opposite of that.
And it could be, and it would be simpler.
So instead of the condition
how about using
With this in mind, I would recommend this implementation for
Another improvement I slipped in here is not reassigning to the
Why all the redundant parentheses in the
All these could be simply like this without parentheses:
You should follow PEP8, the official coding style guide of Python.
For example, instead of this:
You should put a space after every comma separating a list of parameters, like this:
And put 2 empty lines before function declarations (before every
This rule is only for functions in the global namespace,
for functions inside classes 1 empty line is enough.
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 resultAnother 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.
Code Snippets
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): ...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)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 resultreturn(lst[new_index])
return(index)
# ...return lst[new_index]
return index
# ...Context
StackExchange Code Review Q#66570, answer score: 4
Revisions (0)
No revisions yet.