Simple encryption/decryption program using python (and tkinter)

Question

I am creating a simple program with a GUI to encrypt/decrypt text using different ciphers. I tried to make it so that it would be easy to add new ciphers.

This is my code:

import tkinter as tk


ALPHABET = "abcdefghijklmnopqrstuvwxyz"


class Cipher:
    # encode_fun and decode_fun should take in args (key, message).
    def __init__(self, encode_fun, decode_fun):
        self.encode_fun = encode_fun
        self.decode_fun = decode_fun


        self.output = None

    def encode(self, key, message):
        try:
            self.output.delete(0, "end")
            self.output.insert(0, self.encode_fun(key, message))

        except:
            return

    def decode(self, key, message):
        try:
            self.output.delete(0, "end")
            self.output.insert(0, self.decode_fun(key, message))

        except:
            return

    def display(self, master):
        frame = tk.Frame(master)
        frame.pack()


        encode_button = tk.Button(frame, text="Encode")
        encode_button.pack(side="left")

        decode_button = tk.Button(frame, text="Decode")
        decode_button.pack(side="right")

        tk.Label(frame, text="Key:").pack()
        key_text_box = tk.Entry(frame)
        key_text_box.pack()

        tk.Label(frame, text="Text:").pack()
        message_text_box = tk.Entry(frame)
        message_text_box.pack()

        self.output = tk.Entry(frame, background="gray", foreground="white")
        self.output.pack()

        encode_button.config(command=
                lambda: self.encode(key_text_box.get(), message_text_box.get()))
        decode_button.config(command=
                lambda: self.decode(key_text_box.get(), message_text_box.get()))


def caesar_shift_encode(key, message):
    encoded_message = ""
    for char in message.lower():
        try:
            encoded_message += ALPHABET[(ALPHABET.index(char) + int(key)) % len(ALPHABET)]

        except:
            encoded_message += char

    return encoded_message


def caesar_shift_decode(key, message):
    return caesar_shift_encode(str(-int(key)), message)


def xor_cipher_encode(key, message):
    return "".join(
        [ALPHABET[ALPHABET.index(key_char) ^ ALPHABET.index(message_char)]
            for key_char, message_char in zip(key, message)]
    )


root = tk.Tk()

caesar_shift = Cipher(caesar_shift_encode, caesar_shift_decode)
caesar_shift.display(root)

xor_cipher = Cipher(xor_cipher_encode, xor_cipher_encode)
xor_cipher.display(root)

tk.mainloop()

GUI:

I would be grateful to hear about any improvements that could be made to this code.

Answer 1

ALPHABET = "abcdefghijklmnopqrstuvwxyz"

We don't like from string import ascii_lowercase ? Ok, whatever.

---

        self.output = None

    def encode(self, key, message):
        try:
            self.output.delete(0, "end")

I don't understand what's going on here.

Did we want to initialize output to the [] empty list, instead?

            self.output.insert(0, self.encode_fun(key, message))

Note that inserting at front is O(N), while appending at end is O(1). Consider using a deque.

[EDIT: Later we see self.output = tk.Entry(frame, .... Maybe the identifier output is on the vague side and could be renamed to suggest that it's a display window? Or maybe just a # commment here would help to better communicate the intent.]

        except:
            return

No.

This should apparently mention pass instead of return, though there's no difference in behavior ATM. A pass emphasizes to the Gentle Reader that we're evaluating this method for side effects. If we did want return for some reason, consider making it an explicit return None, since that is what happens. Prefer pass, consistent with the signature's (lack of) type hints.

Avoid "bare except". Prefer except Exception:, so as not to interfere with KeyboardException. Better yet, use except AttributeError: if that's what you're expecting, and fix the code defect which triggers "'NoneType' object has no attribute 'delete'" if that's what motivated the try.

---

Kudos on keeping the Tk graphic calls confined to .display(). For one thing, this makes it easy to add unit tests later on for encode / decode.

I also appreciate how everything is a local variable rather than defining self.this, self.that, and self.the_other. It reduces coupling, and means the Reader has fewer things to worry about when reading other bits of code.

---

Please add these type hints:

def caesar_shift_encode(key: str, message: str) -> str:

I initially expected key: int, based on the usual definition of a Caesar cipher.

        except:
            encoded_message += char

No.

Here, at least, it is apparent that Author's Intent was to cope with "ValueError: substring not found". So say that explicitly. In any event, don't use a bare except.

---

def xor_cipher_encode(key, message):

This is an odd signature. It suggests parallel structure with the Caesar signature that accepted a single integer f"{key}". But it's not that.

In a crypto context we might expect 128 bits or 512 bits of high-entropy keying material. But it's not that.

It turns out the "key" is a "one-time pad". Ok. I'm just surprised is all.

Validate that len(otp) >= len(message), so caller won't be surprised by silent truncation.

---

root = tk.Tk() ...

Please protect this with a guard:

if __name__ == "__main__":
    root = tk.Tk() ...

Why?

At some point you or a collaborator will want to write a unit test, which should be able to import this without side effects.

---

This program accomplishes its design goals, with fairly clear and mostly bug free code.

I would be happy to delegate or accept maintenance tasks on this codebase.