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.

A few days ago I've faced an annoying problem. Let's imagine we have 3 classes: Base, System, Handler.

The classes Systen and Handler both inherit from the class Base. I want to access those classes this way: System::DoStuff(); I also need the Handler be available in the System, but do not want to instantiate it there, use any kind of reference injection.

I could simply design the System and the Handler class to be static. The problem would be: They need to inherit form the Base class, which should be static too, because if it's not, it won't work with the System and Hanlder because they ARE static. If is set Base to static, it won't work either, because once a child changes some parameter in it's parent, the parent would change for ALL children.

After some testing, I've came up with this solution:

class Wire
{
    //List of classes I'll have to load
    private static $load = array("Foo", "Hello");

    //Template of the Bootstrap class
    private static $template = NULL;

    //Wire everything up
    public function Up()
    {
            //Get the bootstrap template
        self::GetTemplate();

            //Get each class
        foreach(self::$load as $class)
        {
                    //Load it...
            require_once "./class/" . $class . ".php";

                    //Build the static shell
            $bootstrap = str_replace("%CLASS%", $class, self::$template);
            eval($bootstrap);
        }
    }

    //Get the tempalte of the bootstrap class
    private function GetTemplate()
    {
        if(self::$template === NULL)
        {
                    //I've stored it in a separate file
            self::$template = file_get_contents("./class/Bootstrap.php");
        }
    }
}


Wire::Up();

I'me using the Wire class to load the classes I need. And use this Bootstrap file to create a static shell for each instatiated class.

//This is the Bootstrap class
//It offers a static shell for other instatiated classes
class %CLASS%
{
    public static $instance = NULL;
    public function __callStatic($method, $arguments)
    {
        if(self::$instance === NULL)
        {
            $class = get_called_class() . "Controller";
            self::$instance = new $class();
        }
        call_user_func_array(array(self::$instance, $method), $arguments);
    }
}

That's the Base class, for testing purpouses only

class Base
{
    public $value = 1;
}

Here are the classes Foo and Hello that I've used above

class FooController extends Base
{
    public function Bar($args)
    {
        print $args . " " . $this->value . "<br>";
    }
}

class HelloController extends Base
{
    public function World($args)
    {
        $this->value = 10;
        print $args . " " . $this->value . "<br>";
    }
}

Now, if you call

Hello::World("Hello World") //Hello World 10
Foo::Bar("Foo Bar") //Foo Bar 1 
//If Foo and Base would be static, whitout the Bootstrap shell, you would see "Foo Bar 10"

What do you think about that solution? Am I hunting a mouse with a shotgun? Or maybe there are better solutions, for this scenario? I'd appreciate costructive critics!!

share|improve this question
2  
Static classes tend to quickly move you in the exact opposite direction as OOP. I have a feeling that there's a much better way to accomplish what you're doing, but after reading your post 3 times, I'm not quite sure I understand what it is that you are doing. Why do you need to wrap some objects in a static 'shell'? Is it for convenience? Abusing them as globals? Or what? It seems like dependency injection is the right approach here, even if it is less convenient. (Once again though, maybe I'm missing something -- not sure I quite get what the end goal is.) –  Corbin Jan 7 '13 at 21:27
    
Read my post below, please. –  maximkott Jan 7 '13 at 23:10
    
Rule of thumb: If it has mutable state it shouldn't be static –  CodesInChaos Sep 6 '13 at 9:24
add comment

2 Answers

I don't know if I'm just not understanding your situation properly or what, but I don't think converting three classes to static for one static method is really the right thing to do here. Why would you need to call System::DoStuff() statically? Why not just $system->DoStuff()? I would refactor the one method before I refactored the three classes. At most I would convert your base class to an abstract class to prevent its instantiation and I would define the value for the $value property in each individual class.

abstract class Base {
    public $value;
}

class FooController extends Base {
    public $value = 1;

    //etc...
}

class HelloController extends Base {
    public $value = 10;

    //etc...
}

You should use doccomments instead of those normal comments. They're more useful and can be read by your IDE.

/** List of classes I'll have to load */
private static $load = array( 'Foo', 'Hello' );

"Up" is not a very descriptive method name. What is it supposed to do? From your comments and your code it looks like its instantiating the class. So normally this would be in the constructor. I would suggest renaming this to something like instantiate(), or maybe getInstance(); And then I would make this a private method, or at the very least ensure that it hasn't already been instantiated so you don't end up doing it again.

