-
Notifications
You must be signed in to change notification settings - Fork 8
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
Turning in MP4 #1
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good job! Please remember to add header comments to your files so we know what's going on when we open it up. Avoid global variables and free floating code. Either put them in functions or in classes.
@@ -1,3 +1,33 @@ | |||
# InteractiveProgramming | |||
# Interactive Programming |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good readme!
import random, pygame, sys, pygame.font | ||
from pygame.locals import * | ||
|
||
navyblue = (60,60,100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid global variables when you can. It seems like you could make these class attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A special case of a variable that doesn't actually vary – that's used as a constant. Python-the-languge doesn't distinguish between constants and other variables, but Python-as-used-by-humans does. The convention for a constant is all uppercase NAVY_BLUE
.
@@ -0,0 +1,34 @@ | |||
import random, pygame, sys, pygame.font |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good practice to have a header comment at the top of every file to explain what this file does. That way when people are cruising through all the files they can easily read a summary of the code.
self.rect.centerx = self.x + (self.image.get_width()/2) | ||
self.rect.centery = self.y + (self.image.get_height()/2) | ||
|
||
#figure out direction to fruit to move based on spawn position (left or right) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo :D
self.rect.centery = self.y + (self.image.get_height()/2) | ||
|
||
#figure out direction to fruit to move based on spawn position (left or right) | ||
border = screen.get_width()/2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of this init function could almost be it's own method that init calls. It's determining how the fruit will move so it's can be sectioned off in a comprehensible manner and there are only two class attributes that are created in 15 lines of code.
self.rect.centerx = x + (self.image.get_width()/2) | ||
self.rect.centery = y + (self.image.get_height()/2) | ||
|
||
def toss(self, time_passed): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot going on in this function that result in tossing. While the name of the function kind of explains what the function does, it might be worth having a docstring to explain the specifics.
add_y = .7*time_passed | ||
self.y += add_y | ||
|
||
class Apple(Fruit): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These fruit classes/half fruit classes could have been one class with an extra variable in the init call that determines the name of the file. It would have brought you down to 2 more classes rather than 9 more classes.
@@ -0,0 +1,246 @@ | |||
import random, pygame, sys, math |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really your master branch should only have the files that you need to run the program successfully. These redundant or unneeded files should either be on a separate 'dev' branch or not present at all. It took me a while to figure out why you had redundant code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw below that you had a test folder. This file could go there or have it's own or be deleted. Just something to show that it's not part of the main thing beyond just your README
from all_fruits import * | ||
from Scoring import * | ||
|
||
# Hand Tracking Parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the functions in this file should be in a 'game' class. Having it freeform like this makes me sad.
import imutils | ||
import cv2 | ||
import pygame | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be wrapped up in a function at the very least
class Fruit(pygame.sprite.Sprite): | ||
def __init__(self, screen, image_name): | ||
# Call the parent class (Sprite) constructor | ||
pygame.sprite.Sprite.__init__(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use super().__init__(self)
instead of pygame.sprite.Sprite.__init__(self)
. This makes the code less fragile (you can can change class Fruit(pygame.sprite.Sprite)
to class Fruit(MySprite):
, where MySprite
is some intermediate subclass of pygame.sprite.Sprite
, without needing to search Fruit
's methods for occurrences of pygame.sprite.Sprite
).
This also allows you to eliminate the comment, since the code is now self-documenting.
max_angle = math.atan(max_height/distance_from_border) | ||
|
||
self.angle = random.randint(int(min_angle), int(max_angle)) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle a specific exception.
In this specific case, use math.atan2
instead of math.atan
. Then there's no chance of an exception, and there's no numerical instability when distance_from_border
is close, but not equal, to 0.0.
|
||
y_speed = random.randint(30,43) | ||
add_y = math.cos(self.time) *y_speed | ||
self.y -= add_y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going beyond: there's a lot of code that does something to x, and then the same thing to y. You can use numpy vectors to reduce the redundancy (at the cost of using a more complicated data structure).
self.screen.blit(self.image, (self.x, self.y)) | ||
|
||
def checkCollision(self, other): | ||
# returns True or False if apple has collided with other object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this comment describes the specification of the method, it could actually be a docstring. Advantages: signals to the reader that this describes what the code does, not how it works; will show up in generated documentation.
No description provided.