GUI Caesar Cipher with model-view-controller

Question

I created a Caesar Cipher Program in Tkinter, to help me learn the Model-View-Controller concepts and Tkinter in general. The code works, but it is a mess, and I want some help on cleaning it up, reducing junk, and just making it better.

import string
import tkinter as tk
from tkinter import ttk

class TextIO(tk.Frame):

    def __init__(self, parent):
        tk.Frame.__init__(self, parent)
        self.parent = parent

        self.inputLabel = ttk.Label(self, text = "Input:")
        self.inputLabel.grid(row = 0, column = 0)
        self.inputString = tk.Text(self)
        self.inputString.grid(row = 1, column = 0)

        self.outputLabel = ttk.Label(self, text = "Output:")
        self.outputLabel.grid(row = 2, column = 0)
        self.output = tk.Text(self)
        self.output.insert("0.0", "Type -1 in shift if you want all shifts when decrypting")
        self.output.grid(row = 3, column = 0)


class ButtonBox(tk.Frame):

    def __init__(self, parent):
        tk.Frame.__init__(self, parent)
        self.parent = parent

        self.shift = tk.Frame(self)
        self.shift.grid(row = 0, column = 0)

        self.shiftLabel = ttk.Label(self.shift, text = "Shift:")
        self.shiftLabel.grid(row = 0, column = 0)
        self.amountShift = ttk.Entry(self.shift, width = 5)
        self.amountShift.grid(row = 0, column = 1)

        self.encryptButton = ttk.Button(self, text = "Encrypt")
        self.encryptButton.grid(row = 0, column = 2)

        self.decryptButton = ttk.Button(self, text = "Decrypt")
        self.decryptButton.grid(row = 0, column = 3)


class Model:

    def encrypt(self, message, shift):
        shift = int(shift % 26)
        ALPHALOWER = string.ascii_lowercase
        ALPHAUPPER = string.ascii_uppercase

        #Shift letters based on the 'shift' amount
        shiftLower = ALPHALOWER[shift:26] + ALPHALOWER[0:shift]
        shiftUpper = ALPHAUPPER[shift:26] + ALPHAUPPER[0:shift]

        lowerMapping = dict(zip(ALPHALOWER, shiftLower))#Entry: {a: b, b: c}
        upperMapping = dict(zip(ALPHAUPPER, shiftUpper))

        encrypted = ''
        #For every character in unencrypted, find if it is a letter,
        for index, char in enumerate(message):
            if char.isalpha() is True:
                #Then if it is, look for the key in lowerMapping, and if it
                #isn't found, look in upperMapping
                try:
                    encrypted += lowerMapping[char]
                    #Returns the shifted letter
                except KeyError:
                    encrypted += upperMapping[char]
            else:
                #If it is not a letter, just add it to the string
                encrypted += char

        return encrypted


    def decrypt(self, message, shift):
        if shift == -1:
            allDecryptions = {}
            for i in range(0, 26):
                allDecryptions[i] = self.encrypt(message, i)
            allDecryptionsString = ''
            for i in range(0, 26):
                allDecryptionsString += str(i) + ': ' + allDecryptions[i]
            return allDecryptionsString
        else:
            shift = int(shift % 26)
            decrypted = self.encrypt(message, -shift)#Negative Shift
            return decrypted        


class MainWindow(tk.Frame):

    def __init__(self, parent):
        tk.Frame.__init__(self, parent)
        self.parent = parent
        self.model = Model()


        self.UserIO = TextIO(self)
        self.UserIO.grid(row = 0, column = 0)
        self.Buttons = ButtonBox(self)
        self.Buttons.grid(row = 1, column = 0)
        self.Buttons.encryptButton.config(command =  self.Encrypt)
        self.Buttons.decryptButton.config(command =  self.Decrypt)


    def Encrypt(self):
        message = self.UserIO.inputString.get("1.0", tk.END)
        try:
            shift = int(self.Buttons.amountShift.get())
        except ValueError:
            tk.messagebox.showerror("Invalid Shift",
                                   "You did not enter a valid shift, assumed 1")
            shift = 1
        self.UserIO.output.delete("1.0", tk.END)
        self.UserIO.output.insert("1.0", self.model.encrypt(message, shift))

    def Decrypt(self):
        message = self.UserIO.inputString.get("1.0", tk.END)
        try:
            shift = int(self.Buttons.amountShift.get())
        except ValueError:
            tk.messagebox.showerror("Invalid Shift",
                                   "You did not enter a valid shift,assumed 1")
            shift = 1
        self.UserIO.output.delete("1.0", tk.END)
        self.UserIO.output.insert("1.0", self.model.decrypt(message, shift))          


