5
\$\begingroup\$

I am new to Python and following tutorials. This one I came across was for TKinter, and throughout I felt I re-wrote the same lines of code time and time again which I wondered if it could be better written.

I am interested to know how you would improve this code.

Thanks.

(First time posting, hope it's ok).

from tkinter import * from PIL import ImageTk, Image root = Tk() root.title("Learn to code") root.iconbitmap("C:\\palmTree.ico") #random images my_img1 = ImageTk.PhotoImage(Image.open("C:\\mountainLake.jpg")) my_img2 = ImageTk.PhotoImage(Image.open("C:\\freedom.jpg")) my_img3 = ImageTk.PhotoImage(Image.open("C:\\streetCoffee.jpg")) my_img4 = ImageTk.PhotoImage(Image.open("C:\\surfBoarding.jpg")) my_img5 = ImageTk.PhotoImage(Image.open("C:\\oldFence.jpg")) image_list = [my_img1, my_img2, my_img3, my_img4, my_img5] my_label = Label(image=my_img1) my_label.grid(row=0,column=0,columnspan=3) def forward(image_number): global my_label global button_forward global button_back my_label.grid_forget() my_label = Label(image=image_list[image_number-1]) button_forward = Button(root, text=">>", command=lambda: forward(image_number+1)) button_back = Button(root, text="<<", command=lambda: back(image_number-1)) if image_number == 5: button_forward = Button(root, text = ">>", state=DISABLED) my_label.grid(row=0,column=0,columnspan=3) button_back.grid(row=1, column=0) button_forward.grid(row=1, column=2) def back(image_number): global my_label global button_forward global button_back my_label.grid_forget() my_label = Label(image=image_list[image_number-1]) button_forward = Button(root, text=">>", command=lambda: forward(image_number+1)) button_back = Button(root, text="<<", command=lambda: back(image_number-1)) if image_number == 1: button_back = Button(root, text = "<<", state=DISABLED) my_label.grid(row=0,column=0,columnspan=3) button_back.grid(row=1, column=0) button_forward.grid(row=1, column=2) button_back = Button(root, text="<<", command=back, state=DISABLED) button_quit = Button(root, text="Quit", command=root.quit) button_forward = Button(root, text=">>", command=lambda: forward(2)) button_back.grid(row=1, column=0) button_quit.grid(row=1, column=1) button_forward.grid(row=1, column=2) root.mainloop() ```
\$\endgroup\$
0

1 Answer 1

Reset to default
3
\$\begingroup\$

Specific suggestions

  1. Usually when you find yourself numbering variables it's time to put all of them in an iterable of some sort. In your case I would simply inline my_img1 etc. into image_list.
  2. Usually names like images would be used instead of image_list. Python is a duck typed language, so it doesn't really matter which type of iterable you use for the code which does the iterating.

  3. Rather than running the main code like root.mainloop() the Pythonic way to do it is this:

    if __name__ == "__main__": main()

    This ensures that the code doesn't run when importing from your file, making it possible to import into a bigger program.

  4. Importing everything from a module is considered an antipattern - it makes it easier to end up with name collisions and it makes it harder to see at a glance where something was imported from.
  5. Refer to existing data structures whenever possible. One example is if image_number == 5, which would be better as if image_number == len(image_list).
  6. Avoid doing unnecessary work. For example, you create an object and assign it to button_forward, then you check whether you have to create a different object to assign to the same variable, in which case the original work was wasted (or in the worst case, had detrimental side effects). I would instead use an if/else to assign button_forward only once.
  7. globals are considered a big code smell these days. A preferable pattern is to pass around exactly the values which each function/method needs. That way the code is much easier to debug. In fact, this is pretty much the perfect example of something which should be encapsulated in one or more classes. That way you have a natural place to keep track of state like image_number, and won't have to pass it around between methods.
  8. It doesn't seem like a stretch that the images would be the user input to this application. If so it would be better to provide them using argparse.

Tool support suggestions

  1. 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.

  2. flake8 can give you hints to write idiomatic Python. I would start with this configuration:

    [flake8] max-complexity = 4 ignore = W503,E203
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.