Attempt by Patrik Bóna #3
Conversation
* create Gemfile and add rspec gem * create empty spec dir * set some default options in .rspec
* URI content is stored in @file_content
* with speaker method in XmlParser object
Declared PERSONAS don't mach SPEAKER in SPEECHEs, for example <PERSONA>Three Witches.</PERSONA> vs. <SPEAKER>First Witch</SPEAKER> ... whole xml: http://www.ibiblio.org/xml/examples/shakespeare/macbeth.xml
* run it with `ruby lib/shakespeare_analyzer.rb`
* refactor `speakers` method to one line * rename `lines_by_speaker` to `count_lines_by_speaker`
Almost all methods are now one line long.
output.txt
Outdated
There was a problem hiding this comment.
Even after fixing the bug in my code, I am not getting a number this high for Macbeth. Do we know what the actual answer is?
There was a problem hiding this comment.
Well I do not know what are the correct numbers ;).
I perform my tests on small test.xml file and for this file everything is calculated correctly. But I do not know if I do not miss some edge cases in XML.
There was a problem hiding this comment.
Well I just found bug in my code, I need to fix it. Hopefully then we will have same numbers ;).
There was a problem hiding this comment.
Bug fixed, now we have same numbers for Macbeth ;)
`count_lines_by_speaker` is now looking for exact match in speaker's
name.
Example XML:
<SPEECH>
<SPEAKER>A</SPEAKER>
<LINE>Line!</LINE>
</SPEECH>
<SPEECH>
<SPEAKER>AAA</SPEAKER>
<LINE>Line!</LINE>
</SPEECH>
before:
count_lines_by_speaker('A') == 2
now:
count_lines_by_speaker('A') == 1
|
Hi, Patrik... Nice work! You might want to consider updating the output so that the names aren't upper-cased. |
There was a problem hiding this comment.
I think it would be good to avoid passing a URL to the initializer, and just provide the data instead. That means you could supply text from a file, and it should also make testing easier.
|
@andyw8 thanks for all your comments. They all seems valid to me and I will keep them in mind for my next projects. Thanks! |
There was a problem hiding this comment.
You're one of the only folks to extract this into a constant. I like it. :)
There was a problem hiding this comment.
Nice high level of abstraction.
7 participants
I am afraid that I am not able to do more refactoring, so here is my pull request.