0
\$\begingroup\$

I need to create an administration board that allows devs to create or modify options and display them in a panel. The Admin needs to be the same on each call made to it. I've chosen to make all dependencies as singletons wrapped under Admin. This code only covers the approach, disconsider the lack of details.

class Admin {
    private static $instance;

    public static function getInstance() {
        if ( null === static::$instance ) {
            static::$instance = new static();
        }

        return static::$instance;
    }

    private function __clone() {

    }

    private function __wakeup() {

    }

    public $Options;
    public $Panel;

    protected function __construct() {
        $this->Options = Options::getInstance();
        $this->Panel = Panel::getInstance();
        $this->run();
    }

    protected function run() {
        $this->Panel->run();
    }
}

class Options {
    private static $instance;

    public static function getInstance() {
        if ( null === static::$instance ) {
            static::$instance = new static();
        }

        return static::$instance;
    }

    private function __clone() {

    }

    private function __wakeup() {

    }

    protected $options = array();

    public function getOptions() {
        return $this->options;
    }

    public function addOption($option) {
        $this->options[] = $option;
    }
}

class Panel {
    private static $instance;

    public static function getInstance() {
        if ( null === static::$instance ) {
            static::$instance = new static();
        }

        return static::$instance;
    }

    private function __clone() {

    }

    private function __wakeup() {

    }

    protected $title = 'Panel';

    public function run() {
        $this->render();
    }

    public function render() {
        $Options = Options::getInstance();
        $options = $Options->get_options();
        foreach($options as $option) {
            echo $option['name'];
        }
    }
}



$Admin = Admin::getInstance();
$Admin->Options->addOption( array( 'name' => 'Test' ) );
\$\endgroup\$

closed as off-topic by Vogel612, Mast, t3chb0t, mdfst13, Mathias Ettinger Feb 22 '17 at 9:35

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Welcome to Code Review! I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. The example code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Unlike Stack Overflow, Code Review needs to look at concrete code in a real context. Please see Why is hypothetical example code off-topic for CR? \$\endgroup\$ – Vogel612 Feb 21 '17 at 15:00
1
\$\begingroup\$

First, from a styling standpoint, your classes are structured strangely. Typically, one would have all class properties defined as first thing in class, then the constructor as first method, then the other methods (oftentimes with public methods first with protected/private methods following). Don't mix your properties throughout your class.


Be consistent with style around variable naming. You probably should not be using first letter caps for any properties or variables in PHP (which is more or less industry standard). See the PHP Standards Recommendations for good suggestions on standards to follow (particularly PSR-2 around coding style).


I have several concerns regarding class design:

  • I don't really understand need for singleton here. I would actually suggest you consider dependency injection as a preferable way to inject dependencies into classes that need them.

  • It particularly doesn't seem to make sense to have late static binding in place like you do here if there is no intent to use inheritance. Now if you had a Singleton base class from which you inherited singleton behavior for each of these classes, then use of late static binding might be appropriate.

  • Additionally, you make the options and panel properties on the Admin class mutable, so you are potentially allowing the caller to put the Admin class in a bad state.

  • I don't really even know why options are being held on Admin class at all. Your dependency chain seems to be Options -> Panel -> Admin, so why put an Options dependency on Admin at all?

  • Why introduce run() alias to render() in Panel? This method adds nothing new.

  • I don't understand why you would call run() from Admin constructor when, in your usage example, you are adding options after the class is instantiated. Would these options never be rendered?

  • I am struggling to see the need for so much static/concrete duality across all these classes. To me this could be greatly simplified using dependency injection with concrete implementations like shown below:

Example:

class Admin {
    // perhaps this should not really be public?
    // Do you really want this to be mutable outside the constructor
    public $panel;

    public function __construct(Panel $panel) {
        $this->panel = $panel;
    }

    protected function run() {
        $this->panel->render();
    }
}

class Options {
    protected $options = array();

    public function __construct() {}

    public function getOptions() {
        return $this->options;
    }

    public function addOption($option) {
        $this->options[] = $option;
    }
}

class Panel {
    protected $title = 'Panel';
    protected $optionContainer;

    public function __construct(Options $options) {
        $this->optionContainer = $options;
    }

    public function render() {
        $options = $this->optionContainer->get_options();
        foreach($options as $option) {
            echo $option['name'];
        }
    }
}

// usage
$options = new Options();
$options->addOption( array( 'name' => 'Test' ) );
$admin = new Admin(new Panel($options));
// maybe add more options
$admin->panel->options->addOptions( array( 'anotherName' => 'Another Test' ) 
$admin->run();
\$\endgroup\$

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