4
\$\begingroup\$

The following code works and does what I want, but I'd like to confirm I'm using this OOP concept correctly. I'm using the following class to get and set some configuration parameters for my program. It used $GLOBALS for this before, but I'm trying to use a singleton pattern for this instead.

Below is the Config class. Does anything jump out as aggressively stupid? Seeing as how I'm going to be including this in most files any way I figured it would make sense to throw the autoload function in there too- is this bad design?

<?php

  //This class will be included for every file
  //needing access to global settings and or
  //the class library

  spl_autoload_register(function($class){
      require_once 'classes/' . $class . '.php';
  });

  final class Config {

      private static $inst = null;

      //Array to hold global settings
      private static $config = array(
          'mysql' => array(
              'host' => '127.0.0.1',
              'username' => 'root',
              'password' => '123456',
              'db' => NULL
          ),
          'shell' => array(
              'exe' => 'powershell.exe',
              'args' => array(
                  '-NonInteractive',
                  '-NoProfile',
                  '-NoLogo',
                  '-Command'
              )
          )
      );

      public static function getInstance() {
              if (static::$inst === null) {
                  static::$inst = new Config();
              }
              return static::$inst;
      }

      public function get($path=NULL) {

          if($path) {
              //parse path to return config
              $path = explode('/', $path);

              foreach($path as $element) {
                  if(isset(static::$config[$element])) {
                      static::$config = static::$config[$element];
                  } else {
                      //If specified path not exist
                      static::$config = false;
                  }
              }
          }

          //return all configs if path NULL
          return static::$config;
      }

      public function set($path=NULL,$value=NULL) {

          if($path) {
              //parse path to return config
              $path = explode('/', $path);
              //Modify global settings
              $setting =& static::$config;

              foreach($path as $element) {
                      $setting =& $setting[$element];
              }

              $setting = $value;
          }
      }

          //Override to prevent duplicate instance
          private function __construct() {}
          private function __clone() {}
          private function __wakeup() {}
  }

I access it like so:

$aaa = Config::getInstance();
var_dump($aaa->get());
$aaa->set('mysql/host','zzzzzzzzzzzz');
var_dump($aaa->get());

I read a little bit about how singletons are less useful in php. I will research that further at some point, but for the time being I just want to make sure I'm doing this right.

\$\endgroup\$
  • 1
    \$\begingroup\$ The question you really need to ask is: Would a second Config cause the death of the app? Not just confusion... would the app fall apart if a second instance ever existed, even if the app never knew about it? If it wouldn't, then don't make the class a Singleton. If you want a global, then use a global; Singleton doesn't make that any less evil. :) \$\endgroup\$ – cHao Jan 16 '14 at 20:39
  • \$\begingroup\$ Well I will admit it was simpler when I used $GLOBALS. I should have come clean before, but I don't even know what this mostly hypothetical app will do. For learning purposes I wanted to pretend that I needed a single instance of the config so I could learn how to implement that. Assuming I do need a singleton because a second config would cause the death of the app, is the code acceptable? \$\endgroup\$ – user1028270 Jan 16 '14 at 21:05
  • \$\begingroup\$ Oh also, is it ok to throw the autoload function in this class? I was afraid it might be bad\sloppy. \$\endgroup\$ – user1028270 Jan 16 '14 at 21:08
  • \$\begingroup\$ It's not bad, but i imagine there's a better place for it... \$\endgroup\$ – cHao Jan 16 '14 at 23:39
5
\$\begingroup\$

There should be something about a Singleton class that requires that it allow and present a single, globally available, instance. Otherwise, the pattern is being misused.

The way your class is built, there's not really such a reason. You could have any number of instances, and they'd all work (though they'd all look like the same instance anyway).

As for recommendations to make things less weird...?

  • Use instance variables.

    Everything is essentially global. (Make no mistake; hiding it in a class doesn't do much at all to change that. static is just how a lot of OO languages spell "global". :P) Your "instance" is just an interface to get and set that global stuff. Every "instance" has the same ability, since it gets and sets the same variables -- and in fact, any update through one "instance" will be reflected in every other. So new Config and Config::getInstance() really differ only in whether another instance is created. The caller could say one or the other, and get the exact same results.

    If you insist on a singleton, moving the static variables into the instance would legitimize the existence of an instance at all, and provide a less dodgy reason to require exactly one authoritative instance. (Though it'd also prompt the question, "Why can't i create a separate configuration for testing?". Among others.)

  • Lose the public setter, or provide a way to disable it.

    There's next to no encapsulation or data hiding going on here; the only thing that even remotely counts is your path stuff. Any caller can change global settings at will. This kind of "action at a distance" can cause all kinds of weirdness. It is the very reason mutable global state is discouraged, and set makes it even easier to abuse.

    You should consider either getting rid of the setter, making it private (and thus disabling outside modification altogteher), or adding some way for your setup code to disable it -- effectively "freezing" the instance -- once the configuration is established.

  • Use self::, not static::.

    static (when used in place of a class name) uses PHP's "late static binding" to look up variables through the name of the called class rather than the one where the method lives. It's there to allow subclasses to override static members. But you've explicitly disallowed inheritance...so values will only ever be looked up in this class. static is thus wasteful, and a bit of a mixed message.

  • As for the autoloader, it's not exactly a mortal sin or anything to put that in the same file as the config class. Unless the classes are going to be moving around, though, their current location isn't really part of the config settings, is it?

    I'd suggest making the autoloader a part of the startup code, unless the class files are going to move around (thus making the current location a part of the configuration).

\$\endgroup\$
  • \$\begingroup\$ Thanks for all the great info. I thought that static replaced self and that the behavior of self was a flaw not a feature. Is it wrong to just always use static or are there performance considerations? If declare a class final shouldn't that clear up any confusion? It would just be nice to only have one keyword for this instead of juggling two. Thanks again. \$\endgroup\$ – user1028270 Jan 17 '14 at 14:29
  • 2
    \$\begingroup\$ It can be wrong to use static::. If you have class B that extends class A, and A::method uses static::$stuff, then B::method() will first look for B::$stuff, then A::$stuff. Sometimes you don't want that. In your current case, "wrong" is a bit of an overstatement. But the whole point of static:: is to cause LSB, so it is really better used only when you (1) can have subclasses, and (2) want them to be able to override a static attribute. LSB implies extensibility, which contradicts the one line (more than a screen away from most uses of it) that outlaws extension. \$\endgroup\$ – cHao Jan 17 '14 at 17:24
  • \$\begingroup\$ The difference between self vs static is important, and sometimes quite desirable -- particularly if you have some functionality you don't want overridden, or want to be certain of where you're setting a variable but don't want to hard-code the class name. \$\endgroup\$ – cHao Jan 17 '14 at 17:38
  • \$\begingroup\$ Thanks for the great explanation, that makes sense to me now. So self can be useful if I am extending a class, but also want to access a method in the parent instead of the local instance of that method. \$\endgroup\$ – user1028270 Jan 17 '14 at 17:46
  • \$\begingroup\$ Close. (It sounds like you get it, but the wording is a bit off.) self is useful whenever you want to specifically use your version of a method. It's used by the parent, not the child. (The child can of course use self too, but it will be referring to itself, not the parent.) If you want a child class to use a parent's version of a method, that's what parent is for. \$\endgroup\$ – cHao Jan 27 '14 at 18:29
1
\$\begingroup\$

I just want to add to cHao's explanation of static vs self. You use self to call a static function rather than do $this-> because you can only do $this if you have instantiated an object.

This is typically done when you want to call a function that belongs to a class but that function has static variables (like a hard coded constant/variable) that is used within the function.

\$\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.