Conversation
|
please include the output file in the checkin |
|
having so many private methods, IMO, means you should have a lot more tests. your only public method hits every one of the private ones, and since you can't test them individually, i think you need to try and figure out ways to test them. |
|
You should not test your private methods! http://www.youtube.com/watch?v=URSWYvyc42M https://gist.github.com/jamesgary/5491390 |
|
yeah, yeah,.. .i know you don't test private methods. i meant you do so little in initialize and download_file, that you need to pass in more edge cases to those methods so you exercise all of the private methods for error conditions. |
bin/analyze.rb
Outdated
There was a problem hiding this comment.
It might have been nice for the ShakespeareAnalyzer initializer to just accept a string, to avoid it being coupled to theDownloader class.
There was a problem hiding this comment.
I initially wanted ShakespeareAnalyzer to be concerned only with the xml. I think a good compromise in the meantime would be to make these names a little clearer. "Downloader" is too loaded a term for what happens.
|
|
There was a problem hiding this comment.
I like xml_provider as a name!
|
Excellent work! Great job isolating responsibilities at the class level (with Downloader), and at the method level (things like in_descending_order). |
6 participants
No description provided.