3
\$\begingroup\$

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.

  1. Is this proper OOP?
  2. Or what can I do to improve this class?
  3. Do I need to make it more abstract?
  4. What can I do to make the _parse_weather_xml method less ugly?

Concrete examples are very much appreciated!

<?php

/**
 * Weather class
 */

class Weather
{

    private $_weather;
    private $_temperature;
    private $_symbol_url;

    public function __construct($latitude, $longitude)
    {
        $this->_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', 60*60*1);
        $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'])){
                            $current_weather['temperature'] = $time->location->temperature['value'];
                        }
                    }
                }               
            }
        }
        $this->_weather = $current_weather['symbol']['id'];
        $this->_temperature = $current_weather['temperature'];
        $this->_symbol_url = "http://api.met.no/weatherapi/weathericon/1.0/?symbol=".$current_weather['symbol']['number'].";content_type=image/png";
    }
}

// Instantiate Weather object, parameters: latitude, longitude
$weather = new Weather(59.32893000000001, 18.06491);
echo $weather->get_weather("_weather")."<br />";
echo $weather->get_weather("_temperature")."<br />";
echo "<img src=".$weather->get_weather("_symbol_url")." />";

?>
\$\endgroup\$
5
\$\begingroup\$

Right, first: short answers to your 4 questions:

  1. Is this proper OOP?
    No, there are several issues
  2. Or what can I do to improve this class?
    A few things, we'll get to that in a moment.
  3. Do I need to make it more abstract?
    You may want to look into that
  4. 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)
  • bad for code-hinting IDE's
  • No control over the types being assigned to the actual properties, thus no control over the data your object contains => risk of memory leaks is quite real.

Now, let's examine these 5 issues:

class PrivateOnly
{
    private $email = null;
    private $db = null;
    public function __set($name, $val)
    {
        $this->{$name} = $val;
    }
    public function setEmail($email)
    {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL))
        {
            throw new InvalidArgumentException(__METHOD__.' was not passed a valid email address, instead saw: '. $email);
        }
        $this->email = $email;
        return $this;
    }
    public function setDb(PDO $connection)
    {
        $this->db = $connection;
        return $this;
    }
}

Now here you can clearly see that a setter allows me to validate the input more clearly, and allows me to throw exceptions that actually explain what is wrong with my code at any give time.
I can also use type-hinting, to restrict any value except for those types I actually need, in the case of the db property, it is clear that I'm expecting to receive an instance of PDO to be assigned to that property.
This makes my API more self-documenting, less error prone, faster to learn and (as I will show) faster in terms of performance, too.

performance: I've already covered the internals of PHP objects here. The bottom line, and I was backed up by hakre (a PHP core contributor), is that pre-declared properties are faster, simply because a lookup of a pre-declared (aka property known at compile time) is an O(1) operation, as opposed to O(n).
The same applies to methods, too, and so a pre-defined setter will outperform a magic method. End of. Compare what PHP has to do in the following cases:

$instance->newProperty = 'new Value';
//1. loopup for public newProperty ==> not found
//2. lookup for __set ==> found
//3. invoke __set, pass newProperty zval, and newValue
//4. set flag on instance that PHP uses to check if it's already in the __set method
//5. execute __set 
// 5a=> scan properties_table (HashTable) for property (fast one)
// 5b=> if property is private or protected, change value, else:
//      add new pointer to properies table (the slow table)
//6. return

The same operations will be required for the __get method, give or take. Compare that to a pre-defined property (which only required step 1), and compare that to a custom (safer) setter:

$myObject->setEmail('foo@bar.com');
//1. Lookup for setEmail method
//2. invoke
//3. execute, throw exception on failure
//4. Lookup email property (fast table)
//5. assign
//6. Return

Yes, you may say, this is the same number of steps as the slow approach (magic set), but I think you'll agree that the outcome of this approach is much, much more predictable, it only uses the fast HashTable lookups and performs additional validations that the first approach simply can't do.

