Skip to content

Conversation

@gniquil
Copy link

@gniquil gniquil commented Sep 17, 2013

No description provided.

@gniquil
Copy link
Author

gniquil commented Sep 17, 2013

@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

@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
Copy link
Author

gniquil commented Sep 17, 2013

@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!

Copy link

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
Copy link
Author

gniquil commented Sep 18, 2013

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

Copy link
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
Copy link
Contributor

r00k commented Sep 26, 2013

Good effort! See my comments inline.

@gniquil
Copy link
Author

gniquil commented Sep 26, 2013

@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