Pull request for Shakespeare Analyzer challenge #5
Conversation
|
Yep; back to the drawing board... I just realized that I'm supposed to count LINES of a speech, not just the number of times spoken. Thanks for pointing that out. |
Was counting SPEAKER occurrences when needed to count LINES of each speech.
|
OK; now I'm counting lines of a speech... |
Holding off on the 'ALL' speaker as don't know who to reliably tell who's on stage at the time it's used
|
@JESii When first looking at your code, it did not appear that the check_input or get_http_file methods were being used. Clearly, that was my mistake, but I was curious about how you decided where to split the startup logic between lib/shakespeare_analyzer.rb and bin/shakespeare_analyzer. Since the validation takes place in bin/shakespeare_analyzer perhaps all the validation logic could be localized in one place by moving over check_input / get_http_file? Another option could be to have the initialize method be responsible for calling file validation, and throw an exception if the input was invalid and/or when analyze is called on invalid input. To me, it seems logical for the ShakespeareAnalyzer class to own its input validation and ensure it is run. As a bonus, that keeps all the code in one place and would just leave bin/shakespeare_analyzer as a shell to call ruby lib/shakespeare_analyzer.rb. |
|
Great comments/questions, @briantemple ... thank you so much! re: e058977 lib/shakespeare_analyzer.rb:L59 - Thanks for that Hash.new() usage; I'll definitely remember that! re; e058977 lib/shakespeare_analyzer.rb:L56 - I looked at the 'ALL' usage and it depends on who's actually in the room; that means we'd have to find all the elements, search for the word 'Enter' and then pull the names of the folks who entered. We'd also have to look for the Acts/Scenes, because all characters leave at the end (Exeunt frequently). However, since it's not nicely formatted with an XML element, I steered away from that... That would really raise the complexity, IMO. re: check_input / get_http_file - I actually pondered that a bit and wasn't sure if I'd come up with the best approach. I started off with everything all in the class... pretty procedural. Then, once I get it all working for the basic tests, I refactored out. I like your thought about the class 'owning' it's input validation. I had actually started to do that in the initialize method and moved it... I think I'll go back and review that section. re: 352c76c lib/shakespeare_analyzer.rb:L42 - I see what you mean; however, I'm kind of a fan of 'nil' for things where I can't do anything... such as the case of no/wrong file. I've always felt that it left open a greater number of options. It may also be a holdover from my Perl background where the definition of true/false was broader than for Ruby. Again, thank you for the thoughtful comments. |
lib/shakespeare_analyzer.rb
Outdated
There was a problem hiding this comment.
Wow, didn't see this coming when I was doing the homework. Anyway, how did you discover this? It would be awesome if your tests somehow reflected this (i.e. you tease these things out by TDD, although not sure how that would be done).
There was a problem hiding this comment.
I have a habit of reviewing the incoming data, separately from any TDD activities... gives me a feel for what things really look like and also helps me set up appropriate sample data. And so I found this condition and scanned the file to see what it looked like. My assessment was that the situation was not well-defined: I'd have had to parse the test in the directives to see who entered/left the scene, and also processed the directives to set actors on stage to nil. That's a very complex situation and not guaranteed to be correct. briantemple also raised that question and we got the word back from Thoughtbot to ignore that case.
Per suggestion from brian, move file validation/access into the initialize method; now when using the class, you just instantiate it, call analyze to parse the file, and then call the list_by... method to print the output. Less dependence on the caller doing the right thing.
lib/shakespeare_analyzer.rb
Outdated
There was a problem hiding this comment.
Lots of return points from this method. Makes me raise my eyebrows a bit.
There was a problem hiding this comment.
I try to avoid doing almost any work in initialize methods.
|
Good effort! Overall thoughts: you're on the right track, but I'd like to see you:
|
|
Ben... Thank you for the incredible amount of detailed feedback... Wow! I'll take a look over the weekend, refactor, and let you see what I come up with. Btw, great presentation at gogaruco! Cheers... Jon Sent from my iPhone On Sep 26, 2013, at 1:13 PM, Ben Orenstein [email protected] wrote:
|
|
Glad it helped! Good luck with the refactoring. On Thu, Sep 26, 2013 at 4:33 PM, Jon Seidel [email protected]:
|
|
Thanks again for the detailed feedback, Ben; definitely something for me to work on. I do have one issue: You mentioned that you thought I was “a little paranoid” and should worry about errors less. I don’t agree with that. I learned a long time ago to practice defensive programming which has at least two benefits:
Cheers...jon Jon Seidel, CMC® Effective Decisions... Priceless! From: Jon Seidel [mailto:[email protected]] Ben... Thank you for the incredible amount of detailed feedback... Wow! I'll take a look over the weekend, refactor, and let you see what I come up with. Btw, great presentation at gogaruco! Cheers... Jon Sent from my iPhone On Sep 26, 2013, at 1:13 PM, Ben Orenstein [email protected] wrote: Good effort! Overall thoughts: you're on the right track, but I'd like to see you:
— |
|
I think we'll have to agree to disagree here. "As a developer, I don’t get surprised when something blows up due to "As the end-user, at least there’s any explanatory message telling me what On Thu, Oct 3, 2013 at 8:48 PM, Jon Seidel [email protected] wrote:
|
|
By the way, I think you'd like this book: http://www.confidentruby.com/ On Fri, Oct 4, 2013 at 10:49 PM, Ben Orenstein [email protected] wrote:
|
|
Fair enough, Ben... I hear what you’re saying and I will keep it in mind. Cheers...jon Jon Seidel, CMC® Effective Decisions... Priceless! From: Ben Orenstein [mailto:[email protected]] I think we'll have to agree to disagree here. "As a developer, I don’t get surprised when something blows up due to "As the end-user, at least there’s any explanatory message telling me what On Thu, Oct 3, 2013 at 8:48 PM, Jon Seidel [email protected] wrote:
— |
Remove unnecessary start/end processing messages Have empty file simply return and empty result so it's handled without exception.
Use <<-EOF instead of <<EOF for greater clarity. Correct order of result lines '2 persona, no speaking' test.
Cleanup <<-EOF spacing for improved readability Changed commented-out test to use 'xit' so that it shows as pending.
Replace hacky initialize file handling code with a checked_file_for_open() method which returns a @file variable which can be safely passed to the Nokogiri/open handling. If an unreadable file, then we raise an error. Able to remove much of the old initialize code entirely and better show the intent of the analyze method.
|
First off, Ben, thanks very much for the link to Confident Ruby – I definitely liked Avdi’s presentation and am buying the book/video package. Secondly, I better understand where your previous comments came from. Looking at the Confident Ruby stuff, it seems clear to me that error handling is still there, it’s just handled in a much clearer fashion than I was doing. I still believe that checking for error conditions is A Good Thing, but now it’s encapsulated and doesn’t get in the way of the overall flow. I updated much of the file-handling in my latest push to github, using a checked_file_for_open() method which handles the various options, throws an appropriate error, and incidentally cleaned out a lot of the initialize() cruft. I’d appreciate your thoughts on the updates; I plan to do more from what I’ve learned from the book. Thanks again...jon Jon Seidel, CMC® Effective Decisions... Priceless! From: Ben Orenstein [mailto:[email protected]] By the way, I think you'd like this book: http://www.confidentruby.com/ On Fri, Oct 4, 2013 at 10:49 PM, Ben Orenstein [email protected] wrote:
— |
Raise 'Unreadable file' if error getting http file Minor cleanup on get_http_file method.
Remove stub code for speaker=ALL; not handled. Remove default file=nil in initialize.
|
Hey Jon, I think this is getting better, but I still see lots of error checking Keep reading and pushing. :) On Sat, Oct 5, 2013 at 2:55 PM, Jon Seidel [email protected] wrote:
|
|
Thanks, Ben... I only did the one cleanup to see how it would work; I like it! I’ll work on more now :=) Cheers...jon Jon Seidel, CMC® Effective Decisions... Priceless! From: Ben Orenstein [mailto:[email protected]] Hey Jon, I think this is getting better, but I still see lots of error checking Keep reading and pushing. :) |
|
Ah, sorry. I misread. Didn't realize you'd just done the one. Onward! |
5 participants
File 'macbeth.output' file contains output from my program. I'm looking forward to your feedback!