code-hinting: simple - most decent IDE's will use the doc-blocks in your class for code-completion and code hinting. If you use a magic setter, then there are no doc-blocks in place to help you out. If you use a getter and setter per property, then your IDE will tell you what each method expects, in terms of arguments and, perhaps more importantly, what a getter returns.
If a getter returns an instance of a class, the IDE will know what class you're using here:

$pdo = $myInstance->getDb();//assume doc-block @return \PDO
$pdo->p[auto-complete:prepare]();

This makes your code easier to learn and use for others. Use doc-blocks and type-hints whenever you can.

Control: Well, most of this is covered already, but take the PrivateOnly class example again: I hint PDO in the setDb method. That means that if I pass an invalid argument at some point, the method won't ever get called, and the app will crash, on the plus side, you'll have an easy time identifying the exact line in your code that caused this problem, and might see that that line of code is attempting to pass an instance of Mysqli to the setDb method, and you can set about fixing the code.
Using a setter, the line that looks like this:

$instance->db = new Mysqli();

Goes by unnoticed... it's perfectly valid, but then, when you attempt to call a method of the instance that actually uses the db property, thinking it's a PDO instance, you're in trouble:

$stmt = $this->db('SELECT * FROM userData WHERE email = :email');//all good
//stmt is mysqli_stmt instance, though you think it's a PDOStatement
$stmt->execute(
    array(
        ':email' => $this->getEmail()
    )
);//ERROR mysqli_stmt::execute expects void!

This is where you'll find yourself stepping through a lot of code, in search of that one statement $this->db = $mysqliInstance;, which you are likely to overlook, simply because it's a perfectly valid and simple statement. Don't torture yourself with stuff like this: use the tools you have at your disposal: methods + typehints save lives, or at least: working hours.

Memory leaks: PHP manages memory using reference counts, if an object, or value has no references (no var left that point to a given object), then the memory associated with that value is freed. This works fine in simple cases, but leaks like a b**** when you introduce it to some circular references:

$instance->itself = $instance; 

The ref-count of $instance is increased by 1, because $instance has been given a new property, and that property references $instance, so this is a circular reference, because we have an object that points to itself. The reference count, then will always be >= 1, never 0, because within the scope of $instance, a reference to itself is still accessible.
Even if PHP has addressed this specific issue (and I think it has), stuff like this is still prone to leak:

$instance->brother = $instance2;
$instance2->sister = $instance;

2 separate objects reference each-other, if I would then reassign $instance and $instance2, it wouldn't matter, because the ref-count will be >= 1 for both objects:

[scope instance] -> instance2 ==> refcount instance2 => 1
[scope instance2] -> instance ==> refcount instance => 1

Because both instances are referenced in a scope, that they supposedly can still access, they can't be GC'ed, yet neither of these scopes is accessible to you. Hence __set can easily lead to memory leaks:

$instance->typ0 = $someObj;
//comment: avoid memory leak, remove reference to $someObj
$instance->typo = null;

Because of the typo, you're actually not unsetting the reference to $someObj. Don't use PHP's object overloading, that's the bottom line.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for your feedback! I'm looking forward to hear more! \$\endgroup\$ – DoubleRainbow Jan 28 '14 at 9:45
  • \$\begingroup\$ @DoubleRainbow: Added a little side-note on constructor \$\endgroup\$ – Elias Van Ootegem Jan 28 '14 at 14:39
  • \$\begingroup\$ Wow thanks! I didn't expect to receive this much help! It's very appreciated! Sorry for being a noob, but do you usually write one separate getter and setter for each property in a class? And do you prefer not to have an underscore as the first character in a private or protected property or method? I'd love to hear more feedback! \$\endgroup\$ – DoubleRainbow Jan 28 '14 at 19:37
  • \$\begingroup\$ @DoubleRainbow: Added reasons for my omitting the underscore, and some more details on setters and getters and why to use them \$\endgroup\$ – Elias Van Ootegem Jan 29 '14 at 8:27
  • \$\begingroup\$ Again, thank you so much for your help! It is really very helpful! You've already helped me too much, but if you're in the mood, could you explain more about what you mean by "Ditch it, or better yet, inject a dependency that just parses XML" regarding the _parse_weather_xml method? \$\endgroup\$ – DoubleRainbow Jan 29 '14 at 15:32
