Static method get - is this bad practice?

Question

Had a discussion with a colleague about wether this is bad practice or not. Now I can not find immediate examples of this online.

We have a lot of database object mappers and call it's functions like so

(example) - the setId method get's the row from the database and set's it to predefined propertys

class Person {

    public static function get($id) {
        $object = new Person;
        $object->setId($id);
        return $object;
    }
}

Using it like this we can use simple constructions like this: (where we got the id from for-example a post)

$person = Person::get($id);

instead of

$person = new Person;
$person->setId($id);

Now, my instinct tells me this is bad practice. But I can not explain it. Maybe someone here can explain why this is, or is not bad practice

Here are some other examples how we use it. we mainly use it for getters. (just the names, not the code. Almost all of them just run a query, which can return 1 object and then use the id of the result to use the setId method)

class CatalogArticle {
   public static function get($id) { }
   public static function getByArticlenumber($articlenumber) {} //$articlenumber is unique in the database
   public static function getRandom() {} //Runs a query returning a random row
}

Answer 1

This isn't horrible persay. It's an implementation of a Factory Method design pattern. It's not bad at all in principle.

However, in your specific example, it's not really doing anything significant, so I'm not so sure if it's necessary. You could eliminate the need by taking a (perhaps optional) parameter to the constructor for the id. Then anyone could call $foo = new Person($id); rather than needing an explicit factory.

But if the instantiation is complex, or you want the ability to build several different people types that can only be determined by logic, a factory method may work better. For example, let's say you need to determine the type of person to instantiate by some parameter. Then, a factory method on Person would be appropriate. The method would determine what "type" to load, and then instantiate that class.

Statics in general are hard to test and don't allow for polymorphic changes like an instance would. They also create hard dependencies between classes in the code. They are not horrible, but you should really think about it if you want to use one. An option would be to use a Builder or a Abstract Factory. That way, you create an instance of the builder/factory, and then let that instance determine how to instantiate the resulting class...

One other note. I would rename that method from Person::get() to something a little more semantically appropriate. Perhaps Person::getInstance() or something else appropriate.