Turning in my Shakespeare Shtuffs! :D#10
Conversation
poosomooso
left a comment
poosomooso
left a comment
There was a problem hiding this comment.
Awesome project overall! It's a cool concept and looks like fun. I put a lot of comments here but they're mostly pretty surface level; your code looks mostly great. When doing revisions, if you have any questions about my comments, I'd be happy to clarify, since writing is difficult.
MiniProject1.py
Outdated
| #fo = open("ShakespeareSonnets.txt", "r") | ||
| with open('ShakespeareSonnets.txt', 'r') as f: | ||
| read_data = f.read() | ||
| f.closed |
There was a problem hiding this comment.
What is this line for? Are you trying to ensure that the file is closed? There are interesting error throwing ways of doing this, but you probably don't need this line, since the 'with' keyword and the open method together handle file closing.
There was a problem hiding this comment.
Some website told me that it saves memory or something, but I can take it out lol
MiniProject1.py
Outdated
| read_data = f.read() | ||
| f.closed | ||
| sonnetz= read_data.split('\n\n') | ||
| sonnetLine=[0, 1, 2, 3,4, 5,6 ,7, 8, 9, 10, 11, 12, 13] |
There was a problem hiding this comment.
A comment explaining what this line is for would be useful. In all fairness, you do a pretty good job of explaining your code with comments for most of the rest of everything :^)
There was a problem hiding this comment.
Comment on your comment: It's adorable that your emoji has a nose!
MiniProject1.py
Outdated
| f.closed | ||
| sonnetz= read_data.split('\n\n') | ||
| sonnetLine=[0, 1, 2, 3,4, 5,6 ,7, 8, 9, 10, 11, 12, 13] | ||
| import random |
There was a problem hiding this comment.
A little thing, but try to keep all your import statements at the top. That way, someone just casually reading through your code will know what libraries this code needs right away.
MiniProject1.py
Outdated
| for k in range(0,len(sonnetz)-1): | ||
| lines=sonnetz[k].split('\n') #splits into lines | ||
|
|
||
| def DictRhymes(): |
There was a problem hiding this comment.
Nice job modularizing/functionizing your code! So clean. I would recommend putting docstrings or some sort of comment under the 'def ' line, just saying what the method does, what parameters it uses, and what it returns (if it returns). Also, typically you want to have your methods outside of for loops, so they can be more generalized.
There was a problem hiding this comment.
If there's a method only defined inside of a for loop, that's awful coding hygiene and I'm deeply ashamed and going to fix that, cuz my mama didn't raise me to write code like that.
MiniProject1.py
Outdated
| def getRhyme(num1, num2): #num1 and num2 inputs represent which lines are selected (rhyming lines) | ||
| lastword1=lines[num1].split(' ')[-1] #takes the last word of every other line | ||
| lastword2=lines[num2].split(' ')[-1] | ||
| if(sentiment(lastword1)[0]<=.25 and sentiment(lastword2)[0]<=.25): |
There was a problem hiding this comment.
Try not to use 'magic numbers' such as .25. Instead, make a variable whose name explains why the number is significant (for example, 'depressing_sentiment = .25'). This goes for pretty much any number where it's not completely obvious your intentions (like 0 or -1 are not magic numbers). An exception would be when you call 'getRhyme' 6 times--you probably shouldn't make variables for all those line numbers. However, you should probably comment there why those numbers you chose are significant (ie. A sonnet rhyme scheme always rhymes these specific line numbers)
There was a problem hiding this comment.
Also in the case of the .25, having it as a variable at the top of your file or something would be really useful, so if you ever wanted to change it to .5 or something, you only have to change one variable, instead of the three times you use that number
There was a problem hiding this comment.
Low key I'm cutting this entire part out of my code casually...
MiniProject1.py
Outdated
|
|
||
| def getMarkLine(num1): #num1 is the line that is getting processed | ||
| splitLine=lines[num1].split(' ') | ||
| for i in range (1, len(splitLine)): |
There was a problem hiding this comment.
This is a really cool way of writing a loop that goes backwards that I honestly have never thought of before....
|
|
||
| def genLine(line1, line2): | ||
| startLine1=random.choice(rhymeWords.keys()) #picks a random rhyming word**' | ||
| startLine2=rhymeWords[startLine1][random.randint(0,len(rhymeWords[startLine1])-1)] #picks a random rhyme to rhyme with the first rhyme |
There was a problem hiding this comment.
I'm pretty sure random.choice works on a list, and rhymeWords[startLine1] is a list. Just a thought to makes this line slightly more readable and shorter. This also goes for when you are trying to pick random previous words a little farther down. But what you have here definitely works.
There was a problem hiding this comment.
I'm not sure that I get this one!
|
Thanks bae <3 <3 From: Serena Chen [mailto:notifications@github.com] @poosomooso commented on this pull request. Awesome project overall! It's a cool concept and looks like fun. I put a lot of comments here but they're mostly pretty surface level; your code looks mostly great. When doing revisions, if you have any questions about my comments, I'd be happy to clarify, since writing is difficult. In MiniProject1.pyhttps://github.com//pull/10#pullrequestreview-2993400:
+# Kimber's file +from pattern.en import * +rhymeWords=dict() #dictionary of words that rhyme +generalWords=dict() #markov chain dictionary +#fo = open("ShakespeareSonnets.txt", "r") +with open('ShakespeareSonnets.txt', 'r') as f:
+f.closed What is this line for? Are you trying to ensure that the file is closed? There are interesting error throwing ways of doing this, but you probably don't need this line, since the 'with' keyword and the open method together handle file closing. In MiniProject1.pyhttps://github.com//pull/10#pullrequestreview-2993400:
+# Kimber's file +from pattern.en import * +rhymeWords=dict() #dictionary of words that rhyme +generalWords=dict() #markov chain dictionary +#fo = open("ShakespeareSonnets.txt", "r") +with open('ShakespeareSonnets.txt', 'r') as f:
+f.closed +sonnetz= read_data.split('\n\n') +sonnetLine=[0, 1, 2, 3,4, 5,6 ,7, 8, 9, 10, 11, 12, 13] A comment explaining what this line is for would be useful. In all fairness, you do a pretty good job of explaining your code with comments for most of the rest of everything :^) In MiniProject1.pyhttps://github.com//pull/10#pullrequestreview-2993400:
+# Kimber's file +from pattern.en import * +rhymeWords=dict() #dictionary of words that rhyme +generalWords=dict() #markov chain dictionary +#fo = open("ShakespeareSonnets.txt", "r") +with open('ShakespeareSonnets.txt', 'r') as f:
+f.closed +sonnetz= read_data.split('\n\n') +sonnetLine=[0, 1, 2, 3,4, 5,6 ,7, 8, 9, 10, 11, 12, 13] +import random A little thing, but try to keep all your import statements at the top. That way, someone just casually reading through your code will know what libraries this code needs right away. In MiniProject1.pyhttps://github.com//pull/10#pullrequestreview-2993400:
+from pattern.en import * +rhymeWords=dict() #dictionary of words that rhyme +generalWords=dict() #markov chain dictionary +#fo = open("ShakespeareSonnets.txt", "r") +with open('ShakespeareSonnets.txt', 'r') as f:
+f.closed +sonnetz= read_data.split('\n\n') +sonnetLine=[0, 1, 2, 3,4, 5,6 ,7, 8, 9, 10, 11, 12, 13] +import random +for k in range(0,len(sonnetz)-1):
Nice job modularizing/functionizing your code! So clean. I would recommend putting docstrings or some sort of comment under the 'def ' line, just saying what the method does, what parameters it uses, and what it returns (if it returns). Also, typically you want to have your methods outside of for loops, so they can be more generalized. In MiniProject1.pyhttps://github.com//pull/10#pullrequestreview-2993400:
Try not to use 'magic numbers' such as .25. Instead, make a variable whose name explains why the number is significant (for example, 'depressing_sentiment = .25'). This goes for pretty much any number where it's not completely obvious your intentions (like 0 or -1 are not magic numbers). An exception would be when you call 'getRhyme' 6 times--you probably shouldn't make variables for all those line numbers. However, you should probably comment there why those numbers you chose are significant (ie. A sonnet rhyme scheme always rhymes these specific line numbers) In MiniProject1.pyhttps://github.com//pull/10#pullrequestreview-2993400:
This is a really cool way of writing a loop that goes backwards that I honestly have never thought of before.... In MiniProject1.pyhttps://github.com//pull/10#pullrequestreview-2993400:
Also in the case of the .25, having it as a variable at the top of your file or something would be really useful, so if you ever wanted to change it to .5 or something, you only have to change one variable, instead of the three times you use that number In MiniProject1.pyhttps://github.com//pull/10#pullrequestreview-2993400:
+def genLine(line1, line2):
I'm pretty sure random.choice works on a list, and rhymeWords[startLine1] is a list. Just a thought to makes this line slightly more readable and shorter. This also goes for when you are trying to pick random previous words a little farther down. But what you have here definitely works. — |
|
@kwinter213 Just make your changes in your forked version, and then make another pull request! Revisions are due on Monday |
|
Hey Kimber! Some comments. Did you test your code after making the revisions? There are a couple of bugs, mostly from variables that don't exist in certain scopes. The first thing is that functions need to be defined before you call them. Specifically in the example where you have the for loop starting in line 16: for k in range(0,len(sonnetz)-1): That throws an error because you have your 'def DictRhymes' and 'def getMarkov' afterwards. It's a problem with how python is interpreted. Another variables issue that I see is that the getRhyme function (and markLine) is trying to get sonnet lines from the list 'lines'. However, lines doesn't exist! I assume you use 'lines' because you copied the method from the above for loop to outside the for loop. However, you need to pass in the list of sonnet lines you want to look at, or make lines a variable in the very outside scope (outside the for loop). I prefer the former, because then, when you call DictRhymes and getRhyme, it's not dependent on the list 'lines' being up to date, so it's more generalized. I don't know if this paragraph makes any sense; if it doesn't I'm happy to talk about it in person. I think I found your bug in the rhyming words. You have 'split('\n\n')', which splits at every two lines endings. But If you have two empty lines, you actually have three new lines--one at the end of the last line of the previous sonnet, one for the first empty line, and one for the second empty line. Since you were splitting on two new-line characters (the \n), it kept the second empty line at the beginning of the next sonnet, so all your lines were off by one. Tricky bug :/ One more thing--you have great comments, but I want to suggest using docstrings instead. For example, instead of: def DictRhymes(lines): do: def DictRhymes(lines): Which is more of a standard python docstring. Doesn't seem super important, but it's a standard, and people reading your code will be looking for those docstrings. Also, doctests work in them, so that's nice. But overall, awesome project! |
2 participants
No description provided.