Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have created simple class inheritance of an abstract class Pet which is basically the skeleton of all other children classes.

abstract class Pet {

    private static $relations = array(
        'Cat',
        'Dog'
    );

    public static final function getClassById($id){
        return self::$relations[$id];
    }

    protected static $foods = array(

    );

    public static function getEdibleFoods(){
        $class = get_called_class();
        return $class::$foods;
    }

    abstract public function play();
    abstract public function eat();
    abstract public function sleep();

}

Then I have the Dog and Cat classes

class Dog extends Pet {

    protected static $foods = array(
        'bones'
    );

    public function eat() { }

    public function play() { }

    public function sleep() { }

}

class Cat extends Pet {

    protected static $foods = array(
        'fish'
    );

    public function eat() { }

    public function play() { }

    public function sleep() { }

}

As you see, the only difference between them is the foods that they like. My concern about this code is the getEdibleFoods static method in the parent class.

public static function getEdibleFoods(){
    $class = get_called_class();
    return $class::$foods;
}

In order to not have to implement it in all children classes, I figured I can use get_called_class() instead of self to return the foods static variable. Notice that every child class has its own foods array and they are not all the same.

My problem is that this does not look like clean code. I'd like to ask if this is good practice and if not, how is it done the right way?

share|improve this question

2 Answers 2

up vote 5 down vote accepted

You're assumption that your code is messy is indeed correct. Let's examine how we can fix this:

class A {
    protected static $str = "str1";
    public static function getString() {
        return static::$str;
    }
}
class B extends A {
    protected static $str = "str2";
}
echo B::getString();

Running this will print "arr2" each time. Why?

It's the static:: keyword that does the magic. It's a simple trick called late static binding. It's basically a way to carry parent attributes down the structure of the classes.

Here's the PHP docs, which also explains self:: versus static::. It's a good read.

I'd like to ask if this is good practice and if not

It all depends, everything has a purpose and a correct time to use it. It's up to you to determine if what you're doing is the best implementation.

Would your example be correct usage, yes. However, many people are unfamiliar with late static bindings, and so it could be easy to misinterpret or skip that line if a new developer is reading your code. Perhaps there's a way you could reevaluate your code to see if you can restructure it in a way that does not depend on this functionality.

share|improve this answer
    
I myself am, kind of, a new developer. I, as well, were not aware of this functionality. I'm afraid I'm not fluent enough to restructure this code, as I built it thinking that it was structured pretty well actually. I'd love it if you could give me some pointers on to how I would do this? Thank you for your answer, I shall get to reading some docs now! –  php_nub_qq Aug 24 at 21:06
    
In that case, I doubt you need to "restructure" your code. If you're just playing around, everything is fine. :) –  Alex L Aug 24 at 21:08
    
I'm actually working on a project that has the potential to make it out into the world, sadly now that I've been told my code is not structured well I'm a little demotivated, temporarily. :D –  php_nub_qq Aug 24 at 21:11
1  
It just may be hard to develop like this if your application becomes very large. Having classes reference and act in this way can make it difficult to trace back an error. It's nothing to be demotivated by, it's all a learning experience. When you get knocked down, you gotta get back up! –  Alex L Aug 24 at 21:13
1  
I guess I will just have to learn it the hard way. Thank you for your replies, stay awesome! –  php_nub_qq Aug 24 at 21:15

I think the true problem is that the method shouldn't be static at all.


You should think about static methods as namespaced functions. They logically belong in the same category as the class they're defined on, but they do not actually operate on objects of that class. This means that their use (at least proper use) actually comes up a lot less than you'd think it would.

Typically static methods tend to be utility functions and this is for good reason. Utility methods are basically the only thing that don't have state, and things with state shouldn't be static. As a result of this, if a method is static, it should be idempotent.


Another concern in usability. Having a static method exist on a class hierarchy rather than just the top level ends up in a rather odd situation. Consider code consuming your Pet hierarchy. Obviously this hierarchy exists for the sake of polymorphism, so we can assume that your code takes objects by type Pet. Guess what? This makes calling getEdibleFoods incredibly unwieldy.

function doSomethingWithPetFoods(Pet $pet) {
    // How in the world do I call getEdibleFoods?
    // get_class($pet)::getEdibleFoods? Ew.
}

Don't forget that static methods also equal strong coupling (conveniently, you typically don't care if your coupled to a utility method). It's a bit hard to illustrate with a non-contrived example, so you'll have to bear with me.

function calculateSalesPrice($price, $tax) {
    return Math::round($price * (1 + $tax), 2); // Round to 2 digits
}

What could be wrong with this? It's just a simple round. The problem is that the code is now coupled to a very specific round. What if the United States demands that sales prices be rounded up to the nearest penny, and Argentina dictates that sales prices be rounded down to the nearest penny.

While this is a rather contrived example and its relevence to your code is rather limited, it's worth remembering.


Another problem is that you're (probably) not asking "what do cats eat?" when you call it. You're asking "what does this cat eat." It's a subtle difference, but an important one. What if two dogs eat different things? Or what if a certain cat is allergic to fish? This is why you have to be incredibly sure that a static method has absolutely not relation to state.

share|improve this answer
    
Boy was that a deep one! In the beginning I actually made it as an instance method, but then I figured, for my application, that all pets of the same kind will like the same food. So for that purpose I suppose this is exactly the way to go, do you disagree? Also in the example method doSomethingWithPetFoods(Pet $pet) getEdibleFoods can be accessed as $pet::getEdibleFoods(), which seems pretty natural and returns accurate information. Thank you for your detailed answer, even though I am, sadly, not fluent enough to rationalize a significant portion of it! –  php_nub_qq Aug 24 at 21:40
    
@php_nub_qq I could have sworn PHP gets grumpy if you call a static method on an instance, but you seem to be right. That still feels a bit gross though as calling a static method on an object rather than a class implies that it belongs on the object, not the class. –  Corbin Aug 24 at 21:47
    
Makes sense, but I suppose I would be better off to have this little ambiguity, rather than to have the same information stored a number of times, for example if I utilize memchache every single pet object will have foods information stored in it, which is generally the same for each type of pet. Such a waste just makes no sense for me and I opt to avoid that. Cheers! –  php_nub_qq Aug 24 at 21:56
1  
@php_nub_qq That certainly makes sense if it really is an attribute of each pet species. It's just a dangerous road to go down if you take that path then realize that the foods are actually per animal, not per species. Either way though, as long as you're aware of the tradeoffs, a case can be made in either direction :) –  Corbin Aug 24 at 22:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.