-1
\$\begingroup\$

I need to create a small app that can add options, save them into a database and retrieve info, which are painted in Panel and an Editor, and I've decided to do it manually, and trying to figure out if it's ok to follow the route above.

Admin - will contain functions for the options and other global stuff like, user roles....

Panel - will paint the options....

Editor - will paint an editor and use the options for some actions.

All of them need to be single instances. I need to access these options globally.

class Admin {
    private static $instance;

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

        return static::$instance;
    }

    private function __clone() {}

    private function __wakeup() {}


    private $options;
    public $Panel;
    public $Editor;

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

    protected function __construct() {}

    public function init() {
        $this->Panel = Panel::get_instance();
        $this->Panel->setOptions( $this->filterOptionsByKey('show_in_panel') );

        $this->Editor = Editor::get_instance();
        $this->Editor->setOptions( $this->filterOptionsByKey( 'show_in_editor' ) ); 
    }

    public function filterOptionsByKey( $key ) {
        $output = array();
        foreach($this->options as $option) {
            if (isset($option[$key]) && $option[$key] === true) {
                $output[] = $option;
            }
        }

        return $output;
    }

}


class Panel {
    private static $instance;

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

        return static::$instance;
    }

    private function __clone() {
    }

    private function __wakeup() {}


    private $options;

    public function setOptions( $options ) {
        $this->options = $options;
    }


    protected function __construct() {}

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


class Editor {
    private static $instance;

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

        return static::$instance;
    }

    private function __clone() {}

    private function __wakeup() {}


    private $options;

    public function setOptions( $options ) {
        $this->options = $options;
    }

    protected function __construct() {}

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


$Admin = Admin::get_instance();


$Admin->add_option( array(
    'name'           => 'a',
    'value'          => '1',
    'show_in_panel'  => false,
    'show_in_editor' => true
) );

$Admin->add_option( array(
    'name'           => 'b',
    'value'          => '2',
    'show_in_panel'  => true,
    'show_in_editor' => true
) );

$Admin->init();


$Admin->Panel->render();
$Admin->Editor->render();
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to code review. Unfortunately the question is off topic because it isn't working code. Please see codereview.stackexchange.com/help/how-to-ask. \$\endgroup\$ – pacmaninbw Jun 21 '17 at 11:38
  • 1
    \$\begingroup\$ @JohnD I think what he's trying to say is that it would be more acceptable to use non-example code, we don't really like things like // return filtered options. Even if you create a simplistic implementation that demonstrates the solution to a real-world problem we are O.K. with that, as long as it's reasonable production-ready code. (No things like Foo/Bar unless it's about an actual bar.) If you make those changes I think you'll find this question is much more positively received. :) \$\endgroup\$ – Der Kommissar Jun 21 '17 at 12:24
  • \$\begingroup\$ Thanks for the notice, now the code is an working example. \$\endgroup\$ – JohnD Jun 21 '17 at 12:49
  • \$\begingroup\$ I didn't run this but I can't image it works considering static::$instance should be self::$instance. \$\endgroup\$ – I wrestled a bear once. Jun 21 '17 at 17:57
  • \$\begingroup\$ @Iwrestledabearonce. static::$instance is totally fine usage here if there are classes extending from this class that are leveraging late static binding (php.net/manual/en/language.oop5.late-static-bindings.php) Of course, no such use case is shown here, so I am not so sure why it is being used. \$\endgroup\$ – Mike Brant Jun 21 '17 at 21:34
1
\$\begingroup\$

I think you are abusing the singleton pattern. Have you considering taking the approach of dependency injection?

By doing this you can invert your execution logic and clean up the weird filterOPtionsByKey() behavior.

IMagine a call pattern like:

$panel = new Panel($panelOptions);
$editor = new Editor($editorOptions);
$admin = new Admin($panel, $editor);

$admin->render();

Much cleaner. Much less work on the part of the caller to understand that it needs to be initialized, or to hard-code in options (options could easily be derived from configuration).


Why the use of late static bindings (i.e. static keyword)? You don't seem to have any inheritance in play that would warrant this.


Stylistically, I would like to see you be consistent in how you order your properties and methods within your classes.


Why have un-implemented magic methods in your classes? If you don't wan't your class to be clonable or deserializable, then consider throwing exceptions in these cases. Having unimplemented magic methods will not prevent a caller from doing these actions as these methods are called after the clone/deserialization already happens on the object. You probably also need to throw exception on __sleep() to prevent serialization if this is your intent.


If you want multiple classes to be non-clonable or non-serializable consider putting magic methods that throw exceptions as noted above into traits and simply use these traits from all classes needing these behaviors instead of re-implementing these methods across every class.

The same could actually be done for singleton behavior as well, though, I would encourage you to break out of this way of thinking about singleton.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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