I started working through the 100 Days of Code course of Udemy last February, and I’m in the home stretch. I’m on the final lessons, which are really just prompts for projects. No hand holding, just a brief description of the goal. I recently finished a tkinter GUI program, the goal of which was to enable adding text watermarks.

I took a few liberties–mainly, I made it possible to layer a png on top of the background. It was a really fun project and quickly grew more complicated than I expected it to. I got some hands on experience with the Single Responsibility Principle, as I started off doing everything in my Layout class.

Eventually, I moved all the stuff that actually involved manipulating the Image objects to an ImageManager class. I feel like I could have gotten even more granular. That’s one thing I would love to get some feedback on. How would a more experienced programmer have architected this program?

Anyway, I guess this preamble is long enough. I’m going to leave a link to the repository here. I would have so much appreciation for anyone who took the time to look at the code, or even clone the repo and see if my instructions for getting it to run on your machine work.

Watermark GUI Repo

  • best_username_ever@sh.itjust.works
    161·
    5 months ago

    My random shitty opinion, don’t take it personally, I didn’t slept well, also I’m late for work:

    README: you use both py and python3, choose one because I’m confused! Also you say “Navigate to ./src/” No, I’m a lazy user and I want instructions that I can copy-paste, it’s always better when you clone a random project (especially at work) and be able to copy-paste, like:

    1. Install Python 3 (and NOT “make sure…” it’s confusing, how would I make sure? Your code could be used by non-programmers you know? also put a link to python.org too)
    2. Run:
    py -m pip install -r requirements.txt
cd src/
py main.py
    1. Do this
    2. Do that
    3. (…)

    Be affirmative! Also “This will install pip” could be wrong on most systems, remove that sentence if not true.

    Still in the README, why should I run the thing from src? Is your application broken if I I do “py src/main.py”? What happens?

    It seems like the GUI and the code that watermarks are mixed and that’s annoying. If it was clearly separated, you could make a command-line versions of your application in 5 minutes without changing the GUI, for example with argparse.

    Why is there so much code to set the layout in main.py? Put that stuff in Layout, I don’t want to see that in my main. Also do “def main(): …” and “if __name__ == ‘__main__’” or something, it’s cleaner, and it prevents errors if I “import main”

    Do you really really need all those members variables? I understand that Tk is weird, but ImageManager has 12 members, main has 3 instead of 1 (the main “Window”), and Layout has a billion members. For example total_columns and total_rows are not used in Layout.py, that’s confusing. ImageManager.SAVE_DIR and IMAGE_RESIZE_FACTOR are constants, move them out. DEFAULT_FOLDER is only used once, merge it with TEST_BG, that kind of thing.

    ImageManager.path_is_valid is useless and potentially harmful because you’re duplicating standard code, remove it and use path.exists, no need to replicate the standard library, and other coders know what’s inside the path module, they don’t know what’s in your code and why it’s different from the standard modules because they’ll think “if it’s there, it must do something special!” (but it’s not special at all here)

    Ideally you shouldn’t put testing code in your main code. TEST_BG and TEST_FG will have to be removed. I understand why it’s there, it’s faster for your test, but it always show that the architecture has flaws. For example here, it shows that you forgot to make it possible to load those things on the command line, like main.py --test-bg space.png --test-fg python-watermark.png or better main.py --bg space.png --fg python-watermark.png, see? You have the beginning of a command-line application!

    On GitHub you have 6 branches, that’s madness. Merge them all one at a time, or delete them. Too many experiments are not good.

    You commit messages are good and expressive, that’s nice! Also I see that you used the standard .gitignore for Python on GitHub, that very nice and way better than doing one from scratch who will miss a lot of stuff.

    I’ll come back later if I can.

    Edit: there is hardcoded paths “/home/mike/code” and no default pictures, I can’t test it right now, that’s something to fix too.

    • Hammerheart@programming.devOP
      6·
      5 months ago

      Thank you SO MUCH. This is exactly the kind of response i wanted, and also thought it would be naive to hope for. Seriously, you’rr awesome.

      And i really appreciate how you even looked for something nice to say too. :)

      • best_username_ever@sh.itjust.works
        1·
        5 months ago

        Thanks. I like helping for stuff like that.

        Last but not least: when you make a lot of small changes, always do:

        1. Make small fix
        2. Test!
        3. Stage or commit on git

        This way you won’t get lost. And don’t fix everything at once. Make a list of small changes and do that one at a time.

        Also to make development easier:

        • create a virtual environment if you need (maybe hard)
        • use PyCharm, it’s great (easy)
    • Hammerheart@programming.devOP
      2·
      5 months ago

      I just want you to know you weren’t screaming into the void. Look at my new main.py:

      import io
import os
import tkinter as tk
from pathlib import Path, PurePath
from tkinter import ttk

from PIL import ImageTk
from PIL import Image

from Layout import Layout

DEFAULT_FOLDER = PurePath("/home", "mike", "bg")
WATERMARK_DIR = Path(Path(os.getcwd()).parent, "assets", "img")


def main() -> Layout:
    return Layout()

if __name__ == "__main__":
    main()

      (I know I still need to change those folder defaults, but I am still riding the high of getting all that layout stuff into Layout.py and it working. I spent a couple hours today struggling, wondering why I was just getting a blank screen, when i realized i forgot to call .grid() on the frame that held all the widgets! So it was just rendering a blank window. I should probably clean up those import statements too. I was supposed to be done with this project a week ago!)

  • tatterdemalion@programming.dev
    8·
    5 months ago
    def path_is_valid(path: Path) -> bool:
    if not path.exists():
        return False
    return True

    There’s no reason for this function to exist. Can you see why?

  • FizzyOrange@programming.dev
    1·
    5 months ago

    I can give you some basic Python set-up advice (which is hard-won because nobody tells you this stuff):

    1. Use pyproject.toml, not requirements.txt. It’s better and it’s the recommended way.
    2. Always use type annotations. I saw you used it a bit - well done! But just use it everywhere. Add declarations & type annotations for your class member variables. You’ll thank me later! Also prefer Pyright to Mypy; it is far far better (and the default for VSCode).
    3. I would recommend using Black to autoformat your code. It’s easily the best Python formatter. You can automate it using pre-commit.
    4. ALWAYS PUT MAIN IN A FUNCTION. You should not run any code at a global scope. Do it like this:
    def main():
  ...

if __name__ == "__main__":
    main()

    There are two reasons:

    1. Generally it’s good to keep variables to the smallest scope possible. Putting everything in a global scope will just mean people start using it in their functions, and then you have a mess.
    2. Any code you put in a global scope gets run when you import a module. Importing a module should not have any side effects.

    Other minor things:

    1. Path has a crazy/neat override of the / operator so you can do Path("foo") / "bar" / "baz.txt".
    2. Don’t use assert for stuff that the user might reasonably do (like not choosing a background image). assert is meant to be for things that you know are true. You are asserting them. Like “hey Python, this is definitely the case”.
    3. TK is an ancient shitty GUI toolkit. I would recommend QT instead (though it will probably be more pain to set up).