root = tk.Tk()
root.resizable(width = False, height = False)
MainWindow(root).pack()
root.mainloop()

The program uses 2 section for the 'view' of the GUI, a 'TextIO' box of two tk.Text()s, and a ButtonBox with the shift for the CaesarCipher and the Encrypt and Decrypt buttons. I included the encrypt() and decrypt() in the Model, I'm not sure if that is the proper way to do that, and the Controller is the MainWindow

Answer 1

Common sense and general appearance

From a common sense perspective, the code does not look very messy at all. The structure looks relatively well-aranged. My main point of criticism would be that there are no class and/or method/function docstrings (PEP257 at your service here). The class Model should also have a more descriptive name, that tells more what it is really doing. E.g. CaesarCipher could be a good alternative to consider.

Style and code conventions

If one wants to be nit-picky in the sense of Python code conventions (for a full reading, see PEP8), there are a some aspects that might need a little attention.

1. No space around keyword arguments, e.g. ..., text = "Input:") should be ..., text="Input:")
2. Variable and class member names are supposed to be all_lower_case_with_underscores. The same stands for function names, as opposed by Encrypt and Decrypt in MainWindow.

3. You should wrap the part where you start TK's main loop into a function and call that function in a if __name__ == '__main__': block like so:

   def main():
       """Main function for script usage"""
       root = tk.Tk()
       root.resizable(width=False, height=False)
       MainWindow(root).pack()
       root.mainloop()
   
   if __name__ == '__main__':
       main()
   

   This neat trick would allow possible users of your program to import it as a module into their own programs without starting the GUI each time.

Use of MVC

In general the code mirrors the MVC-concept reasonable good. The main window would serve as combined View and Controller, whereas the Model (I still don't like the name too well) would clearly be, surprise surprise, the Model. To answer the question whether or not it is legitimate to put encrypt and decrypt in the Model, I would say absolutely yes. Wikipedia agrees on this and tells us that "[t]he model directly manages the data, logic and rules of the application." and de-/encryption is undeniably the core of your application logic.

In general one could argue that a design pattern might be a little over the top for such a small program, but as you said, it is for the sake of exercise.

Answer 2

Text Indices

The first character in a text widget is the string "1.0", not "0.0" which is what you're using. It works, but it's not 100% correct.

Use of grid

This is somewhat personal preference, but I've found that code is easier to understand if you group all of your grid statements together. At least, all of the statements for widgets that are in a group:

    self.inputLabel = ttk.Label(self, text = "Input:")
    self.inputString = tk.Text(self)
    self.outputLabel = ttk.Label(self, text = "Output:")
    self.output = tk.Text(self)
    self.output.insert("0.0", "Type -1 in shift if you want all shifts when decrypting")

    self.inputLabel.grid(row = 0, column = 0)
    self.inputString.grid(row = 1, column = 0)
    self.outputLabel.grid(row = 2, column = 0)
    self.output.grid(row = 3, column = 0)

In my opinion this makes it much easier to visualize the layout.

Also, you aren't calling rowconfigure or columnconfigure to set row and column weights. As a rule of thumb, every container widget that uses grid must define at least one row and one column with a positive weight. This tells tkinter how to allocate extra space. Without it, your GUI may not look right if the user resizes, or if they have different fonts or a different resolution from you.

Follow PEP8

PEP8 is a set of conventions for writing python code. It's a good idea to follow them unless you really, really need to do something your own way for a very specific reason.

The most glaring violation is using capitalized function names. You need to change Encrypt and Decrypt to encrypt and decrypt. Likewise for self.UserIO, self.Buttons, etc. All should be lowercase.

Treat your code like a module

It's always a good idea to hide your main code under __main__. If for no other reason, it makes it possible to do unit testing by importing the file and interacting with the classes individually.

if __name__ == "__main__":
    root = tk.Tk()
    root.resizable(width = False, height = False)
    MainWindow(root).pack()
    root.mainloop()

Remove root.resizable(width = False, height = False)

I realize this is personal preference, but there is almost never a good reason to remove the ability of the user to resize the window. They know more about their system than you do. Design your GUI to be resizable. It's not hard, and your users will thank you.

Use explicit options to pack

Always include explicit options for fill and expand. The defaults are rarely what you want.

MainWindow(root).pack(fill="both", expand=True)