PHP Weather class

I am kind of new to object orientation in PHP and I need some experts feedback.

This class fetches the weather from this URL.

It grabs the weather, temperature and an icon from the current hour.

• Is this proper OOP?

• Or what can I do to improve this class?

• Do I need to make it more abstract?

• What can I do to make the _parse_weather_xml method less ugly?

Concrete examples are very much appreciated!

```
_check_weather($latitude, $longitude);
}

private function _check_weather($latitude, $longitude)
{
$weather_xml = $this->_get_weather_xml($latitude, $longitude);
$weather_xml = $this->_parse_weather_xml($weather_xml);
}

public function get_weather($property)
{
return $this->$property;
}

public function __get($property)
{
if(!property_exists($this, $property)){
echo "The property {$property} doesn't exist.";
}
}

private function _get_weather_xml($_latitude, $_longitude)
{
$weather_xml = simplexml_load_file("http://api.met.no/weatherapi/locationforecast/1.8/?lat={$_latitude};lon={$_longitude}");
return $weather_xml;
}

private function _parse_weather_xml($weather_xml)
{
define('TZ_OFFSET', 60601);
$now = time();
$current_hour = $now - ($now % (60*60));
$current_weather = array();
foreach ($weather_xml->product->time as $time){
$from_hour = strtotime($time['from']) - TZ_OFFSET;
$from_hour = $from_hour - ($from_hour % (60*60));
if($time['datatype'] == "forecast"){
if($from_hour == $current_hour){
if($time->location->symbol){
if(!isset($current_weather['symbol'])){
$current_weather['symbol'] = $time->location->symbol;
}
}
else {
if(!isset($current_weather['temperature'])){

Right, first: short answers to your 4 questions:

• Is this proper OOP?

No, there are several issues

• Or what can I do to improve this class?

A few things, we'll get to that in a moment.

• Do I need to make it more abstract?

You may want to look into that

• What can I do to make the _parse_weather_xml method less ugly?

Ditch it, or better yet, inject a dependency that just parses XML

I'm at work now, so I'll be updating this answer every once in a while.

But first off: is this proper OOP?

No, for two main reasons: OOP works best if each class (object) has only 1 single task, and is given all it needs to do this one task. Your class does several things: it parses XML, it echoes, and it serves as an API/wrapper thing for a parsed XML file. Take this for example:

public function get_weather($property)
{
    return $this->$property;
}
public function __get($property)
{
    if(!property_exists($this, $property)){
        echo "The property {$property} doesn't exist.";
    }
}

What is quite likely is for me to do either:

$foo = $instance->get_weather('foobar');
//or even
$foo = $instance->foobar;

Both will do the same thing. Not only does your slow magic method __get expose all properties (both public and private) to the user, it also, as in the case I gave here results in unwanted behaviour. Assume, if you will, an MVC pattern. if I were to paste the code $foo = ... in the Model layer, or the controller, the echo would cause the headers to be sent, and I am no longer able to set the headers.

But that's nothing compared to the bugs I might be getting: I don't know which usage of $instance is causing the problem I'll have to debug all of the code. An object shouldn't echo. Especially not if its goal is to notify the user of abuse: throw an exception!

public function get_weather($property)
{
    if (property_exists($this, $property))
        return $this->$property;
    throw new InvalidArgumentException($property. ' does not exist!');
}

Think about these for a kickoff... I'll be back for more, though :)

Back for more:

And let's get right into the nitty-gritty, and question your design. Now this may seem harsh, but IMO, that's what code-review has to do.

You've defined a constructor to accept coordinates. Right, so that, to me, looks like a data class (an object that carries data, and just has getters and setters to allow for easy data access). But in reality, the data I pass to the constructor is never stored, it is, in stead, passed on directly to an API call, and results in your class parsing an XML DOM. From this, you set the actual properties of the object.

Now, in itself, this is perfectly valid: I don't need to know how and whence the data is coming from, but I may want to be offered, in the very least, a choice of when the API call is actually made. Personally, I'd lazy-load it, for example.

I may also find myself in the position where I have only 1 of the coordinates you need, and I may choose to create the instance of the class ahead of time, and set the second coordinate later on.

The main reason why I'd want to do this is because that allows me to use type-hints in my code:

class Weather
{
    private $long = null;
    private $lat = null;
    public function __construct(array $coords)
    {
         if (isset($coords['long'])) $this->long = $coords['long'];
         if (isset($coords['lat'])) $this->lat = $coords['lat'];
    }
    public function setLong($long)
    {
         $this->long = $long;
         return $this;//enables chainable interface
    }
    public function setLat($lat)
    {
         $this->lat = $lat;
         return $this;
    }
    public function getLong()
    {
         return $this->long;
    }
    public function getLat()
    {
         return $this->lat;
    }
}

Now this way, I can pass the instance along god knows how many times, but to methods that all look like this:

public function doStuff(Weather $weatherInstance)
{
    $this->checkCoordinates($weatherInstance);//this once may set missing coordinates
}

Back for more in a moment...

On the reason why I'm not using underscores in the property names: I try to follow the PHP FIG coding style guidelines as much as possible, and like PSR-2 clearly states:


Property names SHOULD NOT be prefixed with a single underscore to indicate protected or private visibility.

The standards also specify that methods should be camelCased. I did not change your methods get_weather to the more preferable getWeather for clarity purposes only, but you may have noticed that I did use camelCased method names for example methods and my getters and setters.

As for those getter and setter methods: Yes, I most certainly do use those whenever I can. Instead of relying on __get and __set magic methods. There are several issues I have with these magic methods, to name a few:

• Exposing private and protected properties.

• Slow (at least 2 additional lookups required

StackExchange Code Review Q#40219, answer score: 5