1
\$\begingroup\$

Well Iam trying to learn OOP for a while now, My idea here is to create a simple CMS kind of thing.And I did something like this:

App Class:

namespace System;

class App {
    private $storage = [];
    private static $_instance;

    public static function getInstance()
    {

        if (is_null(self::$_instance)) {

            self::$_instance = new self();
        }
        return self::$_instance;
    }

    public function __get($key)
    {
        return $this->get($key);
    }

    public function get($key){

        if (!$this->has($key)){
            if ($this->isCore($key)){
                $this->share($key,$this->createNewObj($key));
            }else{
                die($key.' was not found!');
            }
        }
        return $this->storage[$key];

    }

    private function has($key)
    {
        return array_key_exists($key, $this->storage);
    }

    public function share($key,$value)
    {
        $this->storage[$key]= $value;

    }

    private function isCore($class)
    {
        $coreClasses = $this->coreClasses();
        return isset($coreClasses[$class]);
    }

    private function coreClasses()
    {
        return [

            'db'        => 'System\\Database',
            'user'      => 'System\\User'
        ];
    }

    private function createNewObj($key)
    {
        $coreClasses = $this->coreClasses();
        $opj = $coreClasses[$key];
        return new $opj($this);
    }

}

User Class: (Every class I write in this app will take an object from App using Dependency Injection).

namespace System;

class User
{
    private $app = null;

    public function __construct(App $app)
    {
        $this->app = $app;
    }


    public function getAllUsers()
    {
        return $this->app->db->select('* FROM users')->fetchAll();

    }



}

init.php

if (!session_id()){
    session_start();
}

define('ROOT',dirname(__DIR__));
define('DS', /);


require ROOT.DS.'System/Autoload.php';
\System\Loader::reg();

$app = \System\App::getInstance();

an now when I want to get something from the database I use this: in users.php

require 'init.php'; 
//get the object from storage if does exist!
$User = $app->user->getAllUsers();

The idea was to get the an object from User class on the fly. rather doing it like:

$User = new User($database);
$User->getAllUsers();

Am I doing this right, or is it a bad idea?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Your App class seem too broad in purpose. Right now it:

  • Acts as a sort of global key-value store.
  • Acts as a registry of "core" classes used in an application. Something typically just offloaded as an autoloading consideration in modern PHP applications.
  • Acts as a factory/provider for core class instances. Something that will become extremely unwieldy as your application grows in classes, each with their own sets of dependencies and logic around how they need to be set up.

It almost looks like what you are trying to do is build a dependency injection framework container, however the implementation is slightly off in that you are injecting the entire container into consuming classes rather than using the container to store instantiation logic for dependencies. That later apporach might look something like:

// set up and configure container
$container = new \System\App();
$container->register('database', function($cont) {
    return new \System\Database();
});
$container->register('user', function($cont) {
    return new \System\User($cont->get('database'));
});

// get user, which would also instantiate new database object if needed
$user = $container->get('user');

If this is truly what you are trying to do, then I would recommend you take a look at popular PHP dependency injection frameworks/containers like PHP-DI or Pimple either for direct use in your applications or for inspiration on how you can approach the problem.


From a global key-value store standpoint, it is hard to understand how this is actually used in your application from what is shown and unclear what value it really has.

What is stored here? If application configuration, should you really provide the level of mutability you do? Would you better be served to have an immutable configuration class that exposes these values?

If storing key-values that are going to be consumed in classes, why not move these items into those classes where you can better enforce class-specific validation rules for setting these keys? Do you really need the flexibility provided by this functionality and the fragility problems this flexibility can introduce?

I would think it trivial to split this portion of functionality away from App if you decide this store truly gives you some value that is not apparent from the code shown.


I don't agree with the approach of injecting this App object along with every single potential instantiated dependency it holds into every consuming class that might need one of the dependencies. It makes it more burdensome in consuming classes to validate that the dependency you need is actually passed.

For example, say you had a class that needs a PDO object. A good dependency injection model might look like this:

