Caesar cipher with GUI

Question

I have created a functional Caesar cipher in Python 3 and I have implemented a GUI using Tkinter.

def MOST_COMMON_LETTER():
    return("e")

def circle(l,n):
    if n < (len(l) - 1):
        return(l[n])
    else:
        return(l[n % (len(l))])

def most_frequent(l): # not whitespace
    l = l.replace(' ','')
    all_frequencies = [l.count(i) for i in l]
    for i in l:
        if l.count(i) == max(all_frequencies):
            return(i)

def my_formatting(string):
    for i in string:                                     
        if i in ".,1234567890?!;:_;-/()[]{}+-*\"'":
            string = string.replace(i,'')               
    string = string.lower()
    return(string)

def en_de_code(string,key,en_or_de): #calling with en is encode, calling with de is decode
    alphabet = "abcdefghijklmnopqrstuvwxyz"
    new_string = ""
    string = my_formatting(string)
    for i in string:
        if i == " ":
            new_string += " "
        else:
            position = alphabet.index(i)
            if en_or_de == 'en':
                new_string += circle(alphabet,position + key)
            elif en_or_de == 'de':
                new_string += circle(alphabet,position - key)
    return(new_string)

def encode(string,key):
    return(en_de_code(string,key,"en"))

def decode(string,key):
    return(en_de_code(string,key,"de"))

def randomly_encode(string):
    import random
    key = random.randint(1,25)
    return(encode(string,key))

def brute_force_decode(string):
    new_string = ""
    for key in range(1,26):
        new_string += decode(string,key)
        new_string += '-'
    return(new_string)

def meaningful_decode(string):
    alphabet = "abcdefghijklmnopqrstuvwxyz"
    the_most_frequent = most_frequent(string)
    key = -(alphabet.index(the_most_frequent) - alphabet.index(MOST_COMMON_LETTER()))
    return(decode(string,key))

def GUI():
    try:
        import Tkinter as tk
    except:
        import tkinter as tk

    master = tk.Tk()

    master.wm_title("Caesar cipher")

    key_label = tk.Label(master, text="Key")
    key_label.pack()

    key_slider = tk.Scale(master, from_=1, to=25, orient=tk.HORIZONTAL,length=300)
    key_slider.pack()

    text_label = tk.Label(master, text="Enter your text down here")
    text_label.pack()

    text_field = tk.Text(master)
    text_field.pack()

    def allow_to_paste(string):
        master.clipboard_clear()
        master.clipboard_append(string)

    def encode_and_show():
        temp1 = encode(text_field.get("1.0","end-1c"),key_slider.get())
        result.set(temp1)
        allow_to_paste(temp1)

    b = tk.Button(master, text="Encode", command=encode_and_show)
    b.pack()

    def randomly_encode_and_show():
        temp2 = randomly_encode(text_field.get("1.0","end-1c"))
        result.set(temp2)
        allow_to_paste(temp2)

    b = tk.Button(master, text="Randomly encode", command=randomly_encode_and_show)
    b.pack()

    def decode_and_show():
        temp3 = decode(text_field.get("1.0","end-1c"),key_slider.get())
        result.set(temp3)
        allow_to_paste(temp3)

    b = tk.Button(master, text="Decode", command=decode_and_show)
    b.pack()

    def brute_force_decode_and_show():
        temp4= brute_force_decode(text_field.get("1.0","end-1c"))
        result.set(temp4)
        allow_to_paste(temp4)

    b = tk.Button(master, text="Brute force decode", command=brute_force_decode_and_show)
    b.pack()

    def meaningful_decode_and_show():
        temp5 = meaningful_decode(text_field.get("1.0","end-1c"))
        result.set(temp5)
        allow_to_paste(temp5)

    b = tk.Button(master, text="Meanigful decode", command=meaningful_decode_and_show)
    b.pack()

    result = tk.StringVar()
    result.set("Encoded text")

    paste_information = tk.Label(master, text="Just paste to en/de-coded text where you want it.")
    paste_information.pack()

    result_label = tk.Label(master, textvariable=result)
    result_label.pack()

    tk.mainloop()

if __name__ == "__main__":
    GUI()

