1
\$\begingroup\$

I have the following class:

Class Core {

public $security;
public $db;
public $extra;

public function __construct() {
    spl_autoload_register(array($this, 'loader'));
}

public function loader($name) {
    include $name.'.php';
}

public function __get($key) {
    return $this->extra[$key];
}

public function init( $extra ) {
    #error_reporting(0);
    $this->security = new Security();
    #$this->db = new Database();
    $extra = explode(",", $extra);
    if(is_array($extra)) {
        foreach($extra as $key) {
            $key = trim($key);
            $this->extra[$key] = new $key;
        }
    }
}
}

Index.php include 'core/Core.php';

$start = microtime(true);

$core = new Core();
$core->init('User, Control');

$core->Control->test2();

echo "<br />";

$core->User->test();

The purpose of this was to make it so I could load/instantiate classes only when I need them, but use the Core class on all pages. I will of course check if the file exists or not, but I wanted some feedback on whether or not this was viable. Also, is there an easier way to do what I'm trying achieve?

\$\endgroup\$
3
  • \$\begingroup\$ I think you're just looking for auto loading? php.net/manual/en/language.oop5.autoload.php \$\endgroup\$ – Corbin Jul 18 '12 at 6:05
  • \$\begingroup\$ After reading your question again, I see that you are using autoloading in a round about way. I'm not sure I understand the purpose of a registry that acts as an implicit singleton. Can you elaborate a bit more on what drove you towards this? Using autoloading already makes it so that classes are only loaded and instantiated when you need them. \$\endgroup\$ – Corbin Jul 18 '12 at 9:13
  • \$\begingroup\$ Basically, the developer on a project before me wrote an entire CMS from scratch. The thread viewing PHP script has like 8 includes and 5 class instantiations when I know it could be cut down to way less. The idea was to only have to instantiate one class and then being able to do things like $core->User->getUserId(), or, $core->Security->xss_clean($str). It was just to avoid having to do $system = new System(); , $forum = new Forum(); etc.. \$\endgroup\$ – SHH Jul 18 '12 at 10:31
2
\$\begingroup\$

There's a lot of design oddities in this, and I don't quite see the value of it. The brevity it allows now will undoubtedly cause issues once you use this in a complex application.

Why is:

$core = new Core();
$core->init('User, Control');

$core->Control->test2();

echo "<br />";

$core->User->test();

Better than:

spl_autoload_register(...);

$control = new Control();
$control->test2();
$user = new User();
$user->test();

Autoloading is typically setup in some form of bootstrap. It is usually done in the same place that DB connections are made, application-wide configuration is put into place, etc.

This mean that your main script content should never have to worry about autoloading since your bootstrapping will have already taken care of it.

(Note that a boostrap does not need to be a huge complex process. Sometimes it can be as simple as include 'boostrap.php';)

Anyway, your class has a few design problems and a a few implementation problems:

Design:

  • What if you want more than 1 User object in the core?
  • What if you need to pass a parameter to a constructor
  • init() can make the state of the object unknown.
    • if you pass a Core instance to some other place of code, how does that code know what properties are valid on the object
    • This type of magic almost always ends badly since it hides state information

Implementation:

  • __get should check if the key exists in $extra before blindly accessing/returning
  • Unnecessary string processing: Why explode a string? Just use an array. It's cleaner and less error prone.
  • If you do keep this class (which I advise that you don't), I would consider making the init() part of the constructor. I like to ensure that all of my instances are always in a stable, usable state. What happens if you use a Core instance before calling init?
  • Setting up autoloading, storing objects and creating objects are three discrete responsibilities. This means that the class is likely doing too much.
    • Once again, I recommend not using the class, but if you do:
    • Have the constructor take in an Autoloader instance and have that class handle loading
    • Have the class take in instances instead of names (this decouples creation from storing -- which is good since it means you have much more control of instantiation -- but it will kill the brevity of using your class)
\$\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.