7
\$\begingroup\$

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)

}
?>
\$\endgroup\$
  • 1
    \$\begingroup\$ amateur doesn't mean self-taught, it just means unpaid. There's plenty of us who went from amateur to professional self-taught :) \$\endgroup\$ – Jimmy Hoffa Aug 13 '11 at 17:00
  • \$\begingroup\$ Google's Weather API is gone, but Open Weather Map looks like a good replacement. \$\endgroup\$ – David Harkness Jul 26 '14 at 17:53
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I don't know the overhead involved in querying into the SimpleXML object, I'm assuming it's not much, but just for safety I would lazy load in your getCondition and getTemp, have member level temp and condition, and first thing method does is check if(isset($this->condition)) { return $this->condition; } then set $this->condition at the end of getCondition() after parsing it out, otherwise this makes perfect sense. \$\endgroup\$ – Jimmy Hoffa Aug 17 '11 at 11:16
  • \$\begingroup\$ @Jimmy Hoffa Yeah, that could certainly be added. I'll try to update with that in a bit. \$\endgroup\$ – Tim Lytle Aug 17 '11 at 17:06
  • \$\begingroup\$ @Jimmy Hoffa updated with your suggestion (well, a variation of it). \$\endgroup\$ – Tim Lytle Aug 18 '11 at 0:19
  • \$\begingroup\$ "Instead of using object properties, I just put a static variable in the method." Warning: Static local variables are shared across all instances of the class. \$\endgroup\$ – David Harkness Jul 26 '14 at 17:41
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I disagree about splitting it into two objects, as it's already two objects. The actual parsing is done by the SimpleXML object, all the class has to be is a wrapper around that, allowing easy access to the data (easier than knowing where in the XML tree to find the data). I believe the real change should be navigating the tree when needed, and not all at once (in the parse_xml method). If the code did that, I think it would look like your two object suggestion, since the SimpleXML object would replace your suggested GoogleWeatherResponseParser. \$\endgroup\$ – Tim Lytle Aug 17 '11 at 2:39
  • 1
    \$\begingroup\$ @Tim I am unfamiliar with the php frameworks SimpleXML object, but my logic was based on the single responsibility principle and seperation of concerns, right now the one class is concerned with the http request, the response parsing, and the data serving to clients. My suggestion was there be one object for managing the request/parsing (which could use lazy loads for delayed execution) and one for data serving to clients, or in more standard terms a data provider and a data access, in which they do not need to know eachothers implementations. \$\endgroup\$ – Jimmy Hoffa Aug 17 '11 at 11:10
  • \$\begingroup\$ @ Jimmy Hoffa - wasn't meaning to disagree with the principle, just that using SimpleXML already separates the two (and in this case, I find it enough). \$\endgroup\$ – Tim Lytle Aug 18 '11 at 0:11
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.