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

TextMining Submission #61

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

Conversation

jaredbriskman
Copy link

Sorry this is late. Here's my text mining project.

Review on Reviewable

@phuston
Copy link

phuston commented Mar 18, 2016

Review status: 0 of 20 files reviewed at latest revision, 16 unresolved discussions.


generate.py, line 1 [r1] (raw file):
Header comments, header comments, header comments!


generate.py, line 35 [r1] (raw file):
+1 for generators - awesome


TextMining.pdf, line 0 [r1] (raw file):
Overall, your writeup looks solid. Great analysis of what you could do for future projects.

Your implementation shows a strong attention to detail and a lot of thoughtfulness in terms of design and modularity. Your testing was super solid, awesome to see more legit + expansive test coverage than doctests. I also really like your implementation of sql - awesome to see you going above and beyond.

One area you can work on is in documentation - this includes header comments, docstrings, and inline comments. While your code is very well-written, including comments and headers will make it so much easier for us to go about grading and understanding your code.

Looking forward to seeing future projects from you!


markov-text-master/db.py, line 2 [r1] (raw file):
Cool abstraction to a sql database - I would add more comments about what you're doing + more docstrings as well for each function


markov-text-master/db.py, line 11 [r1] (raw file):
I like your separation of sql commands into the Sql class - makes for much cleaner, more readable code


markov-text-master/gen.py, line 1 [r1] (raw file):
Header comments - this would make it easier for someone reading your code to go through and quickly understand how everything works together


markov-text-master/gen.py, line 3 [r1] (raw file):
Looks good - again, more comments + docstrings


markov-text-master/markov.py, line 1 [r1] (raw file):
Great design + code modularity - it really seems like you put some thought into how everything would work together. Headers at the top of your files would be good though


markov-text-master/markov.py, line 14 [r1] (raw file):
Awesome. This extra level of polish in being able to run it as a command line utility is super cool. Great idea to have all functionality run with one command and different options


markov-text-master/parse.py, line 1 [r1] (raw file):
With the modularity you have going on, a header at the top of every file would be good to make it easier to read and go through. This should include what it does


markov-text-master/parse.py, line 4 [r1] (raw file):
Again, more documentation! While it's pretty clear what this class does by its name 'parser', it'd be good to explain in more detail its responsibilities


markov-text-master/parse.py, line 13 [r1] (raw file):
Great abstraction


markov-text-master/parse.py, line 15 [r1] (raw file):
Nice, clean implementation. Good work


markov-text-master/sql.py, line 1 [r1] (raw file):
Great abstraction of all sql commands - I like this design decision a lot


markov-text-master/sql.py, line 18 [r1] (raw file):
Quick comment on this method of string formatting - sometimes, the .format() method can be a little cleaner + easier to read


markov-text-master/test/suite.py, line 7 [r1] (raw file):
Awesome testing suite - very well done. The only comment I have is adding more comments as to what you're testing for - that would make it much easier to understand from an outside perspective.


Comments from the review on Reviewable.io

@phuston
Copy link

phuston commented Mar 22, 2016

Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, 16 unresolved discussions.


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