Last week I worked with one of our junior engineers on a design exercise and I thought I'd share. We're working on a theme system based on plug-able widgets, and we have a Widget_Manifest class that generates an array of widget objects based on the contents of a directory on the file system. Here's what we started out with:

class Mashery_Widget_Manifest
{
    /**
     * The directory in which the Widget classes reside. This is a non-absolute
     * directory, relative to the project root.
     *
     * @var string
     */
    const BUILTIN_WIDGET_DIRECTORY = '/server/root/path/widgets/';

    /**
     * Fetch the set of available Widgets
     *
     * @return array
     */
    public function fetch()
    {
        $return = array();
        $iterator = new DirectoryIterator(self::BUILTIN_WIDGET_DIRECTORY);
        foreach ($iterator as $directory_element) {
            if (!$directory_element->isFile() || $directory_element->isDot() || substr($directory_element->getFilename(), -4) != '.php') {
                continue;
            }
            $return[] = $this->_fetchObjectFromFileClass($directory_element->getPathname());
        }

        return $return;
    }

    /**
     * Given a file path, fetch an object of the class that is in the file
     *
     * @param string $file_path path to the file in question
     *
     * @return object of the class defined in the file
     */
    protected function _fetchObjectFromFileClass($file_path)
    {
        $root_prefix = '/server/root/path/';
        if (strpos($file_path, $root_prefix) === false) {
            throw new Mashery_Exception(array(
                'test' => 'The given file path is not valid: ' . $file_path
            ));
        }
        $class_name = substr($file_path, 0, -4);
        $class_name = substr($class_name, strlen($root_prefix));
        $class_name = str_replace('/', '_', $class_name);
        $object = new $class_name;

        return $object;
    }
} 

For the most part this should do its job and be functionally correct. The code as it exists is alright, but we could improve it. There's pretty much always a way to make code cleaner and clearer, and more in line with SOLID principles of object-oriented design.

The focus here is going to be small changes that improve the readability of the code that don't break anything and are easy to test and roll back if something goes wrong.

One of the main things we'll be doing here is breaking these two methods down into smaller methods that more clearly encapsulate what we're trying to do. For example, in the foreach loop inside of the fetch() method, we have a number of checks chained together that aren't intuitively obvious at first glance, so we can use an Extract Method refactoring and pull out the test into its own method:

if (!$directory_element->isFile() || $directory_element->isDot() || substr($directory_element->getFilename(), -4) != '.php')
{
    continue;
}

into

if ($this->_isNotPhpFile($directory_element)) {
    continue;
}

...

protected function _isNotPhpFile($fileSystemElement)
{
    return !$fileSystemElement->isFile() || $fileSystemElement->isDot() || substr($fileSystemElement->getFilename(), -4) != '.php';
}

Now the original code in fetch is more clear as to the intent of the code, which makes it easier to spot bugs in the future, and we've also reduced that method's complexity. Following this same pattern, we can also make the intent of the _fetchObjectFromFileClass method more clear.

$class_name = substr($file_path, 0, -4);
$class_name = substr($class_name, strlen($root_prefix));
$class_name = str_replace('/', '_', $class_name);

becomes

$class_name = $this->_stripFileExtension($file_path);
$class_name = $this->_stripRootPrefix($class_name, $root_prefix);
$class_name = $this->_replaceDirectorySeparatorsWithUnderscores($class_name);

...

protected function _stripFileExtension($filePath)
{
    return substr($file_path, 0, -4);
}

protected function _stripRootPrefix($filePath, $root_prefix)
{
    return substr($class_name, strlen($root_prefix));
}

protected function _replaceDirectorySeparatorsWithUnderscores($filePath)
{
    return str_replace('/', '_', $class_name);
}

This is already clearer than before, but we can still do better. For one, the comment on the BUILTIN_WIDGET_DIRECTORY constant says that it is a relative path, when it's actually an absolute path. It used to be relative, but the code had changed without the comment keeping pace, so the docblock needs to be updated. Furthermore, this path is not used outside of this class, and child classes may want to change it in the future, so it's better to make this a protected property instead of a class constant.

/**
 * Relative path to the directory in which the Widget classes reside.
 *
 * @var string
 */
protected $_widgetDir = 'Widget/';

