Analyzer attempt by Dave West (@twosevenero)

[twosevenzero]

This was just what I needed to get a grasp on TDD. Thank you!

[r00k]

What about making this url a named constant instead?

[twosevenzero]

so something like MACBETH = then
def initialize url = MACBETH

For readability sake or is there another reason?

[r00k]

Mostly readability, but also because it helps you avoid the temptation to duplicate this string elsewhere (which you actually ended up doing).

[r00k]

Good work!

Left a bunch of comments.

[mrhead]

@twosevenzero There is a bug in your code because it is not able to correctly count speakers in this case:

# hamlet.xml
<SPEECH>
<SPEAKER>CORNELIUS</SPEAKER>
<SPEAKER>VOLTIMAND</SPEAKER>
<LINE>In that and all things will we show our duty.</LINE>
</SPEECH>

I've found it by accident by comparing yours and my results. Your code cannot detect CORNELIUS. However I did not check if there are such SPEECHes in Macbeth.

[mrhead]

just found same SPEECH in Macbeth:

<SPEECH>
<SPEAKER>MACBETH</SPEAKER>
<SPEAKER>LENNOX</SPEAKER>
<LINE>What's the matter.</LINE>
</SPEECH>

@r00k How we should count it? One line for each?

[twosevenzero]

Awesome find! Thanks @mrhead

[briantemple]

Perhaps these two blocks could be combined, since @characters is not used outside this block?

speech.elements.each("SPEAKER") do |speaker|
  @speakers[speaker] += @line_count
end

[andyw8]

By the way, [] is equivalent to Array.new and seems be more idiomatic.

[briantemple]

Nice solution -- I had not seen the rexml library before. Pretty slick!

[andyw8]

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 the start would then be at the same level of abstraction, which I think would make things more readable.

[twosevenzero]

Awesome. Thanks for the feedback!