tags:

views:

589

answers:

6

UPDATE: Rephrasing the question to ask, 'are there too many' static methods (I realize that right now there are only 4 but I originally started with 2) in this class structure? If so, any suggestions on how to refactor these classes to use some sort of Finder class so that I can remove the static functions from the Model classes?

I have the following abstract class:

abstract class LP_Model_Abstract
{
protected static $_collectionClass = 'LP_Model_Collection';

protected $_row = null;

protected $_data = array();

public function __construct($row = null)
{
 $this->_row = $row;
}

public function __get($key)
{
 if(method_exists($this, '_get' . ucfirst($key)))
 {
  $method = '_get' . ucfirst($key);
  return $this->$method();   
 }
 elseif(isset($this->_row->$key))
 {
  return $this->_row->$key;
 }
 else
 {
  foreach($this->_data as $gateway)
  {
   if(isset($gateway->$key))
   {
    return $gateway->$key;
   }
  }
 } 
}

public function __set($key, $val)
{
 if(method_exists($this, '_set' . ucfirst($key)))
 {
  $method = '_set' . ucfirst($key);
  return $this->$method($val);   
 }
 elseif(isset($this->_row->$key))
 {
  $this->_row->$key = $val;
  return $this->_row->$key;
 }
 else
 {
  foreach($this->_data as $gateway)
  {
   if(isset($this->_data[$gateway]->$key))
   {
    $this->_data[$gateway]->$key = $val;
    return $this->_data[$gateway]->$key;
   }
  }
 }
}

public function __isset($key)
{
 return isset($this->_row->$key);
}

public function save()
{
 $this->_row->save();
}

abstract public static function get($params);
abstract public static function getCollection($params = null);
abstract public static function create($params);

}

And then this class which provides additional functionality for class table inheritance schemes (where type is important in determining additional functionality in a factory fashion):

abstract class LP_Model_Factory_Abstract extends LP_Model_Abstract
{
    protected static $_collectionClass = 'LP_Model_Collection_Factory';

    abstract public static function factory($row);
}

These ultimately result in the following type of class declaration:

class Model_Artifact extends LP_Model_Factory_Abstract
{
    protected static $_artifactGateway = 'Model_Table_Artifact';

    public static function create($params)
    {

    }

    public static function get($params) 
    {
     $gateway = new self::$_artifactGateway();

     $row = $gateway->fetchArtifact($params);

     return self::factory($row);        
    }

    public static function getCollection($params = null) 
    {
     $gateway = new self::$_artifactGateway();

     $rowset = $gateway->fetchArtifacts($params);

     $data = array(
      'data' => $rowset,
      'modelClass' => __CLASS__
     );

     return new self::$_collectionClass($data);
    }

    public static function factory($row)
    {
     $class = 'Model_Artifact_' . $row->fileType;
    }
}

When do you know that you have too many static methods in a class? And how would you refactor the existing design so that the static methods are perhaps encapsulated in some sort of Finder class?

+2  A: 

The first indicator I use when determining if I have to many static methods is if the methods functionality is not stateless. If the static methods change the state of the object they reside in, they probably shouldn't be static.

Matthew Brubaker
Reminds of a Microsoft joke, technically correct but in this case not terribly useful. Mostly because these methods already don't modify the state of the object, they simply return the object or create a brand new one. But thx for the answer anyway. ;-)
Noah Goodrich
Fair enough. I'm not familiar enough with php to be of much use past conceptual information in this case =(
Matthew Brubaker
gabriel, you asked "When is too many?" not look at my code is this too many...
cgreeno
+3  A: 

I'd have to agree with Brubaker and add that to my thinking it isn't the number of methods so much as the functionality of said methods. If you start thinking that your class has to many methods (static or otherwise) then you might find they can be re-grouped and refactored into a more intuitive architecture.

Al W
Also a good indicator. I didn't even think of adding that since, to me at least, it seems like common sense.
Matthew Brubaker
+1  A: 

I'll throw in my 2 cents.

First of all, I'll agree that setting some sort of arbitrary limit is not helpful, such as "Once I have more than 10 statics in a class that's too many!". Refactor when it makes sense but don't start doing it just because you've hit some imaginary boundary.

I wouldn't 100% agree with Brubaker's comment about stateful vs. stateless - I think the issue is more about classes vs instances. Because a static method can change the value of another static property which is a stateful change.

So, think of it like this - if the method/property is of or pertaining to the class, then it should probably be static. If the method/property is of or pertaining to an instance of the class, it should not be static.

Peter Bailey
+1  A: 

Personally I find that any number of static methods are a sign of trouble. If your class has instance methods and static methods, then most likely you could split the class into two separate entities and change the static methods to instance methods.

Think of a class as a special kind of object, with the distinctive property that it is global by nature. Since it's a global variable, it implies a very strong level of coupling, so you would want to reduce any references to it. Static members will need to be referred, meaning that your code will get a strong level of coupling to the class.

troelskn
@troelskn How would you structure the new class? I'm kind of thinking along these same lines that static methods are a general danger more than a specific one but should be refactored out whenever possible.
Noah Goodrich
If I understand your code correctly, just treat Artifact and ArtifactGateway as two completely separate classes. You could in theory have more than one gateway to handle the same entity (Think non-db storage, or multiple databases)
troelskn
+1  A: 

I agree with BaileyP and I'll add my couple of pennies:

I always work with the idea that a class should have a single reason for existing; it should have one job that it does, and it should do it well. After deciding that, and figuring out what the interface to that class should be, I go through and mark any functions that don't change the state of an instance of the class as static.

dominic hamon
+1  A: 

If you want to build reusable and testable code, you should avoid static methods altogether. Code which calls static methods (or constructors of non-data-like classes) cannot be tested in isolation.

Yes, you will have to pass around alot more objects if you eliminate static methods. This is not necessarily a bad thing. It forces you to think about the boundaries and cooperation between your components in a disciplined way.

Wim Coenen