Return to Question

added 378 characters in body

Source Link

edited Jul 24, 2013 at 5:28

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?

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?

Source Link

asked Jul 24, 2013 at 5:16

Justin

PHP review of object oriented GeoIP class

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?

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

php object-oriented