Enthusiastic-Electricians

[Leterax]

awesome first PR

[Leterax]

Final Submission.

[lemonsaurus]

You've managed to commit your .idea folder system files. These should've been ignored in your .gitignore

[Leterax]

I myself dont use PyCharm, so i wasnt aware this gets added, but ill make sure to tell my teammates about that! :)

[lemonsaurus]

What's the point of this file?

[lemonsaurus]

ahhhh this is the worst place this brace could be

[Leterax]

what do you mean by brace?

[lemonsaurus]

brace as in parenthesis. If you were going to split this line up over multiple lines, you should be using something called K&R indentation style, or one of the other acceptable indentation styles. These braces are all over the place and this makes readability quite poor. Look at https://en.wikipedia.org/wiki/Indentation_style for some more information on this.

K&R style indentation would look like this:

    def fill_bucket(self):
        self.setCursor(
            QCursor(
                QPixmap("Design/icons/{}.png".format("A bucket filled"))
            )
        )

But to be honest, even if this was a single line, it would only be 88 characters, and would be within the 100 char limit we set in the .flake8 file. so that would be acceptable, too. A little dense, though.

    def fill_bucket(self):
        self.setCursor(QCursor(QPixmap("Design/icons/{}.png".format("A bucket filled"))))

I have no idea why you would .format a string literal into a string either, so we can shorten it by doing this:

    def fill_bucket(self):
        self.setCursor(QCursor(QPixmap("Design/icons/A bucket filled.png")))

At this point readability is okay, but it's a bit unusual to have a filename with spaces, so let's change that too, while we're at it.

    def fill_bucket(self):
        self.setCursor(QCursor(QPixmap("Design/icons/bucket_filled.png")))

But the problem with this is that it will fail on any platform that doesn't support the / for path selection. The solution to this is to involve pathlib.

from pathlib import Path

ICONS = Path("Design", "icons")

def fill_bucket(self):
    # create a platform-agnostic path to the image
    # it's possible QPixmap doesn't support Path, so we can 
    # cast this to a string to make sure we pass it the same way
    filled_image = str(ICONS / "bucket_filled.png")

    self.setCursor(QCursor(QPixmap(filled_image)))

now this looks pretty good.

[adsapientiam]

Good that I am now aware of that. Were there any other serious errors we could have avoided?

[lemonsaurus]

I'm not sure if this code was written by hand or generated by the QT designer, but overall the code style is poor. There's a lot of inconsistent variable and method naming, and some questionable use of try: except:, and some strange conditions. That said, the code passes linting and looks like it probably does what it was intended to do.

You've committed a lot of pycharm files from the .idea folder, which could've been avoided by adding this folder to the .gitignore file. You have a __main__.py that doesn't do anything, and the entire 500 lines of code you wrote is in a single file. It would have probably been a good idea to organize this into more than one file.

We haven't actually tested the application yet (which is by far the most important thing) so it might still be an excellent submission - we'll see on Sunday!