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

Mini Project 3 #6

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

Mini Project 3 #6

wants to merge 16 commits into from

Conversation

hthomas60
Copy link

No description provided.

@@ -1,3 +1,12 @@
# TextMining
To run this program you will need to download the GitHub repository as well as the Natural Language Toolkit (NKTL), the requests package and the Vader sentiment analysis package
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are just generally a few typos in this file, and this sentence is repeated at the bottom. I would suggest putting the lines that should go in the command line between `` because that's good markdown formatting. (ie: pip install vaderSentiment <= would look like => pip install vaderSentiment)

minebooks2.py Outdated
mystring = mystring.replace('-', '')
mystring = mystring.replace('\"', '')
mystring = mystring.replace(',', '')
return mystring.lower() #makes everything lower case words like "The" and "the" are the same
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a cleaner way to do this with a for loop. Make a list of all the characters you want to eliminate and iterate through that list rather than writing down every single character as a distinct line.

minebooks2.py Outdated

"""

return( list(set(list(set(list1).intersection(list2))).intersection(list3)))
Copy link

@Elepert Elepert Oct 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor detail, but there's probably a cleaner way to do this.

minebooks2.py Outdated
tragedies.append('Cymbeline.pickle')
histories = ['King_Henry_IV.pickle']
histories.append('King_John.pickle')
histories.append('King_Richard_II.pickle')
Copy link

@Elepert Elepert Oct 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused about why you appended these strings instead of just writing them out initially.
Do this:
tragedies = ['Antony_and_Cleopatra.pickle', 'Coriolanus.pickle', 'Cymbeline.pickle']
instead of:
tragedies = ['Antony_and_Cleopatra.pickle']
tragedies.append('Coriolanus.pickle')
tragedies.append('Cymbeline.pickle')

@Elepert
Copy link

Elepert commented Oct 16, 2017

Overall, this was a good project! I made a few stylistic notes. There are some areas where you could have used loops instead of writing out things manually. Your documentation was really good and your variable naming was on point. Very minor detail, but you had a few typos in your comments/ReadMe. It's always good to reread and double check your wording.

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

2 participants