Critique my code

[Andy-H]

<?php
interface Geocoding
{
   public function __construct($url, $content_type);
   public function request($formatted);
}
class GetContents
{
   protected function _request($url)
   {
      if(ini_get('allow_url_fopen') != 1)
      {
         $ch = curl_init();
         curl_setopt($ch, CURLOPT_URL, $url);
         curl_setopt($ch, CURLOPT_FAILONERROR, 1);
         curl_setopt($ch, CURLOPT_RETURNTRANSFER,1);
         curl_setopt($ch, CURLOPT_TIMEOUT, 3);
         $contents = curl_exec($ch);
         curl_close($ch);
      }
      else
      {
         $contents = file_get_contents($url);
      }
      return $contents;
   }
}
class Geo extends GetContents
{
   protected $_url, $_content_type;
   
   public function __construct($url, $content_type)
   {
      $this->_url          = $url;
      $this->_content_type = $content_type;
   }
   public function request($formatted = false)
   {
      return ($formatted) ? $this->_request_formatted($this->_url) : $this->_request($this->_url);
   }
}
class Geocode extends Geo implements Geocoding
{
   protected function _request_formatted($url)
   {
      $contents = $this->_request($url);
      if ( stristr($this->_content_type, 'xml') !== false )
      {
         $contents = (array)simplexml_load_string($contents);
         $key      = 'result';
      }
      else
      {
         $contents = (array)json_decode($contents);
         $key      = 'results';
      }
      if ( $contents['status'] != 'OK' )
         return array('status' => 'ERROR', 'message' => 'No results returned');
      if ( key($contents[$key]) )
      {
         $res = array($contents[$key]);
      }
      else
      {
         $res     = $contents[$key];
      }
      unset($contents);
      $results = array();
      foreach($res as $result)
      {
         array_push($results, array(
            'address'  => (is_string($result->formatted_address) ? $result->formatted_address : current($result->formatted_address)),
            'location' => (array)$result->geometry->location,
            'viewport' => array(
               'NE' => (array)$result->geometry->viewport->northeast,
               'SW' => (array)$result->geometry->viewport->southwest
            ),
            'bounds' => array(
               'NE' => (array)$result->geometry->bounds->northeast,
               'SW' => (array)$result->geometry->bounds->southwest
            )
         ));
      }
      $results['length'] = count($results);
      $results['status'] = 'OK';
      return $results;
   }
}
class ReverseGeocode extends Geo implements Geocoding
{
   protected function _request_formatted($url)
   {
      $contents = $this->_request($url);
      if ( stristr($this->_content_type, 'xml') !== false )
      {
         $contents = (array)simplexml_load_string($contents);
         $key      = 'result';
         $comp     = 'address_component';
         $type     = 'type';
      }
      else
      {
         $contents = (array)json_decode($contents);
         $key      = 'results';
         $comp     = 'address_components';
         $type     = 'types';
      }
      if ( $contents['status'] != 'OK' )
         return array('status' => 'ERROR', 'message' => 'No results returned');
      if ( key($contents[$key]) )
      {
         $result = $contents[$key];
      }
      else
      {
         $result = current($contents[$key]);
      }
      unset($contents);
      $result = (array)$result;
      $res = array();
      foreach($result[$comp] as $addr)
      {
         $addr = (array)$addr;
         if ( !isset($addr[$type]) ) continue;
         if ( is_array($addr[$type]) ) $addr[$type] = current($addr[$type]);
         switch(strtolower($addr[$type]))
         {
            case 'street_number' :
               $res['street_number']   = $addr['long_name'];
            break;
            case 'route' :
               $res['street_name']     = $addr['long_name'];
            break;
            case 'locality' :
               $res['town']            = $addr['long_name'];
            break;
            case 'country' :
               $res['country']         = $addr['long_name'];
               $res['country_code']    = $addr['short_name'];
            break;
            case 'postal_code_prefix' :
               $res['postcode_prefix'] = $addr['long_name'];
            break;
         }
      }
      $res['address'] = $result['formatted_address'];
      $res['status']  = 'OK';
      return $res;
   }
}
class Geocoder
{
   private $_settings;
   /**
     *   __CONSTRUCT($type = 'json', $sensor = false, $region = null, $bounds = null, $lang = 'en')
     *   __CONSTRUCT(
     *      array(
     *         'type'   => 'json', 
     *         'sensor' => false, 
     *         'region' => null, 
     *         'bounds' => null, 
     *         'lang'   => 'en'
     *      )
     *   );
     */
   public function __construct()
   {
      $this->_settings = array(
         'type'   => 'json',
         'sensor' => false,
         'region' => null,
         'bounds' => null,
         'lang'   => 'en'
      );
      $arguments = func_get_args();
      if ( count($arguments) )
      {
         if ( count($arguments) == 1 && is_array($arguments[0]) )
         {
            $this->_settings = array_merge($this->_settings, $arguments[0]);
            return false;
         }
         foreach($this->_settings as $k => $v)
         {
            $this->_settings[$k] = current($arguments);
            next($arguments);
         }
      }
   }
   public function __set($name, $value)
   {
      if ( !in_array($name, array_keys($this->_settings)) )
         return false;
      $this->_settings[$name] = $value;
   }
   public function __get($name)
   {
      return isset($this->_settings[$name]) ? $this->_settings[$name] : false;
   }
   public function geocode()
   {
      $args = func_get_args();
      if ( is_array($args[0]) )
      {
         $args = implode(',', $args[0]);
      }
      else
      {
         $args = implode(',', $args);
      }
      $url  = 'http://maps.googleapis.com/maps/api/geocode/';
      $url .= urlencode($this->_settings['type']) .'?';
      $url .= 'address='. urlencode($args);
      $url .= '&sensor='. ($this->_settings['sensor'] ? 'true' : 'false');
      if ( $this->_settings['region'] )
         $url .= '&region='. urlencode($this->_settings['region']);
      if ( $this->_settings['bounds'] )
      {
         if ( is_array($this->_settings['bounds']) )
         {
            if ( count(current($this->_settings['bounds'])) == 4 )
            {
               $this->_settings['bounds'] = array_chunk($this->_settings['bounds'], 2);
            }
            $this->_settings['bounds'] = array_slice($this->_settings['bounds'], 0, 2);
            $url .= '&bounds='. urlencode(implode(',', $this->_settings['bounds'][0])) .'|'. urlencode(implode(',', $this->_settings['bounds'][1]));
         }
         elseif ( is_string($this->_settings['bounds']) )
         {
            $url .= '&bounds='. urlencode($this->_settings['bounds']);
         }
      }
      $url .= '&language='. urlencode($this->_settings['lang']);
      return new Geocode($url, $this->_settings['type']);
   }
   public function reverseGeocode()
   {
      $args = func_get_args();
      if ( count($args) < 2 && !is_array($args[0]) )
         throw new Exception('Invalid arguments passed to Geocoder::reverseGeocode (expected :- (lat, lng) || (array(lat, lng)))');
      if ( is_array($args[0]) )
      {
         $args[0] = array_filter($args[0], 'is_numeric');
         $lat     = current($args[0]);
         $lng     = next($args[0]);
      }
      else
      {
         $lat = $args[0];
         $lng = $args[1];
      }
      $url  = 'http://maps.googleapis.com/maps/api/geocode/';
      $url .= urlencode($this->_settings['type']) .'?';
      $url .= 'latlng='. urlencode($lat .','. $lng);
      $url .= '&sensor='. ($this->_settings['sensor'] ? 'true' : 'false');
      return new ReverseGeocode($url, $this->_settings['type']);
   }
}

Just a geocoding class I've just written, not clued-up on OOP so just looking for advice on what I could have done better, what I've done well etc.

 

 

Thanks

[KevinM1]

You're not using interfaces correctly.  For one, it's highly irregular to specify a constructor in an interface.  You may also get into trouble considering where you implement the request method.  Finally, nothing you're doing really requires an interface.  Interfaces are best used when you're relying on polymorphism.  In this case, that would mean when the request method must be implemented, but the implementation can vary.  Since your request method is really set in the GetContents class (which is a horrible name, BTW - class names should be nouns, not verbs), you don't gain anything.  Rather, you merely add a layer of unneeded complexity.

 

IMO, you should simply use an abstract base class and derive from it.

[Andy-H]

Haven't studied OOP in a while, completely forgot that abstract classes even existed - not that I would have know to use one if I hadn't. I just thought that making a getcontents class, I could re-use that in anything that needed to request contents lol, why was this a bad idea, I still don't understand.

 

 

Ok, so it's good practice to use an abstract class in cases where multiple classes share methods and common functionality, but it's still functionality applicable to only a small set of classes, if it's common behaviour, I should create a common base class to encapsulate this behaviour to extend from, is that right?

 

 

This is the updated code, anything to expand on or improve it?

 

 

<?php
abstract class Geocoding
{
   protected $_url, $_content_type;
   
   public function __construct($url, $content_type)
   {
      $this->_url          = $url;
      $this->_content_type = $content_type;
   }
   public function get($formatted = false)
   {
      return ($formatted) ? $this->_request_formatted($this->_url) : $this->_request($this->_url);
   }
   protected function _request($url)
   {
      if(ini_get('allow_url_fopen') != 1)
      {
         $ch = curl_init();
         curl_setopt($ch, CURLOPT_URL, $url);
         curl_setopt($ch, CURLOPT_FAILONERROR, 1);
         curl_setopt($ch, CURLOPT_RETURNTRANSFER,1);
         curl_setopt($ch, CURLOPT_TIMEOUT, 3);
         $contents = curl_exec($ch);
         curl_close($ch);
      }
      else
      {
         $contents = file_get_contents($url);
      }
      return $contents;
   }
}
class Geocode extends Geocoding
{
   protected function _request_formatted($url)
   {
      $contents = $this->_request($url);
      if ( stristr($this->_content_type, 'xml') !== false )
      {
         $contents = (array)simplexml_load_string($contents);
         $key      = 'result';
      }
      else
      {
         $contents = (array)json_decode($contents);
         $key      = 'results';
      }
      if ( $contents['status'] != 'OK' )
         return array('status' => 'ERROR', 'message' => 'No results returned');
      if ( key($contents[$key]) )
      {
         $res = array($contents[$key]);
      }
      else
      {
         $res     = $contents[$key];
      }
      unset($contents);
      $results = array();
      foreach($res as $result)
      {
         array_push($results, array(
            'address'  => (is_string($result->formatted_address) ? $result->formatted_address : current($result->formatted_address)),
            'location' => (array)$result->geometry->location,
            'viewport' => array(
               'NE' => (array)$result->geometry->viewport->northeast,
               'SW' => (array)$result->geometry->viewport->southwest
            ),
            'bounds' => array(
               'NE' => (array)$result->geometry->bounds->northeast,
               'SW' => (array)$result->geometry->bounds->southwest
            )
         ));
      }
      $results['length'] = count($results);
      $results['status'] = 'OK';
      return $results;
   }
}
class ReverseGeocode extends Geocoding
{
   protected function _request_formatted($url)
   {
      $contents = $this->_request($url);
      if ( stristr($this->_content_type, 'xml') !== false )
      {
         $contents = (array)simplexml_load_string($contents);
         $key      = 'result';
         $comp     = 'address_component';
         $type     = 'type';
      }
      else
      {
         $contents = (array)json_decode($contents);
         $key      = 'results';
         $comp     = 'address_components';
         $type     = 'types';
      }
      if ( $contents['status'] != 'OK' )
         return array('status' => 'ERROR', 'message' => 'No results returned');
      if ( key($contents[$key]) )
      {
         $result = $contents[$key];
      }
      else
      {
         $result = current($contents[$key]);
      }
      unset($contents);
      $result = (array)$result;
      $res = array();
      foreach($result[$comp] as $addr)
      {
         $addr = (array)$addr;
         if ( !isset($addr[$type]) ) continue;
         if ( is_array($addr[$type]) ) $addr[$type] = current($addr[$type]);
         switch(strtolower($addr[$type]))
         {
            case 'street_number' :
               $res['street_number']   = $addr['long_name'];
            break;
            case 'route' :
               $res['street_name']     = $addr['long_name'];
            break;
            case 'locality' :
               $res['town']            = $addr['long_name'];
            break;
            case 'country' :
               $res['country']         = $addr['long_name'];
               $res['country_code']    = $addr['short_name'];
            break;
            case 'postal_code_prefix' :
               $res['postcode_prefix'] = $addr['long_name'];
            break;
         }
      }
      $res['address'] = $result['formatted_address'];
      $res['status']  = 'OK';
      return $res;
   }
}
class Geocoder
{
   private $_settings;
   /**
     *   __CONSTRUCT($type = 'json', $sensor = false, $region = null, $bounds = null, $lang = 'en')
     *   __CONSTRUCT(
     *      array(
     *         'type'   => 'json', 
     *         'sensor' => false, 
     *         'region' => null, 
     *         'bounds' => null, 
     *         'lang'   => 'en'
     *      )
     *   );
     */
   public function __construct()
   {
      $this->_settings = array(
         'type'   => 'json',
         'sensor' => false,
         'region' => null,
         'bounds' => null,
         'lang'   => 'en'
      );
      $arguments = func_get_args();
      if ( count($arguments) )
      {
         if ( count($arguments) == 1 && is_array($arguments[0]) )
         {
            $this->_settings = array_merge($this->_settings, $arguments[0]);
            return false;
         }
         foreach($this->_settings as $k => $v)
         {
            $this->_settings[$k] = current($arguments);
            next($arguments);
         }
      }
   }
   public function __set($name, $value)
   {
      if ( !in_array($name, array_keys($this->_settings)) )
         return false;
      $this->_settings[$name] = $value;
   }
   public function __get($name)
   {
      return isset($this->_settings[$name]) ? $this->_settings[$name] : false;
   }
   public function geocode()
   {
      $args = func_get_args();
      if ( is_array($args[0]) )
      {
         $args = implode(',', $args[0]);
      }
      else
      {
         $args = implode(',', $args);
      }
      $url  = 'http://maps.googleapis.com/maps/api/geocode/';
      $url .= urlencode($this->_settings['type']) .'?';
      $url .= 'address='. urlencode($args);
      $url .= '&sensor='. ($this->_settings['sensor'] ? 'true' : 'false');
      if ( $this->_settings['region'] )
         $url .= '&region='. urlencode($this->_settings['region']);
      if ( $this->_settings['bounds'] )
      {
         if ( is_array($this->_settings['bounds']) )
         {
            if ( count(current($this->_settings['bounds'])) == 4 )
            {
               $this->_settings['bounds'] = array_chunk($this->_settings['bounds'], 2);
            }
            $this->_settings['bounds'] = array_slice($this->_settings['bounds'], 0, 2);
            $url .= '&bounds='. urlencode(implode(',', $this->_settings['bounds'][0])) .'|'. urlencode(implode(',', $this->_settings['bounds'][1]));
         }
         elseif ( is_string($this->_settings['bounds']) )
         {
            $url .= '&bounds='. urlencode($this->_settings['bounds']);
         }
      }
      $url .= '&language='. urlencode($this->_settings['lang']);
      return new Geocode($url, $this->_settings['type']);
   }
   public function reverseGeocode()
   {
      $args = func_get_args();
      if ( count($args) < 2 && !is_array($args[0]) )
         throw new Exception('Invalid arguments passed to Geocoder::reverseGeocode (expected :- (lat, lng) || (array(lat, lng)))');
      if ( is_array($args[0]) )
      {
         $args[0] = array_filter($args[0], 'is_numeric');
         $lat     = current($args[0]);
         $lng     = next($args[0]);
      }
      else
      {
         $lat = $args[0];
         $lng = $args[1];
      }
      $url  = 'http://maps.googleapis.com/maps/api/geocode/';
      $url .= urlencode($this->_settings['type']) .'?';
      $url .= 'latlng='. urlencode($lat .','. $lng);
      $url .= '&sensor='. ($this->_settings['sensor'] ? 'true' : 'false');
      return new ReverseGeocode($url, $this->_settings['type']);
   }
}

 

 

Implementation:

 

 

$latlng = array(53.4829164, -2.0599362);
include 'api/classes/Geocoder.class.php';
$Geocoder = new Geocoder();
echo '<pre>'. print_r($Geocoder->geocode('SK15 1AS')->get(true), 1);
echo '<pre>'. print_r($Geocoder->reverseGeocode($latlng)->get(true), 1);

Array ( [0] => Array ( [address] => Stalybridge SK15 1AS, UK [location] => Array ( [lat] => 53.4829164 [lng] => -2.0599362 ) [viewport] => Array ( [NE] => Array ( [lat] => 53.484052280292 [lng] => -2.0589575 ) [sW] => Array ( [lat] => 53.481354319709 [lng] => -2.0620962 ) ) [bounds] => Array ( [NE] => Array ( [lat] => 53.483383 [lng] => -2.0589575 ) [sW] => Array ( [lat] => 53.4820236 [lng] => -2.0620962 ) ) ) [length] => 1 [status] => OK ) Array ( [street_name] => Caroline St [town] => Stalybridge [country] => United Kingdom [country_code] => GB [postcode_prefix] => SK15 [address] => Caroline St, Stalybridge SK15, UK [status] => OK )

[pre]Thanks for your help Kevin.[/pre]

[KevinM1]

Haven't studied OOP in a while, completely forgot that abstract classes even existed - not that I would have know to use one if I hadn't. I just thought that making a getcontents class, I could re-use that in anything that needed to request contents lol, why was this a bad idea, I still don't understand.

 

GetContents was a one-method class.  Something like that is often better represented as a trait (available only in PHP 5.4 - see: traits).  In lieu of that, make it a static method, and don't inherit from it.  Inheritance creates is-a relationships.  Does it make sense for a Geo object to also be treated as a GetContents object?

 

I also had an issue with the class name - GetContents - itself.  Class names should be nouns as they are used to represent things: a database connection, a MVC controller, a data transfer container, etc.  Even in environments that are more OO oriented and have events baked in, events are represented by an Event class, not a DoSomething class.

 

Ok, so it's good practice to use an abstract class in cases where multiple classes share methods and common functionality, but it's still functionality applicable to only a small set of classes, if it's common behaviour, I should create a common base class to encapsulate this behaviour to extend from, is that right?

 

Generally speaking, yes.  Abstract base classes provide the foundation of functionality which can then be layered upon by concrete child classes.  Interfaces, while on the surface appear to do the same job, are actually used when you want to keep an object's public interface (its public method signatures) the same, but vary the implementation behind the scenes.  The best example I can think of is Dependency Injection, which is what really opened my eyes to the way interfaces are supposed to be used.

 

Let's say you're working in a MVC environment.  In your controller, you need to be able to access the database for basic CRUD functionality.  Instead of directly accessing the db, you use a repository.  So, you have a setup like this:

 

class BlogController extends Controller {
   private $repo;

   public function __construct($repo) {
      $this->repo = $repo;
   }

   public function saveBlogPost() {
      $this->repo->update();
   }
}

public class BlogRepository {
   private $dbc; // PDO or MySQLi

   public function __construct($dbc) {
      $this->dbc = $dbc;
   }

   public function update() {
      // grab $_POST data and update the db through the $dbc
   }
}

 

That works great, but what if you need to test the repository and don't want to access the database with the tests, so as to not fill the database with crap test data?  Instead of commenting out code, you put the repository method signatures in an interface, and have both the 'real' and test repositories implement them:

 

interface IRepository {
   public function get($id);
   public function update();

   // etc.
}

class BlogController extends Controller {
   private $repo;

   public function __construct(IRepository $repo) {
      $this->repo = $repo;
   }

   public function saveBlogPost() {
      $this->repo->update();
   }
}

class BlogRepository implements IRepository { /* code that accesses the db */ }
class TestRepository implements IRepository { /* code that does NOT access the db */ }

 

The controller doesn't care about which repository it gets, as both are of the type IRepository.  This is one way that OO code becomes modular.  There's the adage "Code to an interface (meaning an object's public method signatures), not an implementation."  The language construct interface allows us to do that by specifying an object's public method signatures, leaving the implementation up to individual classes to define.

 

Admittedly, something like the above isn't all that necessary in PHP since it's dynamically typed.  $repo could be anything, and PHP wouldn't care.  In statically typed languages (C++, Java, C#), proper use of interfaces is vital.

 

So, after all that, rules of thumb:

 

Use an abstract base class when: You need to share particular implementations across objects, but don't need to instantiate the base class itself.

Use an interface when: You need to keep the method signatures the same for a group of objects, but also need to change how they work under the hood.  Think plug-ins/modules.

 

---

 

One thing, in your code, I'd just put the method declaration of _request_formatted in your Geocoding class definition, like so:

 

abstract class Geocoding
{
   protected $_url, $_content_type;
   
   public function __construct($url, $content_type)
   {
      $this->_url          = $url;
      $this->_content_type = $content_type;
   }
   public function get($formatted = false)
   {
      return ($formatted) ? $this->_request_formatted($this->_url) : $this->_request($this->_url);
   }
   protected function _request($url)
   {
      if(ini_get('allow_url_fopen') != 1)
      {
         $ch = curl_init();
         curl_setopt($ch, CURLOPT_URL, $url);
         curl_setopt($ch, CURLOPT_FAILONERROR, 1);
         curl_setopt($ch, CURLOPT_RETURNTRANSFER,1);
         curl_setopt($ch, CURLOPT_TIMEOUT, 3);
         $contents = curl_exec($ch);
         curl_close($ch);
      }
      else
      {
         $contents = file_get_contents($url);
      }
      return $contents;
   }

   protected function _request_formatted($url);
}

 

With the way you have it now, Geocoding can't 'see' it as it's defined in the child classes.

[Andy-H]

Wow, thanks, I think this OO stuff is finally starting to click (at last lol). I'll implement that into the abstract class, thanks again for you help, is there any way I can "faviroute" a PHP freaks topic for future reference?

[KevinM1]

AFAIK, we don't have 'save as a favorite topic' functionality.  Saving it as a browser bookmark should work.

[Andy-H]

Ahh OK, just because I'm at work, will email it to myself, cheers.

[KevinM1]