2
\$\begingroup\$

I wrote a little PHP wrapper class for GeoIP which looks like:

<?php
class GeoIP {   
    private $ip;
    private $geo_ip_details;

    function __construct($ip) {
        $this->ip = $ip;
        $this->fetch();
    }

    private function fetch() {
        if(filter_var($this->ip, FILTER_VALIDATE_IP)) {
            $curl = new Curl();
            $json = $curl->get_request("http://freegeoip.net/json/" . $this->ip, true);
            $result = json_decode($json);

            if($result !== null) {
                $this->geo_ip_details = $result;
            }
        }
    }

    public function __get($property) {
        if(property_exists($this, $property)) {
            return $this->$property;
        }
    }

    public function __toString() {
        if(isset($this->geo_ip_details) && !empty($this->geo_ip_details)) {
            if(isset($this->geo_ip_details->city) && !empty($this->geo_ip_details->city) && isset($this->geo_ip_details->region_name) && !empty($this->geo_ip_details->region_name) && isset($this->geo_ip_details->country_name) && !empty($this->geo_ip_details->country_name)) {
                return $this->geo_ip_details->city . ", " . $this->geo_ip_details->region_name . " " . $this->geo_ip_details->country_name;
            } else if(isset($this->geo_ip_details->region_name) && !empty($this->geo_ip_details->region_name) && isset($this->geo_ip_details->country_name) && !empty($this->geo_ip_details->country_name)) {
                return $this->geo_ip_details->region_name . " " . $this->geo_ip_details->country_name;
            } else if(isset($this->geo_ip_details->country_name) && !empty($this->geo_ip_details->country_name)) {
                return $this->geo_ip_details->country_name;
            }
        }
    }
}
?>

Usage is like:

//Constructor, and autocalls fetch()
$geoip = new GeoIP('64.87.28.98');

//Getter of ip property 
print_r($geoip->ip);

//Getter of geo_ip_details property
print_r($geoip->geo_ip_details);

//toString of the class 
echo $geoip;

Basically, I am going back and forth on implementation details. Should I expose fetch() publicly, and not automatically call it in the constructor? I.E.

$geoip = new GeoIP('64.87.28.98');
$result = $geoip->fetch();

Is using a getter a good idea? Should I have a class method called stringify() instead of overriding the __toString?

Also, right now, once you instantiate GeoIP with an ip address, there is no way to pass a new ip address into the existing object. You must instantiate a new GeoIP object. Does making a method:

public function set($ip) {
    $this->ip = $ip;
    $this->fetch();
}

Make sense, or simply just do something like: $geoip = new GeoIP('new-ip-address');

What is the best practice for object oriented design for this pattern?

\$\endgroup\$
4
\$\begingroup\$
  • I'm not a fan of magic getters. Without really good documentation, they're just a pain for consumers of your code. For example, when I first looked at your code, I had no idea at all what properties the class exposes. I basically had to follow the cURL request to find out. Proper documentation could fix this, but why not just have proper properties instead (or a getter)?

  • As a general rule of thumb, a class should never successfully construct if it will be left in a useless state. In other words, your constructor shouldn't finish unless the IP request actually went through and the data was retrieved.

    • If the IP is invalid, throw an exception.
    • If the cURL request fails, throw an exception
  • cURL isn't really necessary here (at least not with the way you're using it). Just use file_get_contents("http://.../...");

  • Your getter should return null if the property doesn't exist. null will be returned implicitly, but I think the code would be a bit clearer (alternatively, you could throw an exception depending on how you want the class to be used)

  • isset($var) && !empty($var) is redundant. !empty($var) is equivalent to isset($var) && $var meaning you can just do !$empty.

  • Your __toString doesn't have a real use at the moment. The return is too unpredictable to reliably use it to print the data. I would either normalize it to always print the same fields in the same format or get rid of it.

    • I've not a fan of __toString for most classes. Unless a class has a canonical string represenation (like a BigInt would, or a IPAddress, etc), I wouldn't bother having a __toString.
    • __toString can ease debugging though, so if you like to use it for that, it might be worth keeping it around
  • I think you need to decouple the data object (the city/state/area code/etc) from the retrieval of it. I would have two classes: GeoIP and GeoIPInformation (or whatever you want to call it).

    • GeoIP would then have a method public function getInformation($ipAddress); that returns a GeoIPInformation.
      • if you think there's a chance you'd ever want to have a different method for ipv4 or ipv6 or the ability to lookup host names, you might want to name it getInformationByIPv4
    • If you wanted to go all out, you could code the retriever to an interface. This would remove your ties to freegeoip.net (which might stop existing or working at any time)
      • A second retriever that immediately comes to mind is the maxmind geoip databases
      • Interfaces are good for decoupling, blah blah blah... ask me if you more details on this.

