Google Weather API wrapper

Question

I'm an amateur programmer (self-taught). Anyway, this class provides a few methods to access Google's unofficial weather API. I'm having trouble on how to go about handling parsing errors in parse_xml() so any suggestions there would also be helpful.

<?php
  class weather {
  private $_location;
  private $_url = 'http://www.google.com/ig/api?weather=';
  private $_isParsed = false;
  private $_wData;

  public $lastError;

  public function __construct( $location) {
      // Set location
      $this->_location = $location;

      // urlencode doesn't seem to work so manually add the + for whitespace
      $this->_url = preg_replace('/\s{1}/', '+',$this->_url .= $location);
      $this->parse_xml($this->get_xml());
  }

  public function get_temp($type = "f") {
      if (!$this->_isParsed)
          return false;

      // User specificed celsius, return celsius
      if ($type == "c")
          return $this->_wData['current']['temp_c'];

      // return fahrenheit
      return $this->_wData['current']['temp_f'];
  }

  public function get_condition() {
      if (!$this->_isParsed)
              return false;
      // provide current conditions only
      return $this->_wData['current']['condition'];
  }

  public function get_forecast_for_day($day) {
      if (!$this->_isParsed)
          return false;
      return $this->_wData['forecast'][$day];
  }
  public function get_forecast_assoc() {
      if (!$this->_isParsed)
          return false;
      return $this->_wData['forecast'];
  }

  public function get_cond_assoc() {
      if (!$this->_isParsed)
          return false;
      return $this->_wData['current'];
  }

  public function dump_wData() {
      if (!$this->_isParsed)
          return false;
      return $this->_wData;
  }

  public static function to_celsius($f) {
      // Convert Fahrenheit to Celsius.
      // I figured this would be quicker than trying to parse the XML.
      return floor(((int)$f - 32) * (5 / 9));
  }

  private function get_xml() {
      // Download raw XML to be parsed.
      $ch = curl_init($this->_url);

      // I don't know why I altered the useragent. It must have been for a good reason. Oh well.
      curl_setopt($ch, CURLOPT_USERAGENT, 'Mozilla/5.0 (Windows NT 6.1; rv:5.0.1) Gecko/20100101 Firefox/5.0.1');
      curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
      $rawXML = curl_exec($ch);

      if (!$rawXML)
          return false;
      curl_close($ch);
      return $rawXML;
  }

  private function parse_xml($xData) {
      libxml_use_internal_errors(true);
      try {
        $weather = new SimpleXMLElement($xData);
      } catch (Exception $err) {
          // Set $lastError to getMessage()
          $this->lastError = $err->getMessage();
         return false;
      }
      // Select the current_conditions node ($cNode)
      $cNode = $weather->weather[0]->current_conditions;

      // ========= Set up our current conditions array ====================

      // Tempreature - temp_f Fahrenheit, temp_c celsius - set as floats.
      $this->_wData['current']['temp_f'] = (float)$cNode->temp_f->attributes()->data;
      $this->_wData['current']['temp_c'] = weather::to_celsius($this->_wData['current']['temp_f']);

      // Condition
      $this->_wData['current']['condition'] = (string)$cNode->condition->attributes()->data;

      // Condition Icon - icon url is not absolute, append google.com
      $this->_wData['current']['icon'] = (string)"http://www.google.com" . $cNode->icon->attributes()->data;

      // Wind Condition
      $this->_wData['current']['wind'] = (string)$cNode->wind_condition->attributes()->data;

      // ============= Set up our forecast array =============
      $fNode = $weather->weather[0]->forecast_conditions;

      // Iterate through each day of the week and create an assoc array.
      foreach ($fNode as $forecast) {
          // Get the day.
          $day = (string)$forecast->day_of_week->attributes()->data;

          // Insert an array of info for that day
          $this->_wData['forecast'][$day] = array (
              "high" => (float)$forecast->high->attributes()->data,
              "low" => (float)$forecast->low->attributes()->data,
              "icon" => (string)"http://www.google.com" . $forecast->icon->attributes()->data,
              "condition" => (string)$forecast->condition->attributes()->data
            );
      } //foreach ($fNode as $forecast)
      // Let the class know wData is ready for use.
      $this->_isParsed = true;
  } //private function parse_xml($xData)

}
?>

Answer 1

Use methods to get the data, this is a kind of dependency injection and will allow you to test the code with out having to hit google's server everytime. It will also avoid the if($this->_isParsed) on all your functions.

