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

  • Daniel Quinn
    link
    fedilink
    English
    arrow-up
    1
    ·
    edit-2
    5 months ago

    It’s Saturday night and I have nothing interesting to do, so I’m going to go through your code and leave a bunch of comments 'cause this is fun for me :-) Please don’t interpret the number of comments as a negative, there’s always something to criticise, even in work done by very experienced people. I just like to be thorough.

    1. You’ve got a readme.md file, which is good, but convention is that it should be named README.md. I know, it’s weird, but that’s the convention.

    2. You’ve pinned your dependency (Pillow) version, which is excellent, and at this stage you’re just dealing with the one dependency so it probably feels overkill, but you may want to look into more robust tools like Poetry for managing your dependencies. Poetry will handle all dependency management and upgrades and stuff those dependencies into a pyproject.toml file in the project root. Conveniently, you can use this same file to configure other tools you’ll come to depend on, like Ruff and Coverage. Some of the other comments here have recommended Black for formatting, but I think you’ll find that the community is moving away from the combination of linting & formatting tools (Black, iSort, flake8, pycodestyle) and are instead embracing “do everything” Ruff.

    3. Python convention is that all files should be named with snake_case as opposed to your CamelCase. Python will still work however you name your files, but if you want to write your code such that it plays well with other developers, it’s good to conform.

    4. Using a tool like Ruff or Black will ensure that your code conforms to community standards, again making it easier to read for everyone else. You’ve got some rather uncommon indenting and white space choices in your code that make it hard to read for those of us trained on the common style, so I recommend embracing one of these tools to conform your code.

    5. Before reaching for environment variables, consider what you can get with Python built-ins and the standard library. For example, you don’t need to import os and then run environ["PWD"] to get the current directory. In fact, doing this is a Bad Idea since it assumes things like the OS and environment that you can’t always count on. Instead, try just calling the built-in pwd() or Path.cwd(). Similarly, if you want to reference your ${HOME} folder, you can use Path.home().

    6. I love the use of classes. Not everyone does, but frankly I don’t understand people who avoid them. With that said though, you’ve implemented yours with behaviours you usually see in Java or C++, using getters & setters. Python will let you do this, but often it’s easier/cleaner to just allow calling code to set class properties directly. So for example, your .set_fg_position() method, could just as easily be done externally with my_manager.fg_x_position, my_manager.fg_y_position = coords

    7. For that matter, it’s a good idea to define your methods to expect the format you want, rather than be “forgiving” and do the conversion for you. So the aforementioned set_fg_position() might be better like this:

      def set_fg_position(self, coords: tuple[int, int]) -> None:
          self.fg_x_position, self.fg_y_position = coords
      

      Force the calling code to conform to the interface. It keeps your code tidy and your interface predictable.

    8. Don’t use assertions. In some projects, assertions are disabled in production and shouldn’t be counted on for validation or anything like that. They’re good for automated tests though.

    9. For that matter, you should consider writing some tests. Python comes with a built-in unittest framework, so you can just subclass TestCase and go to town. If you like though, there’s also PyTest just please if you choose to use it, keep your tests simple and don’t overindulge in conftest.py. That way lies madness.

    10. If your project depends on an asset like an image or font, you should include that asset in your source code. I see you’re referencing Pillow/Tests/fonts/FreeMono.ttf, but I’m not sure that you can count on that font being installed and available everywhere. Better to either have it in your project so you can reference it with a relative path, or list it as a dependency and talk about how you install it. I may be wrong on this though, given the Pillow in the path. I’m not sure how the font would be found if it’s in a virtualenv as part of the Pillow installation, but it’s weird.

    11. You definitely don’t want to be using environment values like OLDPWD for something like a SAVE_DIR. That variable is just the directory you were in before you came to your current one, so it’ll likely be different every time you run the program! Instead, I strongly recommend you define these values to be constant. Something like . (current folder) or Path.home() / ".config" / "myproject".

    12. For that matter, constants and variables are meant to be written differently in Pythonland. A constant should be in ALL_CAPS while a variable should be snake_case. I see you’re using that style, but you’re setting a constant on a class instance with self.SAVE_DIR = .... That’s pretty odd, as it’d mean that potentially your “constant” can have a different value between instantiations. What you probably want is a class constant, which is defined outside the __init__:

      class ImageManager:
          SAVE_DIR = pwd()
          IMAGE_RESIZE_FACTOR = 5
          def __init__(self) -> None:
              ...
      
    13. Looking at Layout.py now, I see you’ve baked your personal computer’s directory structure into your program. This is a classic “it works on my machine” situation ;-) You probably should set make IMG_HOME a variable that’s user-defineable (maybe with a command line argument?) or if the files in question can/should be part of your project, you could do something like:

      IMG_HOME = Path(__file__).parent / "path/to/assets/imgs"
      
    14. I love the use of type hints you’ve got going on, but they’re incomplete. It’s a good idea to strive for type completion if you can get it. It will help your IDE find bugs you didn’t realise were there, and if you wanna go all out and adopt mypy (use it with caution, it’s not a religion), it’ll help you get to complete type support.

    15. Looking at main.py now, I note that you’ve made the classic mistake of executing code on module load. This is a big Python no-no, because it means that if someone were to import your code, it would trigger your program! The standard way to do this properly is with this wacky incantation:

      def main():
          # This is where the program lives
      
      
      if __name__ == "__main__":
          main()
      
    16. The paths in this file are definitely going to break on other people’s computers as well. /home/mike/bg does not exist on my machine ;-) Similarly, you’ve used Path(Path(os.getcwd()).parent, "assets", "img") which will only work if (a) the current working directory has assets/img/ in it, and (b) assets/img actually exists on the machine. As they’re not in this repo, there’s no way this would run for me :-(

    You probably want to (a) copy the assets required for the project into the repo, and (b) define values like DEFAULT_FOLDER and WATERMARK_DIR based on values you know will exist on everyone’s machines, namely either (a) relative to your project (using the Path(__file__).parent() trick I mentioned above, or (b) somewhere you’ve instructed the user to place them as part of the installation.

    Finally, kudos on your choice of license. We need more AGPL code in the world <3!