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

Project Submission #4

Open
wants to merge 66 commits into
base: master
Choose a base branch
from
Open

Project Submission #4

wants to merge 66 commits into from

Conversation

iblancett
Copy link

No description provided.

sokuno222 and others added 30 commits October 18, 2017 20:26
…l need to clear screen so text goes away after not over icon.
Copy link

@Elepert Elepert left a 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
Copy link

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:
Copy link

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
Copy link

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
Copy link

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

Copy link

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

Copy link

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)
Copy link
Contributor

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 after Course() and before new_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)
Copy link
Contributor

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))
Copy link
Contributor

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
Copy link
Contributor

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):
Copy link
Contributor

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
Copy link
Contributor

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))
Copy link
Contributor

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:
Copy link
Contributor

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'''
Copy link
Contributor

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
Copy link
Contributor

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()):
Copy link
Contributor

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:
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants