Litigious Liberators

[duarteocarmo]

No description provided.

[jb3]

Code is Pythonic but in places is more complicated than needs be. In the code responsible for handling the introduction to the game and the storyline attributes are dynamically assigned and fetched which should have been a dictionary instead of attributes. There are other places where there are things such as multiplication with 0.

There is a lack of consistent code style, an example of this is the differing styles of super, one of the review comments addresses this. A positive note is that black has been installed which I suspect has helped to unify the code somewhat.

There is a lot of commented out code, even examples code which was not commented out for bugs and could have been removed such as:

# def ola():
#     print("HELLLOOO")
#
#

Also a lot of commented out print statements, I found a few TODO comments as well which should have been addressed.

On the topic of comments, or lack thereof, the code was missing pointers as to what was going on. No comments provided any hints on why certain decisions were made and how certain things worked. There were no docstrings and no documentation on any code whatsoever. Reviewing this PR comments and docstrings would have made this a lot easier to do, nothing huge, it is understandable that documentation is not a key focus during a jam, but for the benefit of your fellow team members it would be useful to add documentation.

Code was dense in some places, for example:

        super(ProfileCard, self).__init__(**kwargs)
        dir_path = os.path.dirname(os.path.realpath(__file__))
        pic_addr = f"../profiles/pictures/{profile['Picture']}"
        sound_addr = f"../profiles/sounds/{profile.get('Sound')}"
        self.ids.picture.source = os.path.join(dir_path, pic_addr)
        self.sound = SoundLoader.load(os.path.join(dir_path, sound_addr))
        self.deltas = Counter(profile["Deltas"])
        self.ids.name.text = profile["Name"]
        self.ids.right_choice.text += profile["Choices"]["right"]
        self.ids.left_choice.text += profile["Choices"]["left"]
        self.results = profile["Results"]
        self.ids.picture.popup.content.story.text = profile["Biography"]

Splitting this up somewhat would have been nicer. Another example of dense code was the imports, it is difficult when so many modules are used but there could have been some form of grouping used to make things tidier. Some basic errors such as having unused variables were in the code, while I haven’t programatically tested for these I’ve spotted one by eye which was very evident looking through the files.

As for the README and Project setup, the readme was tidy but could’ve given a description of the application, a screenshot of the running application or some pointer as to what the application was or did. You have suggested the usage of a virtual environment for dependency installation which is a good choice but really a tool such as pipenv or poetry to manage virtual environment and dependencies (as opposed to using pip) should have been utilised.

Commit quality dropped as the jam progressed going fro commits such as Added the transition from the onboarding Screen to Profile Creation to things like Readme updated and More readme. These commits could have done with a more descriptive body to allow someone to know what a commit does without having to look at the changes. There were a lot of commits adding code which was purely for testing. This led to commits that had to remove this code such as cleaning trash which messed up the commit history significantly, other commits such as:

• several test commits
• test file now clean
• main py (this did add an app but it displayed pictures of kittens seemingly)

Pros

• KV language was nice
• Good user experience, backstory was nice
• Readme detailed how to set up project well
• Lots of Kivy functionality used
• YAML for config was a nice way of handling constant data, very tidy setup

Cons

• No comments, no documentation, nothing.
• Unnecessary commits for adding and removing test documents
• Lack of consistency on some Python aspects
• Some code was convoluted and without comments was almost impossible to decipher what was going on
• Some non-Pythonic practices such as creating attributes dynamically as opposed to dictionaries.

[jb3]

Lack of consistency between super styles. super(cls, self) is the legacy way of super calls, this would be fine if all supers were in this style but in other classes plain super() is used which leaves this to be messy.

[OMotornyi]

I saw this comment in many code jam reviews, and I think the biggest reason super(cls, self) is used all over the places is because it is what we all see in kivy docs :)

[tshirtman]

i did think that too, not too long ago kivy supported python2 (and latest release still does), so the doc use the syntax that works in both versions.

[jb3]

Pathlib could be utilised here.

[jb3]

Import groups would be nice here. Maybe:

• stdlib imports (plain imports and then from X import Y imports)
• kivy standard library
• additional kivy libraries

[jb3]

Classes could do with having docstrings in them to explain usage for them. This review comment applies to all classes lacking docstrings.

[jb3]

Kitten URLs? This commented out code should be removed.

[jb3]

This section could have benefitted from line breaks.

[jb3]

Grammar and dialect of this label are a bit weird, instructions should be clear, not personalised, which means no ;), spaces after commas and arrow keys

[jb3]

You can initialise a counter with a dictionary.

[jb3]

k and v are not descriptive enough variable names, what are k and v? Variables should not be one character.

[jb3]

You use YAML elsewhere, this could probably be YAML. What is the file name as well?