Additionally, move the 'parsing' - which is really just navigating the XML structure - to the functions that return the data. This removes your parse_xml function (which would now be just a function that creates the SimpleXML object), and allows you to throw exceptions only when requested data is missing/unexpected (or return just return false, if that's how you want your client to act).

Here's some example code.

I tend to use camelCase, I'm not trying to change your style, it's just second nature.

<?php
class GoogleWeather{
  protected $api = 'http://www.google.com/ig/api?weather=';
  protected $xml;
  protected $response;
  protected $location;

  public function __construct( $location) {
    // Set location
    $this->location = $location;
  }

  public function setResponse($response)
  {
    $this->response = $response;
  }

  public function getResponse()
  {
    //if the xml hasn't been fetched, then fetch it
    if(empty($this->response)){
      $this->setResponse($this->fetchData());
    }
    return $this->response;
  }

  //was get_xml renamed to avoid confusion
  public function fetchData()
  {
    // Download raw XML to be parsed.
    $ch = curl_init($this->api . urlencode($this->location));

    // I don't know why I altered the useragent. It must have been for a good reason. Oh well.
    curl_setopt($ch, CURLOPT_USERAGENT, 'Mozilla/5.0 (Windows NT 6.1; rv:5.0.1) Gecko/20100101 Firefox/5.0.1');
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
    $rawXML = curl_exec($ch); 

    if (!$rawXML){
      //check the curl error for a better message
      throw new Exception('could not fetch data'); 
    }
    curl_close($ch);
    return $rawXML;
  }

  public function getXml()
  {
    if(empty($this->xml)){
      try{
        $this->xml = new SimpleXMLElement($this->getResponse());
      } catch (Exception $e) {
        //there's no real recovery here, except maybe to retry fetching the data
        throw new Exception('bad response from from API');
      }

      //check for 'problem_cause' element
      if(isset($this->xml->weather->problem_cause)){
        throw new Exception('API responded with error');
      }
    }
    return $this->xml;
  }

  public function getCondition()
  {
    //make sure there's data
    if(!isset($this->getXml()->weather->current_conditions)){
      //you could throw an exception and assume any code using this is 
      //calling it in a try block
      throw new Exception('could not find conditions');
    }

    return $this->getXml()->weather->current_conditions;
  }

  public function getTemp($type = 'f')
  {
    //validate type
    if(!in_array($type, array('f','c'))){
      throw Exception('invalid temp type: ' . $type);
    }

    $element = 'temp_' . $type;
    //make sure there's data
    if(!isset($this->getCondition()->{$element}['data'])){
      throw new Exception('could not find temp');
    }

    //cast as float and return
    return (float) $this->getCondition()->{$element}['data'];
  }
}

You can avoid navigating the SimpleXML object multiple times for the same data using the same basic concept the other lazy loading functions use (getXML(), getResponse()).

Instead of using object properties, I just put a static variable in the method - since the location can not change, there's no never a need to unset all the data.

However, if the location could change, the same could be accomplished with $this->data[$property] - obviously resetting $this->data to array() every time the location is changed.

Here's an improved getTemp():

  public function getTemp($type = 'f')
  {
    static $temp = array();

    //validate type
    if(!in_array($type, array('f','c'))){
      throw Exception('invalid temp type: ' . $type);
    }

    if(isset($temp[$type])){
      return $temp[$type];
    }

    $element = 'temp_' . $type;
    //make sure there's data
    if(!isset($this->getCondition()->{$element}['data'])){
      throw new Exception('could not find temp');
    }

    //cast as float and return
    $temp[$type] = $this->getCondition()->{$element}['data'];
    return $temp[$type];
  }

I'd be interested in seeing if there's any significant performance gained over navigating the SimpleXML object each time.

Answer 2

I'm a C# developer myself but I like this altogether, very clean and easy to read and understand even though it's not a language I've worked in.

The layout of the class is clean with the members and methods nicely ordered and clear to their purpose.

The only suggestions I have are: seperate it into 2 classes, the one that has the gets, and the one that does the parsing. Make the GoogleWeatherResponseParser a member of your GoogleWeatherDataAccess and you can either construct the parser at construction, or a more testable approach is that it's handed to the constructor.

If it's handed to the constructor, the consumer of the data access would construct the parser and be able to check if it was parsed successfully or not before handing it to the data access. Then the data access would only have to check at construction and throw an exception, never checking in the individual methods making them cleaner.

Also, you could then write unit tests that hand in various versions of the GoogleWeatherResponseParser to the GoogleWeatherDataAccess at construction and verify that the data the data access hands out is as you expected. Not sure how common unit tests are in PHP, but they are immensely helpful in allowing you to change code knowing that you'll be notified if you broke something.

Edit: Oh and you're dead right about doing the celsius calculation as opposed to the xml parse step.

Answer 3

If I could make a few suggestions, both for readability and to avoid problems in the future.

1. Indent from the class name.

Instead of

<?php
  class weather {
  private $_location;
  private $_url = 'http://www.google.com/ig/api?weather=';
  private $_isParsed = false;
  private $_wData;

write

<?php
  class weather {
    private $_location;
    private $_url = 'http://www.google.com/ig/api?weather=';
    private $_isParsed = false;
    private $_wData;

2 Always use parenthesis with an if statement

instead of

public function get_cond_assoc() {
    if (!$this->_isParsed)
        return false;
    return $this->_wData['current'];
}

use

public function get_cond_assoc() {
    if (!$this->_isParsed)
    {
        return false;
    }
    return $this->_wData['current'];
}

This may seem unnecessary since your construction is perfectly legal, but the first time you spend 2 hours hunting down a bug caused by not putting in the braces you'll thank me :)

3 Dont typecast unnecessarily

The (string) here is unnecessary and confusing.

 // Condition Icon - icon url is not absolute, append google.com
  $this->_wData['current']['icon'] = (string)"http://www.google.com" .
                                      $cNode->icon->attributes()->data;

These are just a few pointers to make your code a bit more readable and less likely to cause you headaches in the future.