1
\$\begingroup\$

The bulk of your code looks fine, I have made what are (In my opinion) a few usability enhancements, but take it or leave it as you see fit.

I don't think you can really make the xml processing any less ugly.

Comments are inline

<?php

/**
 * Weather class
 */

class Weather
{
    // lets define our constants here, so if it ever change we don't have to scan the code to find it
    const TZ_OFFSET_SECONDS = 3600;
    const URL_API = "http://api.met.no/weatherapi/locationforecast/1.8/";
    const URL_IMG = "http://api.met.no/weatherapi/weathericon/1.0/";


    private $_current_weather = null;
//    private $_temperature;
//    private $_symbol_url;
    private $_latitude;
    private $_longitude;


    public function __construct($latitude, $longitude)
    {
        // Nothing wrong with what you have done, but personally i am not a big fan of doing work in the constructor
        // $this->_check_weather($latitude, $longitude);
        $this->_latitude = $latitude;
        $this->_longitude = $longitude;
    }

//    private function _check_weather($latitude, $longitude)
    private function _check_weather()
    {
        // check and store the current weather, then ignore any further requests
        if (!$this->_current_weather) {
            $weather_xml = $this->_get_weather_xml();

//        $weather_xml = $this->_parse_weather_xml($weather_xml);
            $this->_current_weather = $this->_parse_weather_xml($weather_xml);
        }
    }

    // I am not a big fan of passing through properties and getting them dynamically,
    // unless you are expecting dynamic data back from the service you are calling
    // the user has to know the parameters _temperature, _symbol_url and type them correctly
    // if you use clearly defined functions then auto-complete on ide's will work and it makes life easier
    // the less the user needs to know about the inner workings the better

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

    public function get_weather() {
        $this->_check_weather();

        return isset($this->_current_weather['symbol']['id']) ? $this->_current_weather['symbol']['id'] : 'Unknown';
    }

    public function get_temperature() {
        $this->_check_weather();

        return isset($this->_current_weather['temperature']) ? $this->_current_weather['temperature'] : 'Unknown';
    }

    public function get_symbol_url() {
        $this->_check_weather();

        return isset($this->_current_weather['symbol']['number']) ? self::URL_IMG."?symbol=".$this->_current_weather['symbol']['number'].";content_type=image/png" : NULL;
    }

//    private function _get_weather_xml($_latitude, $_longitude)
    private function _get_weather_xml()
    {
        $weather_xml = simplexml_load_file(self::URL_API."?lat={$this->_latitude};lon={$this->_longitude}");
        return $weather_xml;
    }

    private function _parse_weather_xml($weather_xml)
    {
        // it good how you have kept parse weather a separate function to get weather xml
        // this way you can write a unittest and pass in dummy xml and inspect the response

        // don't use define() in a function, what if we call that function twice, will if define it twice and wipe out previous values?
        // you would be better off with a const that is referenced in this class's namespace only (moved code to top of class)

        //define('TZ_OFFSET', 60*60*1);


        $now = time();
        $current_hour = $now - ($now % (60*60));

        $current_weather = array();

        foreach ($weather_xml->product->time as $time){
            $from_hour = strtotime($time['from']) - self::TZ_OFFSET_SECONDS;
            $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'])){
                            $current_weather['temperature'] = $time->location->temperature['value'];
                        }
                    }
                }
            }
        }
     //   $this->_weather = $current_weather['symbol']['id'];
     //   $this->_temperature = $current_weather['temperature'];
     //   $this->_symbol_url = self::URL_IMG."?symbol=".$current_weather['symbol']['number'].";content_type=image/png";
        return $current_weather;
    }
}

// Instantiate Weather object, parameters: latitude, longitude
$weather = new Weather(59.32893000000001, 18.06491);

//echo $weather->get_weather("_weather")."<br />";
//echo $weather->get_weather("_temperature")."<br />";
//echo "<img src=".$weather->get_weather("_symbol_url")." />";


echo $weather->get_weather()."<br />";
echo $weather->get_temperature()."<br />";
echo "<img src=".$weather->get_symbol_url()." />";
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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