4
\$\begingroup\$

I am pretty new to Python and I would like some tips and criticism on my formatting style/code and what can be better organized. I am a bit shaky on how classes work, but I think that it can be used here to make it more simple.

from tkinter import *
from tkinter import ttk
import random

#frame setup
main = Tk()
frame = ttk.Frame(main)
button_frame = ttk.Frame(frame)
text_frame = ttk.Frame(frame)
text = Text((text_frame), borderwidth=10, height=8, width=20,    relief="sunken",)

def complex_password(): #complex password function
    password2 = []
    password2.append(random.choice((capital)))
    count = 0
    x = ""
    while count != 12:
        roll = random.randint(1,4)
        if roll == 1 or roll == 2:
            password2.append(random.choice((letters)))
            count +=1
        if roll == 3 or roll == 4:
            number = random.randint(0,9)
            number = str(number)
            password2.append(number)
            count +=1
        if count == 12:
            password2 = x.join(password2)
            text.insert(INSERT, password2)
            text.insert(INSERT, "\n")

def simple_password(): #simple password function
    y = ""
    password1 = []
    password1.append(random.choice((words)))
    password1.append(random.choice((words)))
    number = random.randint(1,99)
    num = str(number)
    password1.append(num)
    password1 = y.join(password1)
    text.insert(INSERT, password1)
    text.insert(INSERT, "\n")

def clear_text(): #clear txt box function
    text.delete(1.0, END)

#buttons
simple = ttk.Button(button_frame, text="Simple", command=simple_password)
complex = ttk.Button(button_frame, text="Complex", command=complex_password)
clear = ttk.Button(button_frame, text="Clear", command=clear_text)

#buttons grids
simple.grid(column=2, row=1)
complex.grid(column=1, row=1)
clear.grid(column=3, row=1)
text.grid()

#frame grids
frame.grid(column=1, row=2)
text_frame.grid(column=1, row=2)
button_frame.grid(column=1, row=1)

#misc settings
for child in frame.winfo_children(): child.grid_configure(padx=5, pady=10)
main.title("Password Gen")
main.resizable(width=FALSE, height=FALSE)
main.geometry("238x230")


words = ['Dog', 'Cat', 'Mouse', 'Fire', 'Ice', 'Basket', 'Tree', 'Tiger',
     'Lion', 'Flash','Super', 'Light', 'Zoom','Speed', 'Pants', 'Shirt',
     'Hat', 'Suit', 'Berry', 'Yogurt', 'Epic', 'Keyboard', 'Toe', 'Car',
     'Truck', 'Bike', 'Motor', 'Hammer', 'Pizza', 'Heart', 'Arm','Joint',
     'Saw', 'New', 'Carrots', 'Baby', 'Kiss', 'Backspace',  'Enter', 'Alt',
     'Print', "Down", 'Up', 'Question', 'Rain', 'Forest','Red', 'Orange',
     'Yellow', 'Green', 'Blue', 'Purple', 'Brown', 'Black', 'Indigo', 'Grey',
     'Shadow', 'Eye', 'Brick', 'Twig', 'Gangster', 'Thug', 'Chains', 'Gold',
     'Silver', 'Bronze', 'Platinum', 'Titanium', 'Exploding', 'Ladybug', 'Grass',
     'Monkey', 'Rhino', 'Comma', 'Hair', 'Shark', 'Fish']
letters = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm',
       'n','o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']
capital = ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M',
       'N','O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z']

main.mainloop() #end of GUI
\$\endgroup\$

migrated from stackoverflow.com Feb 9 '16 at 13:30

This question came from our site for professional and enthusiast programmers.

6
\$\begingroup\$

I prefer

import tkinter as tk

and I can see that tk.Button, tk.INSERT, tk.FALSE are not my local variables/classes.


You can use string.ascii_lowercase instead of letters list, and string.ascii_uppercase instead of capital.

You don't have to convert string.ascii_lowercase and string.ascii_uppercase into list.

https://docs.python.org/3/library/string.html


password1 and password2 are local variables so you can use one name password in both functions.


for loop put in two lines - to make it more readable.

for child in frame.winfo_children():
    child.grid_configure(padx=5, pady=10)

In comments you can put one space after # - to make it more readable. You can put one space after every , .


You can use "" directly with join

"".join(password1)

You don't need ()

around text_frame in

