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();