Update

Dave Jarvis had a very good point with the encapsulation issue. Concepts like a country should be stored as an object, not a simple scalar. This allows the consuming code to interact with the data in a much cleaner way. For example, imagine that one data source returns a 2 letter country code whereas another returns a country name. If the coutnry is a plain string, this presents a problem: how do we know what echo $loc->getCountry(); is going to be? Is it going to be a country code? Or a name? Or maybe an ID? Who knows.

But, if instead of leaving it a scalar we turn it into an object, we get the benefit of being able to do echo $loc->getCountry()->getName(); and knowing that it will be a proper name. There's an added benefit too of we now have much more data at hand. We are no longer constrained to one small snippet of data about the country. We now have the ability to have the name, code, etc all wrapped up in one convenient object.

Different strategies are possible to create these objects, but for your uses, I would just stick with a simple lookup table or something similar in this situation.

If you wanted to go all out, you could connect it to some kind of data source and then have your code interact with that to load a country. This would be a lot more flexible as the data is no longer part of the code (which tends to be bad), but it's also more complicated, and I'm lazy. If you did go this route, you could create a CountryFinder interface that knows how to create a Country based on a given criteria. You would then inject a CountryFinder implementation into your GeoIP implementations and it would use it.

I've updated my code below to reflect a basic sketch of these updates. I've only demonstrated by encapsulating the IP address and country, but hopefully the idea is clear. (Also, my error handling is pretty terrible in a few places such as the IP address constructor where an exception should be thrown if the argument is invalid).

Oh, and Location should either be named IpLocation to signify that it embodies a Location/IP pair, or it should have the IP address removed from it. (A Location object really has no business having an IP address as an attribute, but as it is actually GeoIP::Location, I might be tempted to leave it.)


All in all, I might consider something like this:

Note that this is a bit crude -- the error handling is bad, the exceptions should be subclasses, I put half a second of thought into names, etc

<?php

namespace GeoIP;

interface GeoIP
{
    /**
     * Retrieves geo information for an IP address.
     * @param string|IpAddress $ipAddress Either an IpAddress or an IP address formatted as a string in dotted decimal notation
     * @return \GeoIP\Location 
     * @throws \InvalidArgumentException if the IP address is invalidly formatted
     * @throws \RuntimeException if some kind of network/disk/etc error occurs
    */
    public function getLocationByIpAddress($ipAddress);
}

class IpAddress
{

    private $_addr;

    public function __construct($addr)
    {
        //You can either guess what $addr is (string, 32 bit int, etc),
        //or you could provide static fromString, fromLong, etc methods.
        //For simplicity, I've assumed it's a decimal dotted string
        $this->_addr = $addr;
    }

    public function asDecimalDotString()
    {
        return $this->_addr;
    }

    public function asLong()
    {
        $octets = explode(".", $this->_addr);
        return ($octets[0] << 24) + ($octets[1] << 16) + ($octets[2] << 8) + $octets[3];
    }

}

class Location
{
    private $_ip;
    private $_country;

    public function __construct($ip, Country $country)
    {
        $this->_ip = $ip;
        $this->_country = $country;
    }

    /**
     * @return IpAddress The IP address that was used in retrieving this geo information
    */
    public function getIpAddress()
    {
        return $this->_ip;
    }

    /**
     * @return Country|null the country the IP belongs to or null is one could not be determined
    */
    public function getCountry()
    {
        return $this->_country;
    }

    // ...

}