tk.Text((text_frame), ...

# OK
tk.Text(text_frame, ...

around capitol in

choice((capital))

# OK
choice(capital)

around letters in

choice((letters))

# OK
choice(letters)

You can add empty lines in functions to separate some part of functions


In complex_password you can randint(1, 2) instead randint(1, 4) because it gives the same probability - 50% for letter and 50% for number.

In complex_password you can use for loop instead while and don't need variable count.

In complex_password you don't need if count == 12 because this part can be outside while/for loop.


You can organize code this way

import tkinter as tk
from tkinter import ttk
import random
import string

# === constants === (UPPER_CASE names)

    # empty

# === classes === (CamelCase names)

    # empty

# === functions === (lower_case names)

def complex_password(): #complex password function
    password = []

    password.append(random.choice(capital))

    for __ in range(12):
        roll = random.randint(1, 2)

        if roll == 1:
            password.append(random.choice(letters))
        else:
            number = random.randint(0, 9)
            number = str(number)
            password.append(number)

    password = "".join(password)

    text.insert(tk.INSERT, password)
    text.insert(tk.INSERT, "\n")

def simple_password(): #simple password function
    password = []

    password.append(random.choice(words))
    password.append(random.choice(words))

    number = random.randint(1, 99)
    password.append(str(number))

    password = "".join(password)

    text.insert(tk.INSERT, password)
    text.insert(tk.INSERT, "\n")

def clear_text(): #clear txt box function
    text.delete(1.0, tk.END)

# === main ===

# --- some variables ---

words = [
    'Dog', 'Cat', 'Mouse', 'Fire', 'Ice', 'Basket', 'Tree', 'Tiger',
     'Lion', 'Flash','Super', 'Light', 'Zoom','Speed', 'Pants', 'Shirt',
     'Hat', 'Suit', 'Berry', 'Yogurt', 'Epic', 'Keyboard', 'Toe', 'Car',
     'Truck', 'Bike', 'Motor', 'Hammer', 'Pizza', 'Heart', 'Arm','Joint',
     'Saw', 'New', 'Carrots', 'Baby', 'Kiss', 'Backspace',  'Enter', 'Alt',
     'Print', "Down", 'Up', 'Question', 'Rain', 'Forest','Red', 'Orange',
     'Yellow', 'Green', 'Blue', 'Purple', 'Brown', 'Black', 'Indigo', 'Grey',
     'Shadow', 'Eye', 'Brick', 'Twig', 'Gangster', 'Thug', 'Chains', 'Gold',
     'Silver', 'Bronze', 'Platinum', 'Titanium', 'Exploding', 'Ladybug', 'Grass',
     'Monkey', 'Rhino', 'Comma', 'Hair', 'Shark', 'Fish'
]

letters = string.ascii_lowercase
capital = string.ascii_uppercase

# --- init ---

main = tk.Tk()

main.title("Password Gen")
main.resizable(width=tk.FALSE, height=tk.FALSE)
main.geometry("238x230")

# --- objects ---

frame = ttk.Frame(main)
button_frame = ttk.Frame(frame)
text_frame = ttk.Frame(frame)
text = tk.Text(text_frame, borderwidth=10, height=8, width=20, relief="sunken")

# buttons
simple = ttk.Button(button_frame, text="Simple", command=simple_password)
complex = ttk.Button(button_frame, text="Complex", command=complex_password)
clear = ttk.Button(button_frame, text="Clear", command=clear_text)

# buttons grids
simple.grid(column=2, row=1)
complex.grid(column=1, row=1)
clear.grid(column=3, row=1)
text.grid()

# frame grids
frame.grid(column=1, row=2)
text_frame.grid(column=1, row=2)
button_frame.grid(column=1, row=1)

# misc settings
for child in frame.winfo_children():
    child.grid_configure(padx=5, pady=10)

# --- start the engine ---

main.mainloop()
\$\endgroup\$
  • \$\begingroup\$ I edited your post to change - lines to being --- as that creates a light horizontal line, which (I think) looks nicer. \$\endgroup\$ – SuperBiasedMan Feb 12 '16 at 17:56
  • \$\begingroup\$ @SuperBiasedMan I used - because on StackOverflow it looks better than - - - (as for me) but here - - - looks better than on StackOverflow :) \$\endgroup\$ – furas Feb 12 '16 at 18:35
0
\$\begingroup\$

Your code is quite confusing in some places. complex_password for example. Usually a function will return a value or run some process that modifies another value (eg a file on your computer or a class's attributes). It's unusual to see something like inserting the password result into a global object. It would be much nicer to return password2 and then call text.insert in the same place you call complex_password.

password = complex_password
text.insert(INSERT, password2)
text.insert(INSERT, "\n")

In your function you have a password2 but no password. If you were worried about duplicate names clashing then you needn't, password will just be a local value inside this function. It wont affect or be affected by other password values.

Instead of using a while loop with count, just use for _ in range(12):. That will loop twelve times with no need for a counter. Also x = "" is unnecessary, just call "".join.

This is how I'd rewrite the function:

def complex_password():

    password = [random.choice(capital)]
    for _ in range(12):
        roll = random.randint(1,2)
        if roll == 1:
            password.append(random.choice(letters))
        else:
            password.append(str(random.randint(0,9)))
    return ''.join(password)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy