-
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
Project Submission #4
base: master
Are you sure you want to change the base?
Conversation
…to implement icon images
…l need to clear screen so text goes away after not over icon.
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 job! Your overall code looks clean. Your documentation was really thorough. The biggest improvement you could make is reorganizing your files to make some functions into class methods rather than free floating and organize some free floating code into functions/methods.
# Author: Isa Blancett | ||
# Project: Interactive Programming | ||
# Date: 10.30.2017 | ||
# Description: Class for reading in a csv file containing course information and |
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 job with the header comments on the files!
class Courses(object): | ||
""" class for the different course options the user can choose from | ||
|
||
contains: |
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 job explaining what all of these are. An extra step you could do is specifically write what type these will end up being (ie: string, int, etc..). It's not necessary for this project, but it's good practice when you're making data structures like these.
@@ -0,0 +1,124 @@ | |||
# Author: Isa Blancett |
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 job with the unit testing!
from ending import ending | ||
from environment import * | ||
|
||
# INITIALIZE GAME |
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 code below should be in a function or a class. It's always good practice to wrap things up and not have them free floating in a file.
import pygame | ||
import os | ||
import sys | ||
|
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 would put all the functions below into a class for the screen. You're changing an object in your game essentially which is when classes are really nice :)
|
||
import pygame | ||
import sys | ||
|
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 wonder if these functions could be put into the GamePlay class. They seem to be important parts of it. Rather than importing into the file, adding them to the class makes more sense
""" | ||
|
||
new_course = Courses() | ||
new_course.setup(config) |
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.
Consider changing Courses.setup
to Courses.__init__
, and changing these two lines to new_course == Courses(config)
.
The principles are:
- Code that requires the client to perform a sequence of calls in order is fragile.
- Reduce the amount of time that an object is in an unusable state. It doesn't make sense to use
new_course
afterCourse()
and beforenew_course.setup(…)
, so shrink the period during which the class is exposed in this state. - Make it impossible (or harder) for the client of a class or module to mis-use it.
This principles all come to the same thing here; I'm listing them as separate principles because they're all good design principles, and they don't always apply to the same situations.
In this case, replacing Courses.setup
by Courses.__init__
allows you to remove create_course
entirely, and replace calls to create_course(config)
by Courses(config)
.
|
||
new_course = Courses() | ||
new_course.setup(config) | ||
return(new_course) |
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.
return
is a statement, not a function; the parentheses aren't necessary (or idiomatic).
row['dependencies'] = row['dependencies'].split() | ||
else: | ||
row['dependencies'] = [] | ||
new_course = (create_course(row)) |
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.
outer parens aren't necessary
'''Turns screen a color by filling in with rectangles.''' | ||
for i in range(700): | ||
pygame.draw.rect(screen, color, (0,i,1000,i+1)) | ||
i+=1 |
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 i+=1
line doesn't do anything, since the next time through the loop resets i
import pygame | ||
import sys | ||
|
||
def screenScroll(screen, color): |
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 Python convention is screen_scroll
.
screenScroll(screen, (0,0,0)) | ||
screen.blit(font50.render("Looks like your portfolio", 1, (255,255,255)), (300, 250)) | ||
screen.blit(font50.render("could use some improvement", 1, (255,255,255)), (280, 350)) | ||
Done = False |
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 convention for variables is lowercase.
if event.type == pygame.QUIT: | ||
Done = True | ||
for i in range(len(levels)): | ||
screen.blit(font20.render(levels[i], 1, (200,200,200)), (100, 450+i*25)) |
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 also use:
for i, level in enumerate(levels):
screen.blit(font20.render(level, 1, (200,200,200)), (100, 450+i*25))
for i in range(len(levels)): | ||
screen.blit(font20.render(levels[i], 1, (200,200,200)), (100, 450+i*25)) | ||
pygame.display.flip() | ||
if victory == 1: |
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 looks like victory
is used a boolean? If so, prefer the boolean type: True
/False
, instead of 1
/0
. This is clearer in intent.
def determine(victory, levels): | ||
'''Determines ending. | ||
Victory is value 0, 1, or 2. 0 is bad, 2 is perfect. | ||
Levels is a list of strings. Print out with for loop''' |
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.
For code that documents itself without as many explicit docstrings and comments, consider:
BAD_OUTCOME = 'bad'
MIDDLE_OUTCOME = 'middle'
PERFECT_OUTCOME = 'perfect'
…
if victory == MIDDLE_OUTCOME:
…
This has the advantages that it's easy to see the intent each place the value is used, without having to find the nearest comment that describes it; that you don't need to repeat comments in each function that generates or uses these values (admittedly, not that many places in this example); that printing out victory
is more helpful even if you've forgotten what represents what; and that you're better protected against a certain class of errors, for example using 4
as a victory value.
FMatSci_Icon = pygame.transform.scale(SMatSci_Icon, (108, 100)) | ||
SMechProto_Icon = pygame.image.load('Source/Course_Icons/mech.png') | ||
FMechProto_Icon = pygame.transform.scale(SMechProto_Icon, (108, 100)) | ||
#basic background |
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.
To take this to the next level, consider replacing these separate variables by a list.
'''This function looks for the click of a mouse and queues an event based on where the mouse is''' | ||
(x,y) = pygame.mouse.get_pos() | ||
clicked_path = 'Source/Course_Icons/%s_clicked.png' | ||
if y >= 550 and y<= 655 and any(pygame.mouse.get_pressed()): |
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.
Python tip: you can also use if 550 <= y <= 655
(x,y) = pygame.mouse.get_pos() | ||
clicked_path = 'Source/Course_Icons/%s_clicked.png' | ||
if y >= 550 and y<= 655 and any(pygame.mouse.get_pressed()): | ||
if x >= 51 and x<= 159 and 'bees' not in order: |
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.
Where do these numbers come from? Can they come from a list of entities and their locations?
No description provided.