public function __construct(\PDO $pdo) {
    $this->pdo = $pdo;
}

You enforce the dependency through type-hinting, allowing you to not have to write validation code in your class. Your class gets it's dependency in a proper state.

VS. your current approach which might require validation such as:

public function __construct(\System\App $app) {
    if($app->db instanceof \System\Database === false) {
        throw new Exception('...');
    }
    // validate other dependencies here
    // set dependencies on object as needed
    $this->app = $app;
}

You also obfuscate dependencies to someone reading/working with your classes. With the approach I suggest, the exact dependencies of the class are explicitly stated in the constructor method signature (or in other method signatures if you have static methods or load dependencies after instantiation for some reason). If someone needs to add functionality to the class that requires a new dependency, you actually have to add that dependency to the class. This may seem like extra work, but, in fact, it is good coding behavior in that you have to be thoughtful and specific in how you consume dependencies. You avoid someone just introducing new dependencies in your classes by simply adding calls against not-previously-used dependencies stored in App.


So, if you resolve that injecting every dependency that could possibly be needed into every class is not the way you want to go, you can then address the problem with your provider/factory behavior.

If you wanted to only pass required dependencies to consuming classes, your createNewObj() method would quickly become overly complex (it might get this way even with your current dependency injection approach if you need more complex instantiation logic for given classes).

You could imagine some horrible switch construct like:

private function createNewObj($key)
{
    switch($key) {
        case 'user':
            return new \System\User($this->get('db'));
            break;
        case 'db':
            return new \System\Database();
            break;
        // and so on
    }
}

This obviously would be a bad approach. So how should you approach this? Each class should have it's own mechanism for instantiation, whether that be a static method on the class itself, or whether it be a wholly separate factory or provider class.

To address the use case in your usage example. That might look like this:

$users = UserProvider::getAllUsers($database);

Or under DI container approach:

$users = $container->get('users');

Some other thoughts:

  • Should App::share() be called set()? This seems like what is happening.
  • Your App class singleton behavior is improperly implemented. You need a protected/private constructor or perhaps constructor that throws exception to prevent caller from instantiating the class directly.
  • There is nothing in your App class that can switch between singleton-type behavior for some classes vs. concrete behavior for others. This would for example only allow a single User object to be created, which may or may not be desired behavior for your application (as it is unclear what User does).
  • Speaking of which, is User class properly named, or is this actually a provider class? It only has one method, which would seem to return a collection of users, not a singular user. Should this class even have concrete context?
  • Don't die() in the middle of your App class, and especially don't die() with message to standard out. The way this class is designed, you have no idea where it is being used in overall application context. The code up the call stack is likely more better positioned to handle the error and provide meaningful end user messaging. Consider using exceptions with this kind of functionality that is so core to your whole application framework.
  • You have unnecessary nesting in your App::get() method. Unnecessary conditionals and nesting can lead to more fragile code.

Consider an approach like:

public function get($key){
    if (!$this->has($key) && !$this->isCore($key)) {
        throw new OutOfRangeException('Some message');
    }
    if (!this->isCore($key)) {
        $this->share($key,$this->createNewObj($key));
    }
    return $this->storage[$key];
}

This example also creates an early exit from the function or code path, which is almost always a good thing. Fail fast and fail loudly.

  • Why are your core classes defined inside a method instead of in class configuration (i.e. class constant), or better yet, in application configuration. Look at those DI container framework examples I have shown, there is nowhere in there where they are hard coding the classes available to an application.
  • You are not really doing any validation against parameters passed to your public methods, which for a class as central in your application as this, might be problematic. What if invalid key values are being sent to share()?
  • $opj = $coreClasses[$key]; Is $opj a good variable name here? This is really a class name right?
  • You should consider getting out of the habit of using SELECT * in queries against the database. This approach oftentimes pulls across more data than necessary (for example if table has timestamps fields or other fields that are not meaningful in wider application context) and it also makes your code potentially fragile to table schema changes.
\$\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.