I have just created an encryption program with a friend. I wanted to know if anyone here could give me back any feedback on it.
Here is how the program works:
Imagine you want to encrypt the word 'hi'.
Depending on the key generated with the program, the computer will choose between 2 strings of two letters randomly for every letter that needs to be encrypted. The strings are embedded in an encryption/decryption key.
Example: 'h' when encrypted will be either 'tn' or 'io', and 'i' when encrypted will be either 'ac' or 'vu'.
So when 'hi' will be encrypted, it can be:
'tnac'; 'tnvu'; 'ioac'; 'iovu'
It is chosen randomly by the computer, so it is impossible to predict what outcome will be.
#######################################################################
# #
# AGENCRYPTION program V.1.1 #
# Made by Elio Hayman and Valentino Magniette #
# Have fun #
# #
#######################################################################
version = 'Version 1.1'
from random import *
import tkinter as tk
current_key = ''
account = ''
in_debug = False
def debug(text):
if in_debug:
print('[Debug]', text)
key = ''
def decrypt(crypted):
debug(crypted)
global current_key
key = current_key
if key == '':
print('[Error] Please insert a key before utilisation. To do so, do readkey <yourKey>.')
return
basemsg = ''
key_list = []
while not key == '':
key_list.append(key[:5])
key = key[5:]
new_key = []
for i in key_list:
new_key_thing = []
new_key_thing.append(i[0])
new_key_thing.append(i[1:3])
new_key_thing.append(i[3:])
new_key.append(new_key_thing)
key = new_key
searchlength = 1
msgleft = crypted
found = False
while len(msgleft) > 0:
searchlength = 0
found = False
while found == False:
if searchlength == 0 and len(msgleft) > 0:
for x in key:
if found == False:
if msgleft[0] in x[1:]:
basemsg = basemsg + x[0]
msgleft = msgleft[1:]
found = True
elif searchlength > 0 and len(msgleft) > 0:
for x in key:
if found == False:
if msgleft[:searchlength-1] in x[1:]:
basemsg = basemsg + x[0]
msgleft = msgleft[searchlength-1:]
found = True
searchlength += 1
basemsg = basemsg.replace('^', ' ')
return basemsg
def encrypt(message):
global current_key
key = current_key
if key == '':
print('[Error] Please insert a key before utilisation. To do so, do readkey <yourKey>.')
return
message = message.replace(' ', '^')
endmsg = ''
key_list = []
while not key == '':
key_list.append(key[:5])
key = key[5:]
new_key = []
for i in key_list:
new_key_thing = []
new_key_thing.append(i[0])
new_key_thing.append(i[1:3])
new_key_thing.append(i[3:])
new_key.append(new_key_thing)
for char in message:
for x in new_key:
if x[0] == char:
endmsg = endmsg + x[randint(1, len(x)-1)]
break
return endmsg
def readkey(input_key):
all_chars = 'aabcdefghijklmnopqrstuvwxyz1234567890&2é~"#\'{([-|è`_\\çà@)]=}°+.+-*/,?;:!§ù%*μ$£¤^ ̈ABCDEFGHIJKLMNOPQRSTUVWXYZ'
key_list = []
if len(input_key) == 779:
print('Key loading...')
parts = list()
for i in range(109):
key_part = input_key[:5]
letter = key_part[0]
part1 = key_part[1:3]
part2 = key_part[3:5]
list_thing = list()
list_thing.append(letter)
list_thing.append(part1)
list_thing.append(part2)
parts.append(input_key[:5])
input_key = input_key[5:]
print('Key inserted')
print('Caution, this key will only stay in the program as long as it is running or when replaced by another one')
else:
print('[Error] This key is unsupported by the program. Make sure it is correct and that you are using the latest version.')
return None
return ''.join(parts)
def genkey():
all_chars = 'aabcdefghijklmnopqrstuvwxyz1234567890&2é~"#\'{([-|è`_\\ç^à@)]=}°+.+-*/,?;:!§ù%*μ$£¤^ ̈ABCDEFGHIJKLMNOPQRSTUVWXYZ'
char_list = list('aabcdefghijklmnopqrstuvwxyz1234567890&2é~"#\'{([-|è`_\\ç^à@)]=}°+.+-*/,?;:!§ù%*μ$£¤^ ̈ABCDEFGHIJKLMNOPQRSTUVWXYZ')
shuffle(char_list)
all_chars = ''.join(char_list)
key = list()
security = 2
for x in range(len(all_chars)*security):
valid = False
while valid == False:
char1 = all_chars[randint(0,35)]
char2 = all_chars[randint(0,35)]
if not (char1 + char2) in key:
key.append(char1 + char2)
valid = True
speshul_chars_letters = list()
for i in range(117):
valid = False
while valid == False:
char1 = all_chars[randint(0,35)]
char2 = all_chars[randint(0,35)]
if not (char1 + char2) in key or (char1 + char2) in speshul_chars_letters:
speshul_chars_letters.append(char1 + char2)
valid = True
key_list = []
for i in all_chars:
chars = [i]
for xx in range(security):
chars.append(key[0])
del key[0]
key_list.append(chars)
key_text = ''
key_text_list = []
for y in key_list:
key_text_list.append(''.join(y))
speshul_chars_letters_text = ''.join(speshul_chars_letters)
return ''.join(key_text_list) + speshul_chars_letters_text
import tkinter as tk
from time import *
mw = tk.Tk()
mw.title('AGE v1.1')
mw.geometry("800x500")
mw.resizable(0, 0)
back = tk.Frame(master=mw,bg='#24b1db')
back.pack_propagate(0)
back.pack(fill=tk.BOTH, expand=1)
mode = 0
def purgescreen():
if mode == 1:
encryption_text_input.destroy()
encryption_text_output.destroy()
encrypt_main.destroy()
elif mode == 2:
decryption_text_input.destroy()
decryption_text_output.destroy()
decrypt_main.destroy()
elif mode == 3:
keygen_text_output.destroy()
keygen_main.destroy()
directapply_main.destroy()
elif mode == 4:
keyread_text_input.destroy()
keyread_main.destroy()
elif mode == 5:
info_main.destroy()
info_copyright.destroy()
info_website.destroy()
info_terms.destroy()
info_version.destroy()
elif mode == 6:
help_main.destroy()
elif mode == 7:
welcome_main.destroy()
def encrypt_shortcut():
message = encryption_text_input.get("1.0",'end-1c')
encrypted = encrypt(message)
encryption_text_output.delete("1.0","end")
encryption_text_input.delete("1.0","end")
encryption_text_output.insert("1.0",encrypted)
def encryption_button():
global encryption_text_input
global encryption_text_output
global encrypt_main
global mode
purgescreen()
mode = 1
encryption_text_input = tk.Text(back, height=10, width=70)
encryption_text_input.place(relx=.25 , rely=0)
encryption_text_output = tk.Text(back, height=10, width=70)
encryption_text_output.place(relx=.25, rely=.6)
encrypt_main = tk.Button(master=back, text='Encrypt', command=encrypt_shortcut)
encrypt_main.config(height=3, width=15)
encrypt_main.place(relx=.5, rely=.4)
def decrypt_shortcut():
message = decryption_text_input.get("1.0",'end-1c')
decrypted = decrypt(message)
decryption_text_output.delete("1.0","end")
decryption_text_input.delete("1.0","end")
decryption_text_output.insert("1.0",decrypted)
def decryption_button():
global decryption_text_input
global decryption_text_output
global decrypt_main
global mode
purgescreen()
mode = 2
decryption_text_input = tk.Text(back, height=10, width=70)
decryption_text_input.place(relx=.25 , rely=0)
decryption_text_output = tk.Text(back, height=10, width=70)
decryption_text_output.place(relx=.25, rely=.6)
decrypt_main = tk.Button(master=back, text='Decrypt', command=decrypt_shortcut)
decrypt_main.config(height=3, width=15)
decrypt_main.place(relx=.5, rely=.4)
def keygen_shortcut():
key = genkey()
key = ''.join(key)
keygen_text_output.delete("1.0",'end')
keygen_text_output.insert("1.0",key)
def apply_shortcut():
key = keygen_text_output.get("1.0","end-1c")
global current_key
current_key = key
def keygen_button():
global keygen_text_output
global keygen_main
global mode
global directapply_main
purgescreen()
mode = 3
keygen_text_output = tk.Text(back, height=15, width=70)
keygen_text_output.place(relx=.25 , rely=.4)
keygen_main = tk.Button(master=back, text='Generate Key', command=keygen_shortcut)
keygen_main.config(height=3, width=15)
keygen_main.place(relx=.4, rely=.2)
directapply_main = tk.Button(master=back, text='Apply Key', command=apply_shortcut)
directapply_main.config(height=3, width=15)
directapply_main.place(relx=.6, rely=.2)
def keyread_shortcut():
key = keyread_text_input.get("1.0","end-1c")
keyread_text_input.delete("1.0","end")
global current_key
current_key = key
def keyread_button():
global keyread_text_input
global keyread_main
global mode
purgescreen()
mode = 4
keyread_text_input = tk.Text(back, height=15, width=70)
keyread_text_input.place(relx=.25, rely=.1)
keyread_main = tk.Button(master=back, text='Insert Key', command=keyread_shortcut)
keyread_main.config(height=3, width=15)
keyread_main.place(relx=.5, rely=.7)
def info_button():
global info_main
global info_copyright
global info_website
global info_version
global info_terms
global mode
purgescreen()
mode = 5
info_main = tk.Label(master=back, text='This program is made by \n Valentino Magniette and iWaroz.', bg='#24b1db')
info_main.config(height=3, width=70)
info_main.place(relx=.25, rely=.1)
info_copyright = tk.Label(master=back, text='Copying this program or resseling it \n without permission from its creators is forbidden.', bg='#24b1db')
info_copyright.config(height=3, width=70)
info_copyright.place(relx=.25, rely=.3)
info_terms = tk.Label(master=back, text='AGencryption and its creators are not responsible for any legal problems regarding \n encrypting sensible documentation that the authorities want decrypted. ', bg='#24b1db')
info_terms.config(height=3, width=70)
info_terms.place(relx=.25, rely=.5)
info_website = tk.Label(master=back, text='You can get the program for free at: http://realtasting.com/AGE-Version1.exe', bg='#24b1db')
info_website.config(height=3, width=70)
info_website.place(relx=.25, rely=.7)
info_version = tk.Label(master=back, text='Version 1.1', bg='#24b1db')
info_version.config(height=3, width=70)
info_version.place(relx=.25, rely=.9)
def help_button():
global help_main
global mode
purgescreen()
mode = 6
help_main = tk.Label(master=back, text='If any help is needed. \n Go to our discord with the link \n https://discord.gg/YVDBudA', bg='#24b1db')
help_main.config(height=3, width=50)
help_main.place(relx=.35, rely=.5)
global welcome_main
purgescreen()
mode = 7
welcome_main = tk.Label(master=back, text='Welcome to AGE \n This is a program which is used for encryption \n You can encrypt an unlimited ammount of text securely for free \n To start, you will need an encryption/decryption key for it to work \n Have fun!!', bg='#24b1db')
welcome_main.config(height=10, width=50)
welcome_main.place(relx=.35, rely=.35)
encryption_main = tk.Button(master=back, text='Encryption', command=encryption_button)
encryption_main.config(height=4, width=20)
encryption_main.place(relx=.095, rely=.07, anchor="c")
decryption_main = tk.Button(master=back, text='Decryption', command=decryption_button)
decryption_main.config(height=4, width=20)
decryption_main.place(relx=.095, rely=.21, anchor="c")
generator_main = tk.Button(master=back, text='Key Generator', command=keygen_button)
generator_main.config(height=4, width=20)
generator_main.place(relx=.095, rely=.35, anchor="c")
reader_main = tk.Button(master=back, text='Key reader', command=keyread_button)
reader_main.config(height=4, width=20)
reader_main.place(relx=.095, rely=.49, anchor="c")
information_main = tk.Button(master=back, text='Information', command=info_button)
information_main.config(height=4, width=20)
information_main.place(relx=.095, rely=.63, anchor="c")
help_main = tk.Button(master=back, text='Help', command=help_button)
help_main.config(height=4, width=20)
help_main.place(relx=.095, rely=.77, anchor="c")
quit_main = tk.Button(master=back, text='Quit', command=mw.destroy)
quit_main.config(height=4, width=20)
quit_main.place(relx=.095, rely=.91, anchor="c")
mw.mainloop()
```
2 Answers 2
First, congratulations! Being able to produce something usable with another person is an incredibly important skill, and especially in programming. Please don't worry about the amount of suggestions below – I'm treating this like any other Python review.
Specific suggestions
Module documentation, like the lines at the top of your file, is usually written within triple double quotes, like this:
""" AGENCRYPTION [...] """
You can also use this to write multi-line strings without having to embed
\n
escape characters within them.- It would be handy to be able to set the debugging flag from the command line rather than having to modify the code. You can do that by using either
argparse
to add a--debug
flag or by checking whetheros.environ["DEBUG"]
is not empty. from [something] import *
is a handy shortcut, but it makes the code harder to read. Consider when you're looking at a line containing a call torandint
somewhere. You might want to see howrandint
is implemented, to understand whether it's doing what you expect. To do so in a simple text editor you would have to first search for the string "randint", and once you confirm that it's not defined locally you'd have to look through each of the*
imports to see where it's defined. If youfrom random import randint
instead it's really easy to find.- Using
global
can also be handy, but it makes the interactions between the various parts of your code much harder to understand. The standard fix for this is to simply remove a single mention of theglobal
keyword and instead pass parameters around until each piece of code gets exactly what it needs from the caller. You can then repeat this until there are no more globals. - The string consisting of all valid key characters is repeated a few times. If you pull it out into a constant such as
KEY_CHARACTERS = "[...]"
at the top of your file you can just refer to that in the rest of your code. - This program mixes GUI (TK) code with encryption code. It would be good to split the GUI code into its own file, and importing the relevant functions there. This makes it possible to understand and use the encryption code on its own, for example if you want to create a GUI with a different toolkit.
If you try to import your file in another file Python will actually run the code. This which means the code isn't actually reusable. The way to work around this is to create a
main
function in your file and call it only when your script is run from the shell:if __name__ == "__main__": main()
Your
main
function actually does the work of instantiating and running the GUI, which means the various functions can be imported without side effects."".join(key)
should be unnecessary, becausegenkey
already returns a string.- The various modes should probably be an
enum
with values likeENCRYPTION
andDECRYPTION
- that way the "magic numbers" assigned tomode
are instead human readable variable references such asmodes.ENCRYPTION
. - You probably already know this, but I need to warn about it just in case: you should not use this code to transfer actual secrets. Cryptography is really, really difficult, and there are existing systems like PGP (available from tools like
gnupg
) to do this. If you want to know how they work there is a lot of advanced maths and difficult code involved.
Tool support suggestions
black
can automatically format your code to be more idiomatic. It'll do things like adjusting the vertical and horizontal spacing, while keeping the functionality of the code unchanged.isort
can group imports (built-ins first, then libraries and finally imports from within your project) and sort them.flake8
can give you more hints to write idiomatic Python:[flake8] max-complexity = 4 ignore = W503,E203
I would then recommend validating your type hints using a strict
mypy
configuration:[mypy] check_untyped_defs = true disallow_any_generics = true disallow_untyped_defs = true ignore_missing_imports = true no_implicit_optional = true warn_redundant_casts = true warn_return_any = true warn_unused_ignores = true
This ensures that anyone reading the code (including yourself) understand how it's meant to be called, which is very powerful in terms of modifying and reusing it. For example, it looks like the
genkey
signature should bedef genkey() -> str:
to show that it returns a string, anddebug
's signature should bedef debug(text: str) -> None:
to show that it takes a string (as opposed to for examplebytes
orStringIO
) and does not return anything.
General suggestions
- Using a free IDE like PyCharm Community or Visual Studio Code is incredibly helpful to write non-trivial code. I can't possibly go into detail here, but if you explore an IDE you'll find literally hundreds of helpful features, such as highlighting unused variables, automatically pulling out variables and inlining them, and spell checking.
-
1\$\begingroup\$ "The standard fix for this is to simply remove a single mention of the global keyword and instead pass parameters around until each piece of code gets exactly what it needs from the caller". I'd say the given code is a great example of how classes can vastly simplify the code and make it more structured and avoid the use of global.. having a dozen parameters on a function isn't particularly great either. \$\endgroup\$Voo– Voo2019年11月10日 12:37:27 +00:00Commented Nov 10, 2019 at 12:37
-
1\$\begingroup\$ @Voo Good point. I avoided mention of OOP here because I thought it would be appropriate for a follow-up review once OP gets rid of all the globals. Moving directly from globals to OOP is two steps in one go IMO, and that's often harmful in terms of coming up with a good structure. \$\endgroup\$l0b0– l0b02019年11月10日 18:59:54 +00:00Commented Nov 10, 2019 at 18:59
In addition to the great answer already provided by l0b0, a few comments.
1. code structure
Some blocks could be extracted to functions. For instance, the following block inside encrypt
:
new_key_thing = []
new_key_thing.append(i[0])
new_key_thing.append(i[1:3])
new_key_thing.append(i[3:])
new_key.append(new_key_thing)
Could be refactored to:
new_key.append(do_transformation(i))
Re-iterating on the new code, you could use Python's map
operation, which is very handy, or you could use Python's cool feature of list comprehension. Here's an example of refactoring (note that I also renamed from i
, which is more used to be an index number to k
, which indicates it's a key):
newkey = [do_transformation(k) for k in key_list]
2. Magic numbers
In some places the code includes magic numbers like 3, 5 and more. Instead of using those numbers as are, try to give them meaningful names.
For instance, dealing with magic number 5 (I hope I got the intention correctly, but even if that's not the case, you can see what the general idea is):
CHUNK_SIZE = 4
while not key == '':
key_list.append(key[:(CHUNK_SIZE + 1)])
key = key[(CHUNK_SIZE + 1):]
Here is how the program works
describing this is not half bad. If you added the purpose of this code (may be as mundane as completing a practice programming project), it may be easier to give advice (such as putting information in the source code makes it less likely that code snippets float around lacking that information). \$\endgroup\$