-
-
Notifications
You must be signed in to change notification settings - Fork 50
Analyzer attempt by Dave West (@twosevenero) #2
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
base: master
Are you sure you want to change the base?
Changes from all commits
26a93b6
20aeacf
4225136
ab6d345
ebf8b3d
a168fbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| --color | ||
| --format progress |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| require_relative "../lib/xml_parser" | ||
| require_relative "../lib/shakespeare_analyzer" | ||
|
|
||
| analyzer = ShakespeareAnalyzer.new | ||
| puts analyzer.start |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| require_relative = "../lib/xml_parser" | ||
|
|
||
| class ShakespeareAnalyzer | ||
|
|
||
| MACBETH_XML = "macbeth.xml" | ||
|
|
||
| attr_accessor :xml | ||
|
|
||
| def initialize xml = MACBETH_XML | ||
| @xml = xml | ||
| end | ||
|
|
||
| def start | ||
| @doc = XmlParser.new(@xml).create_rexml_document | ||
| create_speakers_and_lines_hash | ||
| sort_speakers_by_number_of_lines | ||
| print_speakers_and_lines | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def create_speakers_and_lines_hash | ||
| @speakers = Hash.new(0) | ||
| @doc.elements.each("PLAY/ACT/SCENE/SPEECH") do |speech| | ||
|
|
||
| @line_count = 0 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you clarify why this variable is an instance variable instead of a local variable? (Is it an optimization to prevent re-allocation of the variable for every execution of the loop?) |
||
| speech.elements.each("LINE") do |line| | ||
| @line_count += 1 | ||
| end | ||
|
|
||
| @characters = Array.new | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps these two blocks could be combined, since @characters is not used outside this block? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way, |
||
| speech.elements.each("SPEAKER") do |speaker| | ||
| @characters << speaker.text | ||
| end | ||
|
|
||
| @characters.each do |character| | ||
| @speakers[character] += @line_count | ||
| end | ||
|
|
||
| end | ||
| end | ||
|
|
||
| def sort_speakers_by_number_of_lines | ||
| @speakers = @speakers.sort_by { | key, value | value }.reverse | ||
| end | ||
|
|
||
| def print_speakers_and_lines | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this method name is a little misleading. It returns a string, but doesn't actually 'print' it (e.g. to stdout).
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree.. Originally I think I was using puts here but you are right. Thanks |
||
| output = String.new | ||
| @speakers.each do |name,number_of_lines| | ||
| output << "#{number_of_lines}\t#{name}\n" | ||
| end | ||
|
|
||
| output | ||
| end | ||
|
|
||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| require 'net/http' | ||
| require 'rexml/document' | ||
|
|
||
| class XmlParser | ||
|
|
||
| def initialize xml | ||
| @xml = xml | ||
| end | ||
|
|
||
| def create_rexml_document | ||
| @doc = REXML::Document.new xml_is_a_file? ? read_xml_file : fetch_raw_xml_data | ||
| end | ||
|
|
||
| def xml_is_a_file? | ||
| File.exist? @xml | ||
| end | ||
|
|
||
| def read_xml_file | ||
| File.read @xml | ||
| end | ||
|
|
||
| def fetch_raw_xml_data | ||
| Net::HTTP.get_response(URI.parse(@xml)).body | ||
| end | ||
|
|
||
| end | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you clear split out the four separate steps required. One suggestion I would make is to extract the first line into it's own method, named something like
read_text_file. Each method call in thestartwould then be at the same level of abstraction, which I think would make things more readable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Thanks for the feedback!