Skip to content

MP3 feedback#4

Open
sarahwalters wants to merge 1 commit intomasterfrom
mp3-feedback
Open

MP3 feedback#4
sarahwalters wants to merge 1 commit intomasterfrom
mp3-feedback

Conversation

@sarahwalters
Copy link
Collaborator

Functionality: 5
Documentation: 5
Style: 4.5
Total: 4.8/5

This looks great! The MVC separation is clean -- each of your classes/functions does one thing, and it's easy to tell how they fit together.

A couple overarching comments...

  • You could parameterize a little more -- you're using a bunch of "magic numbers" for positioning, so everything will look wrong if you change the size of your screen. Computing the positioning numbers based on screen size would make your code easier to modify -- always a good thing.
  • Your ifmain in swim_little_fish_swim.py is a little heavy -- I think a lot of the code there could be abstracted away into the model, leaving you with a cleaner and more maintainable implementation.

Look through the files changed to see more comments about specifics in your code. Feel free to respond to the comments inline -- more than happy to have a conversation.

Once you've read the comments, make a comment to the pull request letting us know that you've checked them out. Afterwards, close the PR.

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.

1 participant