Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hormonal Hedonists #16

Merged
merged 145 commits into from Jan 26, 2020
Merged

Hormonal Hedonists #16

merged 145 commits into from Jan 26, 2020

Conversation

@genievot
Copy link
Contributor

genievot commented Jan 18, 2020

This is the initial pull request.

Wattleninja and others added 30 commits Jan 18, 2020
Uploading SFX for the game.
Feat/animated background
lemonsaurus pushed a commit that referenced this pull request Jan 26, 2020
Add main widgets to RotorScreen
lemonsaurus pushed a commit that referenced this pull request Jan 26, 2020
Revert "Sound"
@lemonsaurus lemonsaurus merged commit 01ca65d into python-discord:master Jan 26, 2020
1 check passed
1 check passed
Code Jam 6 - Linting Build #20200126.224 succeeded
Details
lemonsaurus pushed a commit that referenced this pull request Jan 26, 2020
Feature basic deck
lemonsaurus pushed a commit that referenced this pull request Jan 26, 2020
Cuneiform to ledger
lemonsaurus pushed a commit that referenced this pull request Jan 26, 2020
adding a skeleton unittest and beginnings of ViewControl
@lemonsaurus lemonsaurus changed the title Hormonal-Hedonists Hormonal Hedonists Jan 26, 2020
@SebastiaanZ SebastiaanZ self-assigned this Feb 4, 2020

- **Code Jam Rules:** [https://pythondiscord.com/pages/code-jams/code-jam-6/rules/](https://pythondiscord.com/pages/code-jams/code-jam-6/rules/)
## Running the project
`pipenv run start`

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 6, 2020

Member

This section doesn't cover the pip installation method listed above in the "Installation" section.

self.bind(health=self.on_health_change)

def start(self):
Logger.info('Start game')

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 6, 2020

Member

I like the use of the Kivy Logger; it shows that you're trying to make maximum use of the tools Kivy offers you.

from kivy.vector import Vector


class Game(EventDispatcher):

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 6, 2020

Member

I like the decision to go for an EventDispatcher subclass here. It makes sense, since the Game has quite a few possible events that could happen during play. It really helps with reasoning about the flow of execution in the application and it greatly increases the readability.

LIGHT_FOCUS_OFFSET.y+LANE_BOUNDS[self.mirror.state][1]),
surface=self.mirror.mirror_axis)

# This property returns the closest ship in the active lane.

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 6, 2020

Member

This pseudo-docstring should have a been a real docstring since docstrings have a special meaning in Python/in the Python data model. The contents of a docstring will be bound to the object's __doc__ attribute and be used things like auto-generated help information for the object (e.g., help(Game.closest_ship) within Python).

You can read more about this in PEP 257 -- Docstring Conventions and the Data Model page in the documentation (search for __doc__ on that page).

if not self.running:
return False
elif self.pause_game:
return
Comment on lines +65 to +68

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 6, 2020

Member

Does the difference between returning False and None here matter for the game?

(I know the answer: it's for the Clock.schedule_interval, but you'd have to be familiar with the rest of the codebase and Kivy to understand that. A comment or, maybe even better, a docstring explaining why there are two different possible return values (False and None) would make it easier for the reader to know why this construction was added to two methods without having to look for the callers and/or the documentation of Kivy's Clock class.)

from .entity import Entity, MovingEntity
from .light import LightRays
from .mirror_cannon import MirrorCannon
from .ship import BrownShip, GoldenShip

__all__ = ['Entity', 'MirrorCannon', 'LightRays', 'MovingEntity', 'BrownShip', 'GoldenShip']
Comment on lines +1 to +6

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 6, 2020

Member

Good use of relative imports and __all__.

if self.time_to_focus >= 5:
self.time_to_focus -= 0.1
self.event_focus = Clock.schedule_interval(start_focus, 0.01)

if self.time_to_focus <= 0:
Clock.unschedule(self.event_focus)
self.indices = [0, 1, 2]
vertex1, vertex2 = self.mirror[1:3]
self.vertices.extend([self.point.x, self.point.y, vertex1.x, vertex1.y])
self.vertices.extend([vertex1.x, vertex1.y, vertex2.x, vertex2.y])
self.vertices.extend([vertex2.x, vertex2.y, self.point.x, self.point.y])
Comment on lines +62 to +72

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 6, 2020

Member

If I'm reading this right (and I may not be), this trace method relies on being called more than once and scheduling/unscheduling the created event_focus helper function depending on the time_to_focus state. However, the >= in the first if statement makes it appear as if that self.time_to_focus attribute could potentially be larger than 5, which opens up the possibility of scheduling the helper function multiple times if it was initially larger than 5.1 (at least, that's how it appears from just reading the code).

As far as I know, Clock will hold on to a reference internally, which means that an additional schedule task would not be garbage collected or unscheduled. Likewise, is it possible to "untarget" a ship before the "focus" action is complete? Would that also mean that the Clock.unschedule is never run?

Now, I think this is probably not something that actually happens, but it does leave open the possibility of a hard to find bug.

class Entity(EventDispatcher):
id: str = ''
shape: Widget = None

def step(self, dt, game):
pass

def __repr__(self):
return f'{self.__class__.__name__}()'


class MovingEntity(Entity):
health = BoundedNumericProperty(MAX_SHIP_HEALTH, min=0, max=MAX_SHIP_HEALTH,
errorhandler=lambda x: 0 if x < 0 else MAX_SHIP_HEALTH)
velocity: Tuple[float, float]

def __init__(self, health, velocity, **kwargs):
super().__init__(**kwargs)

self.health = health
self.velocity = velocity

@property
def is_dead(self):
return self.health <= 0

def step(self, dt, game):
x, y = self.shape.pos
dx, dy = self.velocity
self.shape.pos = (x + dx, y + dy)
Comment on lines +10 to +39

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 6, 2020

Member

Nice us of inheritance and code reuse. The base classes makes sense and show nicely how you can use a simple inheritance tree to factor out common factors and reuse code.

@SebastiaanZ

This comment has been minimized.

Copy link
Member

SebastiaanZ commented Feb 6, 2020

Code Review

Commit Quality

While the commit quality of this project is mixed, I'd say it's above average when compared to the other projects in the code jam. There are a lot of commits with informative summary lines that help me understand what's going when reading through the commit history without having to study the underlying code. That said, there are still commits that have generic summary lines like "fixed some issues", "added functionality", and "didn't mean to commit this". A summary line that better captures the actual changes introduced by the commit (e.g., "Revert accidentally committed changes to constants and default values") would make it easier to directly see what kind of changes the commit makes. What I am missing are message bodies: There are very few commits that have a message body explaining in more detail what's going on and why.

When I look at the commits themselves, I see a lot of nice, atomic commits that consist of a single, coherent change. There are a few that I think could have been separated into more atomic separate commits, but, overall and considering this is a Code Jam, I'm pleased by how the commits are structured. One thing I would recommend is the use of a linter/pre-commit hook; there are a lot of, often consecutive, "Fixed linting errors" commits that shouldn't have been there. As a general recommendation, I'd recommend checking git features like git rebase -i (interactive rebase) and git commit --amend to make sure the local commit history is "clean" before pushing it to a remote repository. (This obviously only helps if you lint your code before pushing it.)

Since we are a learning server and to provide a bit of context to the commit quality review above, I've attached a couple of links to resources on what makes a good commit/commit message:

Readme

The README of this project is of average quality. While I like the addition of the illustration, the rest of the README is limited to the basic installation details and controls. As the README of a project is often the first thing people see when they browse to an application on GitHub, I would have liked to see a general description of what the application. It isn't until I read the description of the ESC key in the "Controls" section that I know I'm dealing with a game. More generally speaking, a great README serves as a billboard for the project and makes people want to install and play the game. I do highly appreciated the attribution section; I really like it when people pay credit to those whose resources they are using.

(As a small side note: The "Installation" section lists a method of installing the dependencies of the project using pip directly. However, the "Running the project" section only contains information on how to start the project using pipenv.)

Code Quality

I really like the overall code quality of this project. The structure is really good, it makes good use of the features Kivy offers, utilizes inheritance in a really nice manner, and it uses modern and idiomatic Python patterns. The code is easy to read and understand and it makes sense when you read it.

When it comes to the documentation of the code, I like the use of block comments to guide the reader through the more complicated parts of the code. That said, I would have loved to see the use of docstrings to have clear and succinct description of each method right at the top of each one. The use of proper and descriptive names partly makes up for this, but even just adding docstrings with nothing more than summary lines would have been great.

Overall, I think this team has expressed their ideas clearly in their code: it's clean, it's concise, follows common style conventions, and it's easy to grasp what each part does.

Summary

Taken together, I really like this project. Sure, the commit history could have been a bit cleaner, I would have liked to see more docstrings, the README could have had more content, but that doesn't change that it looks like a solid job.

Pros

  • Excellent code quality
  • Using common style conventions
  • Good use of inheritance
  • Make good use of Kivy's features
  • Above average commit quality

Cons

  • No docstrings
  • Very basic README
@lemonsaurus lemonsaurus added the Team PR label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.