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

nielsen text mining #62

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

Conversation

nielseneli
Copy link

Review on Reviewable

@rdiverdi
Copy link

Review status: 0 of 114 files reviewed at latest revision, 14 unresolved discussions.


.gitignore, line 1 [r1] (raw file):
Congratulations, you made a .gitignore, it would have been awesome to use it so that I didn't have to look through 114 files when I care about approximately three of them (sorry for being sassy, I gave a reasonable comment to the first 3 people who did this)


import_hamilton.py, line 14 [r1] (raw file):
You should tab in these comments, it makes your code easier to read


import_hamilton.py, line 27 [r1] (raw file):
This would be a good place to use list comprehension


import_hamilton.py, line 33 [r1] (raw file):
Where did your pretty doc strings go?


import_hamilton.py, line 47 [r1] (raw file):
The stuff below here should be in other functions, also any code that is running in the main body of a script should be under an if __name__ == '__main__'


import_hamilton2.py, line 50 [r1] (raw file):
Is this the only difference between this and the last file?

if so, it should be an input to a function


parsing_text_files.py, line 1 [r1] (raw file):
Everything you have looks pretty good, although some of your variable and function names are questionable. It looks like you have a pretty good grasp of the programming concepts you used and generally did things efficiently.
There were some spots where you had repeated code, and there is a lot of code outside of functions that should really be organized better.
Also, you commented functions at the beginning really well, but you stopped half way through, and there were a few places where inline comments for blocks of code would have been really helpful.


parsing_text_files.py, line 10 [r1] (raw file):
this isn't particularly descriptive


parsing_text_files.py, line 14 [r1] (raw file):
important? really?


parsing_text_files.py, line 42 [r1] (raw file):
...


parsing_text_files.py, line 69 [r1] (raw file):
Make a main function to do all of the things
Also, what is 46?


parsing_text_files.py, line 74 [r1] (raw file):
This is a completely ridiculous function call, you should split it up and comment it


parsing_text_files.py, line 75 [r1] (raw file):
why do I care about the number 9?


parsing_text_files.py, line 83 [r1] (raw file):
don't you have a function for this?


Comments from the review on Reviewable.io

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