Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've been working on my own PHP framework for my projects. I just finished coding my DI container. There is always room for improvement. Any input be it good or bad on my code would be appreciated.

Also using exceptions instead of die() would be a better approach, wouldn't it?

EDIT: I made a few improvements. I replace all the die() class with InvalidArgumentException and RuntimeException.

Should I move the higher API functions (register(), addParameter()) to a separate class?

<?php

class Reference
{
    private $id;

    public function __construct($id) {
        $this->id = strtolower($id);
    }

    public function __toString() {
        return (string) $this->id;
    }
}

class ServiceContainer 
{
    private $services;
    private $lastRegistered;

    /**
     * Register a service with the DI container
     *
     * @param string $id Name of the service
     *
     * @param string $class Class to attach to service
     *
     * @return object $this
     */
    public function register($id, $class) {
        $this->addService($id, $class);
        return $this;
    }

    /**
     * Add a parameter to a service
     *
     * @param mixed $parameter Parameter to add
     *
     * @param string $id Name of the service
     *
     * @return object $this
     */
    public function addParameter($parameter, $id = null) {
        $id = isset($id) ? $id : $this->lastRegistered;
        $this->addService($id, null, array($parameter));
        return $this;
    }

    /**
     * Check if a service is registered
     *
     * @param string $id Name of the service
     *
     * @return bool
     */
    public function isRegistered($id) {
        $id = strtolower($id);
        return array_key_exists($id, $this->services);
    }

    /**
     * Add a service to the DI container
     *
     * @param string $id Name of the service
     *
     * @param string $class Class to attach to service
     *
     * @param array $parameters Array of parameters
     */
    public function addService($id, $class, array $parameters = array()) {
        $id = strtolower($id);

        // Initialize the parameter array
        if (! isset($this->services[$id]['parameters'])) $this->services[$id]['parameters'] = array();

        foreach ($parameters as $parameter) {
            $this->services[$id]['parameters'][] = $parameter;
        }

        if (isset($class)) $this->services[$id]['class'] = $class;

        $this->lastRegistered = $id;
    }

    /**
     * Get a service
     *
     * @param string $id Name of the service
     *
     * @return object Return the service
     */
    public function getService($id) {
        $id = strtolower($id);

        if ($this->isRegistered($id)) {
            // Build only when needed
            return (is_object($this->services[$id])) ? $this->services[$id] : $this->buildService($id);
        } else {
            throw new \InvalidArgumentException(sprintf('Service "%s" does not exist.', $id));
        }
    }

    /**
     * Build the service
     *
     * This a low level function with all the DI container magic
     *
     * @param string $id Name of the service
     *
     * @return object Return the service
     */
    private function buildService($id) {
        if (! $this->isRegistered($id)) {
            throw new \InvalidArgumentException(sprintf('Service "%s" does not exist.', $id));
        }

        if (! isset($this->services[$id]['class'])) {
            throw new \RuntimeException(sprintf('Service "%s" has no class to construct.', $id));
        }

        $reflection = new \ReflectionClass($this->services[$id]['class']);

        // If the paramters is a reference replace the parameter with the referenced service object
        foreach ($this->services[$id]['parameters'] as &$parameter) {
            if ($parameter instanceof Reference) {
                $parameter = (string) $parameter;
                $parameter = $this->buildService($parameter);
            }
        }

        // Replace the whole array with the service object, this is destructive for the parameters
        //  but they are no longer needed
        // If the constructor is null pass no arguments to the class
        $this->services[$id] = (null === $reflection->getConstructor()) ? $reflection->newInstance() : $reflection->newInstanceArgs($this->services[$id]['parameters']);

        return $this->services[$id];
    }
}
share|improve this question
add comment

1 Answer

Throwing Exceptions is better than die(), yes. However, you should consider throwing your own defined Exceptions instead of any built in php ones. For example, you could throw and InvalidService Exception from the getService method.

Using namespaces will help you avoid naming collisions. A class named "Reference" is bound to collide (perhaps you are already and left them out here for simplicity).

You could combine register and add Parameter such that register accepts a second argument: an array of parameters:

$svc->register('foo', array(5, 'bar'));
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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