Is this bad practice?

[Andy-H]

Until participating and reading this topic, I had though the following code was perfectly fine. I don't claim to even be good at OOP or application design, although I would like to be lol

 

 

Anyway, the only functionality regarding google maps polylines I could think of in PHP, was encoding and decoding them, so I placed them in a static class:

 

 

<?php
/*
-- Polyline.class.php
@return string (encoded lat/lng)           - polyline::getEncoded(-2.5675, 2.5456);
@return string (encoded lat/lng's)         - polyline::getEncoded(array(-2.5675, 2.5456), array(-2.5675, 2.5456));
@return string (encoded lat/lng's)         - polyline::getEncoded(-2.5675, 2.5456, -2.5675, 2.5456);

@return array  (decoded array(lat, lng)'s) - polyline::getDecoded('zmtn_?_epn_?zmtn_?_epn_?zmtn_?_epn_?');
*/
class Polyline 
{
private static $_calls   = 0;
private static $_lastLat = 0;
private static $_lastLon = 0;
// public accessors
public static function get_encoded()
{
	// $encoded to store encoded points
	$encoded = '';
	$args = func_get_args();
	if ( is_array($args[0]) )
	{
		while ( list($k, $arg) = each($args) )
			$encoded .= self::_encode($arg[0]) . self::_encode($arg[1]);
	}
	else
	{
		$cnt     = count($args);
		if ( !$cnt )
			return false;
		$i = 0;
		while ( $i < $cnt )
			$encoded .= self::_encode($args[$i++]);
	}
	self::$_calls   = 0;
	self::$_lastLat = 0;
	self::$_lastLon = 0;
	return $encoded;
}
public static function get_decoded($str)
{
	$points = array();
	$lat    = 0;
	$lon    = 0;
	while (strlen($str))
	{
		$lat += self::_decode($str);
		$lon += self::_decode($str);
		$points[] = array($lat * 1e-5, $lon * 1e-5);
	}
	return $points;
}
// private
private static function _encode($dec)
{
	$dec = round($dec * 1e5, 0);
	if ( !(self::$_calls % 2) )
	{
		//lon
		if ( self::$_calls >= 2 )
			$ndec    = $dec - self::$_lastLon;
		self::$_lastLon = $dec;
	}
	else
	{
		//lat
		if ( self::$_calls >= 2 )
			$ndec    = $dec - self::$_lastLat;
		self::$_lastLat = $dec;
	}
	$dec = isset($ndec) ? $ndec : $dec;
	$is_neg  = stristr($dec, '-');
	$dec   <<= 1;
	// invert bits if negative
	if ( $is_neg )
		$dec = (~$dec);
	//0 pad to 32 bits
	$dec     = str_pad(sprintf('%b', $dec), 30, '0', STR_PAD_LEFT);
	// chunk into 5 char strings and reverse
	$dec     = array_reverse(str_split($dec, 5));
	// or with 0x20 except last one ( add 63 to each and convert to ascii )
	$c = count($dec);
	for ( $i = 0; $i < $c; ++$i )
		$dec[$i] = (isset($dec[$i+1]) && $dec[$i+1] & 31) ?  ((bindec($dec[$i]) | 32) + 63) : (((bindec($dec[$i])) > 0) ?  bindec($dec[$i]) + 63 : '');
	// set times called
	self::$_calls++;
	return vsprintf('%c%c%c%c%c%c', $dec);
}
private static function _decode(&$str)
{
	$shift  = 0;
	$result = 0;
	$i      = 0;
	do { // while ascii($str[$i++]) > ascii([space] " ")
		$b       = ord($str[$i++]) - 63;
		$result |= ($b & 0x1f) << $shift;
		$shift += 5;
	} while ($b >= 0x20);
	$str = substr($str, $i);
	return (($result & 1) ? ~($result >> 1) : ($result >> 1));
}
private function __construct() {}
private function __clone()     {}
}
?>

 

 

not that it matters much, since the rest of the application in which this class resides went to design shit, but; is this bad practice in your opinion?

[xyph]

Why is the class static?

[Andy-H]

That's a very good question

 

 

Because I'm an idiot, I'm sure that there was some misguided explanation I had in my head for it at the time, but looking at it, the only reason I can think of is because I'm too lazy to write getInstance() lol

[xyph]

Even then, is there a specific reason you'd need a static instance of the class?

 

Static properties should be avoided. They're nice for some things, like an SQL class that has a static counter (track query counts over several possible instances), otherwise they can turn into a debugging nightmare.

 

Imagine both classA and classB use a shared instance of Polyline. If classA makes changes to properties, it will affect the way classB behaves, and vice versa. If the change is undesired, you now have the joy of tracing down which instance caused the bad change. If it's only 2 methods, it's not that bad. If you later expand the system, it can become problematic.

[Andy-H]

Ahh, I didn't think of that, cheers, will keep that in mind.

[Andy-H]

Should it even be a singleton then?

 

// edit updated to normal class

 

<?php
/*
   @return string (encoded lat/lng)           - polyline::getEncoded(-2.5675, 2.5456);
   @return string (encoded lat/lng's)         - polyline::getEncoded(array(-2.5675, 2.5456), array(-2.5675, 2.5456));
   @return string (encoded lat/lng's)         - polyline::getEncoded(-2.5675, 2.5456, -2.5675, 2.5456);
   
   @return array  (decoded array(lat, lng)'s) - polyline::getDecoded('zmtn_?_epn_?zmtn_?_epn_?zmtn_?_epn_?');
*/
class polyline 
{
   // tracks number of times _encode is called
   private $_calls;
   // stores latitude value from last _encode call
   private $_lastLat;
   // stores longitude value from last _encode call
   private $_lastLon;
   // public accessors
   public function get_encoded()
   {
      // set defaults
      $this->_calls   = 0;
      $this->_lastLat = 0;
      $this->_lastLon = 0;
      // $encoded to store encoded points
      $encoded = '';
      $args = func_get_args();
      if ( is_array($args[0]) )
      {
         while ( list($k, $arg) = each($args) )
            $encoded .= $this->_encode($arg[0]) . $this->_encode($arg[1]);
      }
      else
      {
         $cnt     = count($args);
         if ( !$cnt )
            return false;
         $i = 0;
         while ( $i < $cnt )
            $encoded .= $this->_encode($args[$i++]);
      }
      return $encoded;
   }
   public function get_decoded($str)
   {
      $points = array();
      $lat    = 0;
      $lon    = 0;
      while (strlen($str))
      {
         $lat += $this->_decode($str);
         $lon += $this->_decode($str);
         $points[] = array($lat * 1e-5, $lon * 1e-5);
      }
      return $points;
   }
   // private
   private function _encode($dec)
   {
      $dec = round($dec * 1e5, 0);
      if ( !($this->_calls % 2) )
      {
         //lon
         if ( $this->_calls >= 2 )
            $ndec    = $dec - $this->_lastLon;
         $this->_lastLon = $dec;
      }
      else
      {
         //lat
         if ( $this->_calls >= 2 )
            $ndec    = $dec - $this->_lastLat;
         $this->_lastLat = $dec;
      }
      $dec = isset($ndec) ? $ndec : $dec;
      $is_neg  = stristr($dec, '-');
      $dec   <<= 1;
      // invert bits if negative
      if ( $is_neg )
         $dec = (~$dec);
      //0 pad to 32 bits
      $dec     = str_pad(sprintf('%b', $dec), 30, '0', STR_PAD_LEFT);
      // chunk into 5 char strings and reverse
      $dec     = array_reverse(str_split($dec, 5));
      // or with 0x20 except last one ( add 63 to each and convert to ascii )
      $c = count($dec);
      for ( $i = 0; $i < $c; ++$i )
         $dec[$i] = (isset($dec[$i+1]) && $dec[$i+1] & 31) ?  ((bindec($dec[$i]) | 32) + 63) : (((bindec($dec[$i])) > 0) ?  bindec($dec[$i]) + 63 : '');
      // set times called
      $this->_calls++;
      return vsprintf('%c%c%c%c%c%c', $dec);
   }
   private function _decode(&$str)
   {
      $shift  = 0;
      $result = 0;
      $i      = 0;
      do { // while ascii($str[$i++]) > ascii([space] " ")
         $b       = ord($str[$i++]) - 63;
         $result |= ($b & 0x1f) << $shift;
         $shift += 5;
      } while ($b >= 0x20);
      $str = substr($str, $i);
      return (($result & 1) ? ~($result >> 1) : ($result >> 1));
   }
}
?>

[TimeBomb]

While static classes are gray area in some scenarios, and just plain poor design in others, singletons are very much so antipatterns.

I would suggest making it a normal object, and using dependency injection.

[xyph]

He's done that.

 

Static classes are, in a sense, singleton... you can't really have a singleton without the static property.

 

Singletons can be used effectively, though some design patterns expressly avoid them. They're aren't as dangerous if the class keeps a consistent "state."

 

In this case, there was no need for a static, or singleton class. I agree.

[TimeBomb]

He's done that.

 

Static classes are, in a sense, singleton... you can't really have a singleton without the static property.

 

Singletons can be used effectively, though some design patterns expressly avoid them. They're aren't as dangerous if the class keeps a consistent "state."

 

In this case, there was no need for a static, or singleton class. I agree.

I disagree with the underlined statement. The singleton pattern is an antipattern for a reason. They are unnecessary unless your application is designed poorly. If you effectively use dependency injection, you will never need to use singletons.

[xyph]

This argument has gone on for a while, and the debates are everywhere. There are a lot of very clever people that have no issues with properly implemented singletons, and even see them as necessary in certain situations.

 

If you disagree, that's fine. The Zend Framework uses singleton registries though, so perhaps you could be a little more open-minded as well.