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

turning in TM #64

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

turning in TM #64

wants to merge 2 commits into from

Conversation

KatieButler
Copy link

I'm sorry this was turned in at 2:00 not 12:00
The important ones are kpop.py,kpop_lyrics.py, frequency, and Project Statement

Review on Reviewable

@srli
Copy link

srli commented May 5, 2016

Review status: 0 of 94 files reviewed at latest revision, 9 unresolved discussions.


a discussion (no related file):
Nicely done. kpop.py is fairly similar to kpop_lyrics.py, so I've made a comments in places where improvements could be made on the latter only.

In the future, try to organize your Git repo so it's clear which files are important. The txt files with the lyrics in them should be placed in a "data" folder, and the unused python files should be either deleted or placed in a "misc" folder. Please remember to delete unused code as you go, so it's easier to debug and remember which components of the code are important at any given time.

Typically, functions are defined together at the top of the file, then the necessary functions are called one by one in order under an "if name == main" conditional at the end of the file. This keeps important components together, increasing readability.

All in all, good job. Looking forward to seeing what you come up with in the future. :)


kpop_lyrics.py, line 28 [r1] (raw file):
? Assuming this is an incomplete docstring?


kpop_lyrics.py, line 30 [r1] (raw file):
This doesn't need to be a function since you're only doing one operation on the word. It could be done in-line when you use it later


kpop_lyrics.py, line 33 [r1] (raw file):
Try to use more descriptive variable and function names, or more in-line documentation since it's difficult to understand what these variables stand for on a first glance.


kpop_lyrics.py, line 47 [r1] (raw file):
be sure to delete unused code for final turn ins


kpop_lyrics.py, line 63 [r1] (raw file):
What does this mean? What index?


kpop_lyrics.py, line 64 [r1] (raw file):
There's an elif for numb == 0 vs. if numb is any other number, though these can be combined as the operations done in either conditional are mostly the same.


kpop_lyrics.py, line 65 [r1] (raw file):
You can also just open("lyrics0.txt")


kpop_lyrics.py, line 82 [r1] (raw file):
num_songs will be the same in every iteration of this for loop.
I think what you want to do with check if j is 14 or 40.


Comments from Reviewable

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.

2 participants