2
\$\begingroup\$

I am practicing the SOLID principles and my first step was taking a dive into dependency injection(DI), so I created a class, a container for all DIs. It works, but I'm just not entirely sure if this is how I should go about doing it, although to me it seems fine.

interface ContainerInterface
{
    public function set($name, callable $value);
    public function get($name);
}

class DI implements ContainerInterface
{

    protected $services;


    public function set($name, callable $value)
    {
        $this->services[$name] = $value();
    }

    public function get($name)
    {
        return $this->services[$name];
    }

}

That covers the DI class, now I'll set an example of how I could use it:

$di = new DI();

$di->set('database', function() {
    return new Database();
});

$di->set('pagination', function() use ($di) {
    return new Pagination(DatabaseInterface $di->get('database'));
});

$application = new Application(ContainerInterface $di);

In the last line, I just injected the container and now I can access all the DIs.

\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

A few thoughts:

  • Why not write anonymous functions in form of function($container) { ... } to allow your container to inject itself into function scope vs. having to use function () use ($di) { ... }? This would allow wider compatibility to other forms of callables besides anonymous functions (for example, objects implementing __invoke() and other named functions/closures).
  • Alternately, if you truly want to narrow the type of callable functions that can be passed to set(), you might consider type-hinting against Closure class, which is what PHP in fact uses to represent a "function" created using closure syntax.
  • Why are you invoking the callable on set() and not on get()? This means you are pre-instantiating every dependency even if it is not needed for a particular code path. This kind of limits your framework to only allowing singleton-like behavior. You could not pass a factory provider in one of your methods. Perhaps it would be useful to have flag on set() if you want to implement singleton-like behavior, either by calling immediately or by storing information in separate arrays for "services" vs. single objects.
  • Consider explicitly instantiating your services property to empty array protected $services = [] such that IDE's (and readers) can infer type (that is if you are not going to use Doc blocks).
  • You might consider better enforcing/validating the $name parameter. Right now, I could pass it pretty much anything, making your container fragile to outside input. If using PHP 7+, you should type-hint for string.
  • Is DI really a good class name? DIContainer perhaps?
  • Should you throw an exception if get() is called and key doesn't exist? I would guess that this would not be an expected condition in a well-written application and failing loudly in such a case will hopefully make it obvious to a developer that they have a problem in their code.
  • Is it OK that set() can overwrite existing item in the container? You might consider preventing this in favor of needing an explicit unset() method to remove an item from the container. This might suggest the need for a has() method.

For example:

public function has($name) {
    return array_key_exists($name, $this->services);
}

public function set($name, callable $value) {
    if($this->has($name)) {
        throw new Exception('...');
    }
    ...
}

public function get($name) {
    if(!$this->has($name)) {
        throw new Exception('...');
    }
    ...
}

public function unset($name) {
    unset($this->services[$name]);
}
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your insight on how I should actually setup a DIContainer. Your input has greatly widened my sight to how a DIC works. You pointed out a couple things I didn't even think of!!! \$\endgroup\$ Commented Apr 25, 2017 at 10:51

You must log in to answer this question.