Making this change would break the DirectoryIterator call, so we need to append the widgetDir to the root path, but the root path is only available as a variable inside of _fetchObjectFromFileClass, so we promote that to a protected property as well.

/**
 * Absolute path to the source code. Inputs must begin with this path or
 * they will be rejected
 *
 * @var string
 */
protected $_rootPath = '/server/root/path/';

With that done, we can update the directory iterator, _fetchObjectFromClass, and _stripRootPrefix:

$iterator = new DirectoryIterator($this->_rootPath . $this->_widgetDir);

...

protected function _fetchObjectFromFileClass($file_path)
{
    if (strpos($file_path, $this->_rootPath) === false) {
        throw new Mashery_Exception(array(
            'test' => 'The give file path is not valid: '.$file_path));
        }
        $class_name = $this->_stripFileExtension($file_path);
        $class_name = $this->_stripRootPrefix($class_name);
        $class_name = $this->_replaceDirectorySeparatorsWithUnderscores($class_name);

...

protected function _stripRootPath($filePath)
{
    return substr($filePath, strlen($this->_rootPath));
}

Here's the final, updated class, with just a bit more tidying applied.

class Mashery_Widget_Manifest
{
    /**
     * Absolute path to the source code. Inputs must begin with this path or
     * they will be rejected
     *
     * @var string
     */
    protected $_rootPath = '/server/root/path/';
    /**
     * Relative path to the directory in which the Widget classes reside.
     *
     * @var string
     */
    protected $_widgetDir = 'Widget/';
    /**
     * Fetch the set of available Widgets
     *
     * @return array
     */
    public function fetch()
    {
        $return = array();
        $iterator = new DirectoryIterator($this->_rootPath . $this->_widgetDir);
        foreach ($iterator as $directoryElement) {
            if (!$this->isPhpFile($directoryElement)) {
                continue;
            }
            $className = $this->convertFilePathToClassName($directoryElement->getPathname());
            $return[] = new $className;
        }

        return $return;
    }

    /**
     * Tests to see whether the passed file system object is a PHP file, by name
     *
     * @param DirectoryIterator $fileSystemItem File to test
     *
     * @return boolean
     */
    public function isPhpFile(DirectoryIterator $fileSystemItem)
    {
        if (!$fileSystemItem->isFile()) {
            return false;
        }
        if ($fileSystemItem->isDot()) {
            return false;
        }
        if (substr($fileSystemItem->getFilename(), -4) !== '.php') {
            return false;
        }

        return true;
    }

    /**
     * Converts a file path into its corresponding class name
     *
     * @param string $filePath File path to convert
     *
     * @return string
     *
     * @throws Mashery_Exception if the given path is not part of the defined root path
     */
    public function convertFilePathToClassName($filePath)
    {
        if (strpos($filePath, $this->_rootPath) === false) {
            throw new Mashery_Exception(array(
                'text' => 'The given file path is not valid: ' . $filePath
            ));
        }

        $filePath = $this->_stripFileExtension($filePath);
        $classPath = $this->_stripRootPath($filePath);
        $className = $this->_replaceDirectorySeparatorsWithUnderscores($classPath);

        return $className;
    }

    /**
     * Removes the last 4 characters of the path. Should be ".php"
     *
     * @param string $filePath Path to modify
     *
     * @return string
     */
    protected function _stripFileExtension($filePath)
    {
        return substr($filePath, 0, -4);
    }

    /**
     * Removes $this->_rootPath from the front of the file path
     *
     * @param string $filePath Path to modify
     *
     * @return string
     */
    protected function _stripRootPath($filePath)
    {
        return substr($filePath, strlen($this->_rootPath));
    }

    /**
     * Does what it says on the tin
     *
     * @param string $classPath Path to modify
     *
     * @return string
     */
    protected function _replaceDirectorySeparatorsWithUnderscores($classPath)
    {
        return str_replace(DIRECTORY_SEPARATOR, '_', $classPath);
    }
}

In later iterations, we split out the "file path to class name" functionality into a new class that collaborates with this one in order to not violate the Single Responsibility Principle (the "S" in SOLID). Even with a few small changes that don't break anything and don't take much time, the class becomes much easier to understand and maintain.