search

RLovelett / sports_data_api

A Ruby interface to the Sports Data API. API supports NFL, MLB, NHL, and NBA.
http://developer.sportsdatallc.com/api_gallery
Other
26 stars 28 forks source link

Refactor #29

Closed trdinich closed 10 years ago

trdinich commented 10 years ago

sorry about the 3 commits... b/c of the merge of upstream I got into git squash hell and couldn't find my way out of it. :-/

I'll be adding support for baseball this week so I wanted to try and DRY some of this stuff up before it gets too painful. The MLB api is v4 (brand new), so it's yet another version that we'll support in this gem, which limits refactoring abilities in other places (without a major overhaul anyway).

Lemme know what you think.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.25%) when pulling 504a623df02a50c44128d361d22ea84074723e74 on trdinich:refactor into df39091e1b42baade229fccdd3d83e14c8a024c1 on RLovelett:master.

RLovelett commented 10 years ago

Sorry about taking a long time to respond. I had to travel to Huntsville, AL this week. The snow/ice here is causing havok in the city. Finally, having a few minutes to review this.

I really like the refactor on pulling the SportsDataApi::Nba.generic_request and the SportsData::Nfl.generic_request up into SportsDataApi.generic_request. The only question I have is why not go a step further? What do you think about somehow refactoring SportsDataApi::Nba.response_xml and SportsDataApi::Nfl.response_xml into the SportsData.response_xml?

trdinich commented 10 years ago

Re: status change. Correct... a VCR cassette changed because I implemented the summary call on a game object, which in turn hits the sports data api. It thus re-pulled that game so I needed to update the expectations. No functional changes.

You're absolutely right about going even further. After I submitted the pull request I thought about doing exactly that and figured that I could easily add that in to the pull request for the MLB stuff that I'll be doing here in short order.