eval() is evil. No seriously, it is. It is a horrible security risk. The only saving grace in this instance is that you have not written this in such a way that user input could be used. If you need to dynamically load a class, then you should just do so like this:

foreach( $load AS $class ) {
    require "./class/$class.php";
    $instance = new $class();
}

The *_once() versions of require and include should be avoided if at all possible. With a small number of files it isn't as noticeable, but PHP has to run a special check on each request to ensure that the file hasn't already been included, this means that it will run slower. Again, this is negligible in this case, but it could become an issue later.

I hope this helps, but I really don't know if I understood your issue well enough.

share|improve this answer
1  
+1, but a small nitpick: Auto loading tends to be much more flexible down the road than explicit require/include. (And it can have very similar performance with a simple autoloader.) The problem essentially boils down to that the $class may have already been loaded. Not likely in a simple case like this though. (And in all but a few edge cases, attempting to double load classes is a very bad sign.) –  Corbin Jan 7 '13 at 21:24
    
I wasn't sure whether I should mention autoloading or not, but you are correct, it would be. –  mseancole Jan 7 '13 at 22:05
    
Read my post below, please. –  maximkott Jan 7 '13 at 23:10
1  
@maximkott: You should remove your "answer" and edit your question with that information. I don't understand why you would not want to instantiate a class. That single limitation, and your static solution, is the root of all your problems. "I want some classes be accessible as static ones, but behave like regular classes." That's contradictory, and I cannot imagine a situation where it would be necessary. It really sounds like you want a shared instance of a normal class, (persistence through sessions, cache, etc...) or maybe helper functions. As Corbin said, static is very much anti-OOP. –  mseancole Jan 8 '13 at 15:08
add comment

Thank you very much for feedback. I'll try to explain you my situation a little bit better. I will have some classes that I want to be globaly available without the need to instantiate them, or use any dependency injection. So static is the way to go here. Well... Now imagine some of those static classes share some basic functionality and properties. For functionality I could use a simple Interface. But Interfaces do no offer properties, so.. a regular class is the way to go. Now imagine what happens if a static class inherits from a non static class: the properties will get lost(reset) on each method call of that static class. So the "interface" class may not be regular. Now try to set it as static. For example: 2 children inherit from this "interface" class. Assume one child changes an interface class property. Now this property has changed in the second class too (because the interface class is static). So I had to find a way to make a class globaly available and static callable, but at the same time be able to inherit methods and properties from other classes.

My goal: Call::aClass("like this"); But this Class should also be able to inherit methods and properties from other.

So I've came up with this solution:

We have a BaseClass (with some properties and methods).

And 2 other classes that both extend the BaseClass: FooClass and BarClass;

At this point I can't use them as static classes, because of the description in the big text block =)

Now I created a kind of a template for the ShellClass, that will be treated as a singleton, and will hold an instance of the regular class, I'll assign it to (in this case that would be FooClass and BarClass)

Now the problem was, the naming of this ShellClass. Because, once I'm gona use it with the FooClass and once with the BarClass. So I'll have to generate the name of this ShellClass int runtime depending on the name of the regular classes (FooClass, BarClass).

So the ShellClass template mutates in the runtime to Foo and serves as static shell for the FooClass. In context with the BarClass, the name of the ShellClass would be Bar.

That's why I'm using evals and reading the source code of the ShellClass via file_get_contents().

ANSWERS TO SOME DETAILS IN THE COMMENTS

require_once: I did not know that this slows the execution that much, and will try to avoid it from now on.

using Up() as method name: You could not have seen this, because it kinda cut off the source I've posted. The class the method Up() was located in, was named Wire. So I felt good about calling that whole process Wire::Up() and not Bootstrap() or what ever =)

I appreciate the time, guys, you took, to try to dig in this peace of code, and try to explain you my situation as well as possible. I've redone the code I posted above for now, but I do not see any sense posting it here. The basic idea stays the same.

I want some classes be accessible as static ones, but behave like regular classes

You may recognize this kind of approach in the Laravel framework, where you can use the whole framework in that way: Input::Get("FooBar"), Session::Set("Foo", "Bar"), etc.

I do not know what design pattern the guys there are using. I tried to rebuild/reconstruct it my way, or they way it would probably be possible to do so.

I'd appreciate any critics, comments, thoughts, and other approaches.

share|improve this answer
add comment

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.