• 1 Post
  • 30 Comments
Joined 3 months ago
cake
Cake day: April 11th, 2024

help-circle
rss



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