-
-
Notifications
You must be signed in to change notification settings - Fork 50
Attempt by Patrik Bóna #3
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
c4bfcc3
6e6ed1a
8bf4aac
30afc47
ef5eece
10e57b9
22fd85a
4de95a0
c992144
e123d20
71030a3
711de4c
3558e6e
ec3c3b4
8f4895f
87bbbf5
b8c4025
c544b4f
387ca94
e25e5a6
b811d05
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,3 @@ | ||
| --format documentation | ||
| --no-profile | ||
| --color |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| source 'https://rubygems.org' | ||
| ruby '2.0.0' | ||
|
|
||
| gem 'rspec' | ||
| gem 'webmock' | ||
| gem 'nokogiri' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| GEM | ||
| remote: https://rubygems.org/ | ||
| specs: | ||
| addressable (2.3.5) | ||
| crack (0.4.1) | ||
| safe_yaml (~> 0.9.0) | ||
| diff-lcs (1.2.4) | ||
| mini_portile (0.5.1) | ||
| nokogiri (1.6.0) | ||
| mini_portile (~> 0.5.0) | ||
| rspec (2.14.1) | ||
| rspec-core (~> 2.14.0) | ||
| rspec-expectations (~> 2.14.0) | ||
| rspec-mocks (~> 2.14.0) | ||
| rspec-core (2.14.5) | ||
| rspec-expectations (2.14.2) | ||
| diff-lcs (>= 1.1.3, < 2.0) | ||
| rspec-mocks (2.14.3) | ||
| safe_yaml (0.9.5) | ||
| webmock (1.13.0) | ||
| addressable (>= 2.2.7) | ||
| crack (>= 0.3.2) | ||
|
|
||
| PLATFORMS | ||
| ruby | ||
|
|
||
| DEPENDENCIES | ||
| nokogiri | ||
| rspec | ||
| webmock |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| #!/usr/bin/env ruby | ||
|
|
||
| require_relative '../lib/shakespeare_analyzer.rb' | ||
|
|
||
| DEFAULT_XML_URL = 'http://www.ibiblio.org/xml/examples/shakespeare/macbeth.xml' | ||
|
|
||
| analyzer = ShakespeareAnalyzer.new(DEFAULT_XML_URL) | ||
|
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 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. |
||
| analyzer.run | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| require 'open-uri' | ||
| require_relative 'xml_parser' | ||
|
|
||
| class ShakespeareAnalyzer | ||
| def initialize(uri) | ||
| @file_content = get_content_from_uri(uri) | ||
| end | ||
|
|
||
| def run | ||
| print_speakers_sorted_by_line_count | ||
|
Contributor
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. Nice high level of abstraction. |
||
| end | ||
|
|
||
| private | ||
|
|
||
| def get_content_from_uri(uri) | ||
| open(uri, proxy: ENV['http_proxy']).read | ||
|
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. This suggests the Shakespeare class is doing too much - it's parsing the text, but also having to deal with the complexities of a proxy server.
Contributor
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. Agreed with @andyw8. |
||
| end | ||
|
|
||
| def print_speakers_sorted_by_line_count | ||
|
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. Nice job keeping even your private methods expressive, readable, and short. This whole file is really easy to understand, but I particularly like the clarity of this method. |
||
| xml_parser.speakers_sorted_by_line_count.each do |speaker, lines| | ||
| puts "#{lines} #{speaker}" | ||
| end | ||
| end | ||
|
|
||
| def xml_parser | ||
| @_xml_parser ||= XmlParser.new(@file_content) | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| require 'nokogiri' | ||
|
|
||
| class XmlParser | ||
| def initialize(xml) | ||
| @doc = Nokogiri.XML(xml) | ||
| end | ||
|
|
||
| def speakers_sorted_by_line_count | ||
| speakers_with_line_count.sort_by { |_key, value| value }.reverse | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def speakers_with_line_count | ||
| Hash[*speakers.map { |speaker| [speaker, count_lines_by_speaker(speaker)] }.flatten] | ||
| end | ||
|
|
||
| def speakers | ||
| @doc.css('PLAY SPEAKER').map { |speaker| speaker.text }.uniq | ||
| end | ||
|
|
||
| def count_lines_by_speaker(speaker) | ||
| @doc.css("PLAY SPEECH:has(SPEAKER[text()='#{speaker}']) LINE").count | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| 719 MACBETH | ||
| 265 LADY MACBETH | ||
| 212 MALCOLM | ||
| 180 MACDUFF | ||
| 135 ROSS | ||
| 113 BANQUO | ||
| 74 LENNOX | ||
| 70 DUNCAN | ||
| 62 First Witch | ||
| 46 Porter | ||
| 45 Doctor | ||
| 41 LADY MACDUFF | ||
| 39 HECATE | ||
| 35 Sergeant | ||
| 30 First Murderer | ||
| 30 SIWARD | ||
| 27 Third Witch | ||
| 27 Second Witch | ||
| 24 ALL | ||
| 23 Gentlewoman | ||
| 23 Messenger | ||
| 21 Lord | ||
| 21 ANGUS | ||
| 20 Son | ||
| 15 Second Murderer | ||
| 12 MENTEITH | ||
| 11 Old Man | ||
| 11 CAITHNESS | ||
| 10 DONALBAIN | ||
| 8 Third Murderer | ||
| 7 YOUNG SIWARD | ||
| 5 Third Apparition | ||
| 5 SEYTON | ||
| 5 Servant | ||
| 4 Second Apparition | ||
| 3 Lords | ||
| 2 First Apparition | ||
| 2 FLEANCE | ||
| 2 Both Murderers | ||
| 1 ATTENDANT | ||
| 1 Soldiers |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| require 'spec_helper' | ||
| require 'shakespeare_analyzer' | ||
| require 'stringio' | ||
|
|
||
| describe ShakespeareAnalyzer do | ||
| describe '#initialize' do | ||
| it 'reads provided URI and stores its content to @file_content' do | ||
| stub_request(:get, 'http://www.example.com/test_file.txt').to_return(body: 'This is just a test file!') | ||
|
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 need to learn what these stubs are. Do they interrupt flow and substitute content?
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. Yeah, check this gem: https://github.com/bblimke/webmock I've found it when I was looking for solution how to test http download. It is awesome. |
||
| analyzer = ShakespeareAnalyzer.new('http://www.example.com/test_file.txt') | ||
| expect(analyzer.instance_variable_get(:@file_content)).to eq 'This is just a test file!' | ||
|
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 really could have used this method when I was writing my tests. "instance_variable_get" 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 would actually recommend against using |
||
| end | ||
| end | ||
|
|
||
| describe '#run' do | ||
| it 'prints list of speakers sorted by line count' do | ||
| test_file = File.dirname(__FILE__) + '/test_files/test.xml' | ||
| stub_request(:get, 'http://www.example.com/test.xml').to_return(body: File.read(test_file)) | ||
|
Contributor
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. Notice how you have to stub out the http library when what you really want to do is check that the right parsing/counting is happening. This is a strong hint that your class has a few too many responsibilities. 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 never thought of stubbing the internet here. Thanks. Will add it to my solution too. |
||
| analyzer = ShakespeareAnalyzer.new('http://www.example.com/test.xml') | ||
| output = capture_stdout { analyzer.run } | ||
| expect(output).to eq "4 FourLiner\n3 ThreeLiner\n2 TwoLiner\n1 Liner\n" | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def capture_stdout &block | ||
| old_stdout = $stdout | ||
| fake_stdout = StringIO.new | ||
| $stdout = fake_stdout | ||
| block.call | ||
| fake_stdout.string | ||
| ensure | ||
| $stdout = old_stdout | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| require 'webmock/rspec' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| <?xml version="1.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. I like this concept of having the XML samples in a separate file. I embedded them within my spec, but it makes it very cluttered. I guess there's a trade-off though, in that you need to look at two places to understand what the spec is doing. |
||
| <!DOCTYPE PLAY SYSTEM "play.dtd"> | ||
|
|
||
| <PLAY> | ||
| <ACT> | ||
| <SCENE> | ||
| <SPEECH> | ||
| <SPEAKER>Liner</SPEAKER> | ||
| <LINE>Nothing more to say.</LINE> | ||
| </SPEECH> | ||
| <SPEECH> | ||
| <SPEAKER>TwoLiner</SPEAKER> | ||
| <SPEAKER>FourLiner</SPEAKER> | ||
| <LINE>Hello!</LINE> | ||
| <LINE>World!</LINE> | ||
| </SPEECH> | ||
| <SPEECH> | ||
| <SPEAKER>ThreeLiner</SPEAKER> | ||
| <LINE>One</LINE> | ||
| </SPEECH> | ||
| </SCENE> | ||
| </ACT> | ||
| <ACT> | ||
| <SCENE> | ||
| <SPEECH> | ||
| <SPEAKER>ThreeLiner</SPEAKER> | ||
| <LINE>Two</LINE> | ||
| </SPEECH> | ||
| </SCENE> | ||
| <SCENE> | ||
| <SPEECH> | ||
| <SPEECH> | ||
| <SPEAKER>ThreeLiner</SPEAKER> | ||
| <SPEAKER>FourLiner</SPEAKER> | ||
| <LINE>Three!</LINE> | ||
| </SPEECH> | ||
| <SPEAKER>FourLiner</SPEAKER> | ||
| <LINE>Three!</LINE> | ||
| </SPEECH> | ||
| </SCENE> | ||
| </ACT> | ||
| </PLAY> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| require 'spec_helper' | ||
| require 'xml_parser' | ||
|
|
||
| describe XmlParser do | ||
| describe '#initialize' do | ||
| it 'instantiates @doc with Nokogiri XML parser' do | ||
| doc = XmlParser.new(nil).instance_variable_get(:@doc) | ||
| expect(doc.instance_of?(Nokogiri::XML::Document)).to be true | ||
| end | ||
| end | ||
|
|
||
| describe '#speakers_sorted_by_line_count' do | ||
| it 'returns hash of speakers with line count sorted by line count' do | ||
| filename = File.dirname(__FILE__) + '/test_files/test.xml' | ||
| xml_parser = XmlParser.new(File.read(filename)) | ||
| expect(xml_parser.speakers_sorted_by_line_count.to_a).to eq({'FourLiner' => 4, 'ThreeLiner' => 3, 'TwoLiner' => 2, 'Liner' => 1}.to_a) | ||
| end | ||
| 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.
You're one of the only folks to extract this into a constant. I like it. :)