• Is it understandable the first time you read it?
• Does it make use of "black magic" (absurd expressions that only who used them in the first place knows)?
• Should I explain briefly what my function do with a doc-string?
• Should I use a Class for the Tkinter GUI part?
• Is there something that is just bug-prone or considered bad style?

Answer 1

1.  

   def MOST_COMMON_LETTER():
       return("e")
   

   This should be just a variable, not function.

   MOST_COMMON_LETTER = "e"
   

2.  

   def circle(l,n):
       if n < (len(l) - 1):
           return(l[n])
       else:
           return(l[n % (len(l))])
   

   For simple code, we can eliminate the first branch - the % operator does the work for us. Also need better namings:

   def cycle_get(lst, index):
       return lst[index % len(lst)]
   

3.  

   def most_frequent(l): # not whitespace
       l = l.replace(' ','')
       all_frequencies = [l.count(i) for i in l]
       for i in l:
           if l.count(i) == max(all_frequencies):
               return(i)
   

   Here you do a lot of iterations, because l.count() iterates through the whole list/string. I would suggest using Counter. Also comment is worse than a good function name, let's name the function most_frequent_letter. Also your implementation does count punctuation and other non-letter symbols, though they are quite rare in real texts. Also for CAESAR cipher case is not meaningfull, so we need to strip it.

   import string
   from collections import Counter
   
   
   def most_frequent_letter(text):
       counts = Counter()
       for c in text:
           if c in string.letters:
               counts[c.lower()] += 1
       return counts.most_common(1)[0][0]
   

4.  

   def brute_force_decode(string):
       new_string = ""
       for key in range(1,26):
           new_string += decode(string,key)
           new_string += '-'
       return(new_string)
   

   It's better to return list of possible decryptions, not concatenation with "-". If you need to display it - it's better to concatenate right before the display (e.g. '-'.join(decryptions).

   def brute_force_decode(string):
       decryptions = []
       for key in range(1, 26):
           decryptions.append(decode(string, key))
       return(decryptions)
   

   Or even better, use list comprehension:

   def brute_force_decode(string):
       return [decode(string, key) for key in range(1, 26)]
   

5.  

   def meaningful_decode(string):
       alphabet = "abcdefghijklmnopqrstuvwxyz"
       the_most_frequent = most_frequent(string)
       key = -(alphabet.index(the_most_frequent) - alphabet.index(MOST_COMMON_LETTER()))
       return(decode(string,key))
   

   alphabet should be moved to the top and be capitalized (convention, so other people would suppose it's constant). Also in most_frequent_letter function you would be able to use your ALPHABET instead of string.letters.

6. The GUI function.

   • It is better to move imports to the beginning. Thus import errors will occur at the earliest time. Also people look for imports at the beginning to see which libraries are used.
   • I would suggest splitting the function into a couple of smaller functions. E.g. create_labels, create_buttons., or create objects depending on the visual grouping.

   • There is some redundant code with .pack(), but it seems that it is the badness of the Tkinter API. Except for buttons, you give them bad names which are immediately overwritten:

     code:

     b = tk.Button(master, text="Encode", command=encode_and_show)
     b.pack()
     

     I would replace such things to:

     tk.Button(master, text="Encode", command=encode_and_show).pack()
     

   Except these notes, GUI function looks quite good.

Answer 2

UI issues

• It's not obvious where the input text box is. It has no border or colour difference until it gains focus (which is difficult if you don't know where it is).
• It's not obvious where the output is. I would expect the output to be as prominent as the input — after all, it will contain just as much text. I would also want it to be a read-only text area so that I can select text and copy it to the clipboard.
• The buttons could be laid out better than than in a one-column arrangement.

Algorithm

en_de_code() is an awkward function name, and en_or_de is an awkward parameter. I would just define decode() in terms of encode(). The interpretation that comes to mind is "English or German?"

def encode(string, key):
    alphabet = "abcdefghijklmnopqrstuvwxyz"
    new_string = ""
    string = my_formatting(string)
    for i in string:
        if i == " ":
            new_string += " "
        else:
            position = alphabet.index(i)
            new_string += circle(alphabet, position + key)
    return new_string

def decode(string, key):
    return encode(string, -key)