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
}

*(related)* http://stackoverflow.com/questions/5166087/php-global-in-functions/5166527#5166527 — Gordon, Mar 02 '11 at 12:53
Just my two cents but Taylor Otwell of Laravel got heat because of his use of factory pattern static methods and other static methods but guess what 100% unit coverage. — Michael J. Calkins, Jan 02 '14 at 06:46

score 15 · Accepted Answer · answered Mar 02 '11 at 13:44

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.

edorian · Answer 2 · 2011-03-02T12:54:29.400

This blog post should tell you why people don't like static methods better than i could:

http://kore-nordmann.de/blog/0103_static_considered_harmful.html

The question that strikes me most about your current code snippet: Is a Person allowed to NOT have an Id ?

I feel like that should be an constructor argument if it's representing a real Person. If you use that class to create new persons that ofc might not work.

The difference between the 2 calls is minor. Both "create" a Person class and set the Id so you are not winning / loosing anything there when it comes to 'hard wired dependencies'.

The advantage only shows when you want to be able to pass a Person into another object and that objects needs to change the ID (as an example, the blog post should explain that better than i did here).

score 2 · Answer 3 · answered Mar 02 '11 at 13:03

I'm only adding to edorian's post, but I've used static get methods in the past, where there is a caching engine in place, and (for example) I might have a given Person object in memcache, and would rather retrieve it from the cache than going off to the database.

For example:

class Person {

    public static function get($id) {

        if(Cache::contains("Person", $id))
        {
           return Cache::get("Person", $id);
        }
        else
        {           
            //fictional get_person_from_database, basically
            //getting an instance of Person from a database
            $object = get_person_from_database($id);
        }
        return $object;
    }

}

In this way, all cache handling is done by the class in question, rather than the caller getting a person calls having to worry about the cache.

score 1 · Answer 4 · answered Apr 30 '14 at 11:07

long story short, yes, they are bad practice:

A good reason apart of everything is that you 'should' be testing your code. Static methods cause issues, so there you have a good reason:

if you want to follow good practices, test your code
Ergo, if static causes testing issues, static prevent writing tests so it prevents to follow good practices :-)

score 0 · Answer 5 · answered May 07 '19 at 13:30

time goes things changes.

just in case you have problems with testing you can use AspectMock library https://github.com/Codeception/AspectMock

any way static is not so bad at all. to use static you should just know what you are doing and why. if you will place static only as fast solution it is bad idea in 99% of variations. in 1% time it is still bad solution but it gives you time when you need it.

Static method get - is this bad practice?

5 Answers5

Linked