interface CountryFinder
{
    public function findByCountryCode($code);
    public function findByCountryName($name);
}

class Country
{

    private $_name;
    private $_code;
    private $_population;

    public function __construct($name, $code, $population)
    {
        $this->_name = $name;
        $this->_code = $code;
        $this->_population = $population;
    }

    public function getName()
    {
        return $this->_name;
    }

    public function getCode()
    {
        return $this->_code;
    }

    public function getPopulation()
    {
        return $this->_population;
    }

}

class ArrayLookupCountryFinder implements CountryFinder
{

    private $_countryData = array(
        'US' => array('name' => 'United States', 'code' => 'US', 'population' => 300000000),
        'CA' => array('name' => 'Canada', 'code' => 'CA', 'population' => 34500000)
    );

    public function __construct(array $countryData = null)
    {
        if ($countryData) {
            $this->_countryData = $countryData;
        }
    }

    public function findByCountryCode($code)
    {
        if (isset($this->_countryData[$code])) {
            $country = $this->_countryData[$code];
            return new Country($country['name'], $country['code'], $country['population']);
        } else {
            return null;
        }
    }

    public function findByCountryName($name)
    {
        // Yeah...
        return null;
    }

}

class FreeGeoipNet implements GeoIP
{

    private $_countryFinder;

    public function __construct(CountryFinder $countryFinder)
    {
        $this->_countryFinder = $countryFinder;
    }

    public function getLocationByIpAddress($ipAddress)
    {

        if (!($ipAddress instanceof IpAddress)) {
            $ipAddress = new IpAddress($ipAddress);
        }

        $response = file_get_contents("http://freegeoip.net/json/" . $ipAddress->asDecimalDotString());
        if ($response === false) {
            throw new \RuntimeException("Request to freegeoip.net failed");
        }
        $data = json_decode($response, true);
        if (!is_array($data)) {
            throw new \RuntimeException("Parsing response from freegeoip.net failed: {$response}");
        }
        /* In a real application, you would likely want to somehow handle if the country code wasn't empty but a
         * country couldn't be found in your data source. I'm lazy though, so I've not done that. */
        return new Location($ipAddress, $this->_countryFinder->findByCountryCode($data['country_code']));
    }
}

Usage:

$geoip = new GeoIP\FreeGeoipNet(new GeoIP\ArrayLookupCountryFinder());

$location = $geoip->getLocationByIpAddress("x.x.x.x");

$ip = $location->getIpAddress();
$country = $location->getCountry();

echo $ip->asDecimalDotString() . " (" . $ip->asLong(). ") " . " is in " . $location->getCountry()->getName() 
     . " which has a population of " . $location->getCountry()->getPopulation();
\$\endgroup\$
  • 1
    \$\begingroup\$ This appears to violate encapsulation. See: javaworld.com/jw-05-2001/jw-0518-encapsulation.html?page=9 \$\endgroup\$ – Dave Jarvis Jul 24 '13 at 16:40
  • \$\begingroup\$ @DaveJarvis Which part(s)? If you mean the getters, there's not much of a way to avoid those since it's a value object. \$\endgroup\$ – Corbin Jul 24 '13 at 22:16
  • 1
    \$\begingroup\$ The accessor methods should return immutable objects, not strings. This would help alleviate any dependencies that arise from clients relying on the format of the strings that are returned. You could write $info->getCountry()->is( "Canada" ), for example. How the country code is stored (2- vs. 3-letter ISO codes) should not be a concern of the client. \$\endgroup\$ – Dave Jarvis Jul 25 '13 at 1:28
  • \$\begingroup\$ @DaveJarvis Ah, that's a good point. I didn't think about it from that perspective. That would allow a ton of advantages working with the data compared to modeling it as simple scalars. I think your suggestion definitely warrants an answer and not just a comment if you want to post one. Otherwise, I will edit my answer in a bit to reflect your suggestion. \$\endgroup\$ – Corbin Jul 25 '13 at 1:32
  • \$\begingroup\$ Do update your answer. :-) \$\endgroup\$ – Dave Jarvis Jul 25 '13 at 4:10

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.