Skip to content

Here's my try#6

Open
gniquil wants to merge 10 commits into
thoughtbot:masterfrom
gniquil:master
Open

Here's my try#6
gniquil wants to merge 10 commits into
thoughtbot:masterfrom
gniquil:master

Conversation

@gniquil

@gniquil gniquil commented Sep 17, 2013

Copy link
Copy Markdown

No description provided.

@gniquil

gniquil commented Sep 17, 2013

Copy link
Copy Markdown
Author

@twosevenzero, good call on the duplication. I refactored a bit and added a bit of comments. Hopefully the tests will be more clear

@briantemple

Copy link
Copy Markdown

@gniquil I really liked your solution - very slick!

I was just curious about doing the analysis in two phases. It looks like the first phase collects the speeches by line to the speakers, and then the second phase counts the lines and aggregates them all up. Instead of copying over all the lines, it seems like speeches and analyze could be combined in to something like:

  def analyze
    @output = Hash.new(0)
    Nokogiri::XML(contents).css("SPEECH").map do |speech|
      @output[speech.at_css("SPEAKER").content] += speech.css("LINE").count
    end

    @output
  end

@gniquil

gniquil commented Sep 17, 2013

Copy link
Copy Markdown
Author

@briantemple you are absolutely right! The only reason I did it this way was TDD. I wrote the line parser first to just get an understanding. Then I realized all i need was an inject method. BTW, I think inject might be even better here:

def analyze
  Nokogiri::XML(contents).css("SPEECH").inject(Hash.new(0)) do |recorder, speech|
    recorder[speech.at_css("SPEAKER").content] += speech.css("LINE").count
    recorder
  end
end

Wow, didn't know Hash.new(0) is so cool! Learned something new today! Thanks!

Comment thread lib/macbeth_analyzer.rb Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this block is nicely done. You've made what's happening here extremely clear.

@gniquil

gniquil commented Sep 18, 2013

Copy link
Copy Markdown
Author

Hi All,

I guess I am over-thinking too much. Anyway, check out the analyze method again. XPATH is pretty scarily good sometimes. I think this is about the best I can do.

Hope you guys like it.

Frank

Comment thread lib/macbeth_analyzer.rb

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about pulling this into a named constant?

@r00k

r00k commented Sep 26, 2013

Copy link
Copy Markdown
Contributor

Good effort! See my comments inline.

@gniquil

gniquil commented Sep 26, 2013

Copy link
Copy Markdown
Author

@r00k Yoda at work!

Thanks Ben for the reply. Your comment on the stub is really valuable. The way I was testing it always leaves me a funny uneasy feeling. I will try the responsibility-double approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants