synewaves / rubillow

Ruby library to access the Zillow API
MIT License
50 stars 39 forks source link

Fix common scenarios where xml nodes are not present #21

Closed danielberkompas closed 11 years ago

danielberkompas commented 11 years ago

Zillow typically returns a rentzestimate in this format:

<rentzestimate>
  <amount currency="USD">2505</amount>
  <last-updated>04/01/2013</last-updated>
  <oneWeekChange deprecated="true"></oneWeekChange>
  <valueChange duration="30">50</valueChange>
  <valuationRange>
    <low currency="USD">1979</low>
    <high currency="USD">3056</high>
  </valuationRange>
</rentzestimate>

However, sometimes <valueChange> doesn't have a duration attribute.

<valueChange></valueChange>

This was causing Rubillow to throw an error like the following for me:

NoMethodError: undefined method `value' for nil:NilClass - Zestimateable#extract_zestimate:71

The offending line in Rubillow looked like this:

:value_duration => xml.xpath('//rentzestimate/valueChange').attr('duration').value.chomp

The duration attribute is nil, so this raised an error. Fixed it with this:

:value_duration => xml.xpath('//rentzestimate').first.xpath("//valueChange").first.attr("duration").chomp

I added a test to verify that this fixes the issue, and also bumped the version number to 0.0.6. Since this is a patch, I think SemVer would require a new version number.

P.S. Why are we calling String#chomp on the end here? It was already there so I left it in, but it seems unnecessary to me.

danielberkompas commented 11 years ago

Hold off on merging this for a little while. It seems to be breaking the tests in my app using Rubillow, and I need to figure out why.

danielberkompas commented 11 years ago

Okay, it looks like the problem I encountered was a separate bug, but I decided to push a commit to fix it to this pull request as well. Basically, there are a lot of cases where Rubillow looks for data like this:

xml.xpath('//somePath').first.text

This assumes that somePath will always be present in Zillow's response. This is an unsafe assumption, especially since Zillow doesn't necessarily promise that all of these fields will be present.

If this were Rails, we could do:

xml.xpath('//somePath/').first.try(:text)

However, I don't think you want to add a dependency on ActiveSupport (or wherever try comes from), so I wrote a helper method that I included by default into Rubillow::Model. See the diff for more details. Basically, my helper method allows you to check if a node is present before trying to call #text.

Let me know what you think.

synewaves commented 11 years ago

I like your idea to add xpath_if_present. It is much cleaner than doing unless elm.empty? checks. I think adding in the full dependency of ActiveSupport would be a bit overkill for the situation.

This all looks great. I'll merge this in and push a new version. Thanks for your help!