Can I get some feedback / review on my code? (programming.dev)

submitted 5 months ago by Hammerheart@programming.dev to c/python@programming.dev

14 comments fedilink hide all child comments

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

you are viewing a single comment's thread
view the rest of the comments

[-] best_username_ever@sh.itjust.works 17 points 5 months ago* (last edited 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:

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)
Run:

py -m pip install -r requirements.txt
cd src/
py main.py

Do this
Do that
(...)

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.dev 7 points 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 points 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:

Make small fix
Test!
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.dev 2 points 4 months ago* (last edited 4 months ago)

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


from pathlib import PurePath


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