4
\$\begingroup\$

I'm writing an object oriented package, and one problem I am trying to solve is a class that will get all PHP files in a particular directory and instantiate the classes but not abstracts or interface types.

From an object oriented perspective, is this class SOLID?

<?php namespace Massculator;

class Collector
{
    public function collect($path, $gatewayNamespace)
    {
        $classes = new \RecursiveDirectoryIterator(dirname(__FILE__) . $path);

        foreach(new \RecursiveIteratorIterator($classes) as $file) {
            $class = basename($file);
            $class = str_replace('.php', '', $class);
            if(class_exists($gatewayNamespace . $class)) {
                $constructedClass = $gatewayNamespace . $class;
                if((new \ReflectionClass($constructedClass))->isAbstract())
                    $return[] = new $constructedClass;
            }
        }

        return $return;
    }
}

The class has one method and that method has a couple of tasks:

  1. get files
  2. parse only PHP files
  3. instantiate non abstracts

It seems like this breaks the single responsibility principle, but creating another class simply to do a str_replace seems pointless. What do you think?

\$\endgroup\$
3
\$\begingroup\$

README:
First off, I'll just focus on making this one class a tad more generic, and expandable and maintainable and... well, generally more OO. Then, though I'm not going to go into too much detail on the matter, I'll just list a few quick tips of how you could go about separating concerns even more and create an entire File namespace.
Code-review can be harsh, but I do not intend to be hurtful or patronizing. My intentions are simply to convey, to the best of my abilities my views on what might be a preferable approach. I base this on experience, as well as personal preference and. While I try to be as objective as possible, I'm only human, so it stands to reason that the code I suggest might not be to your liking. But the code listed here is untested, written off the top of my head and only serves to illustrate my point.

This class/method definitely could do with a couple of additional methods. For example: I might want to set a path, and then -at various points in time- want to get reflection classes for a given "gateway namespace".
Your code, as it now stands would imply calling the collect method with the same $path argument, which will create an all-new RecursiveDirectoryIterator instance, only to get those files I'm after.

If, however, your class would look like this:

class Collector
{
    protected $iterator = null;
    public function __construct($path = null)
    {
        if ($path) $this->setPath($path);
    }
    public function setPath($path)
    {
        $this->iterator = new \RecursiveDirectoryIterator(
            dirname(__FILE__) . $path
        );
        return $this;
    }
    public function collect($namespace, $path = null)
    {
        $return = array();
        if ($path !== null) $this->setPath($path);
        foreach(new \RecursiveIteratorIterator($this->iterator) as $file)
        {
            $class = str_replace('.php','', basename($file));
        }
    }
}

You allow users to set the base-path when creating the instance, and then use that instance to get whatever reflection classes they will be needing at any given moment.
Moreover, you can expand on this "theme" even more, by allowing them to set a property that you can use instead of using dirname(__FILE__), which -if we're honest- isn't really improving the reusability of your current code.

I also struggle to see the point in a method that only allows reflection on abstract classes only:

if((new \ReflectionClass($constructedClass))->isAbstract())

Why not create reflection classes for all matched files, and assign them to a property, so you can then use a getter where the user can specify which classes he/she needs?

class Collector
{
    const COLLECT_ABSTRACT = 0;
    const COLLECT_REGULAR = 1;
    const COLLECT_BOTH = 2;
    //properties:
    $iterator = null;
    $collection = array();
    public function getCollection($gatewayNS, $which = self::COLLECT_ABSTRACT)
    {
        if (!isset($this->collection[$gatewayNS]))
            $this->getNS($gatewayNS);
        if (!isset($this->collection[$gatewayNS][$which]))
            throw new \InvalidArgumentException('invalid offset (0,1,2 are allowed)');
        return $this->collection[$gatewayNS][$which];
    }

    private function getNS($ns)
    {
        $this->collection[$ns] = array(
            self::COLLECT_ABSTRACT => array(),
            self::COLLECT_REGULAR  => array(),
            self::COLLECT_BOTH     => array()
        );
        foreach(new \RecursiveIteratorIterator($this->iterator) as $file)
        {
            $class = $ns.str_replace('.php','', basename($file));
            if (class_exists($class))
            {
                $class = new \ReflectionClass($class);
                $this->collections[$ns][self::COLLECT_BOTH] = $class;
                if ($class->isAbstract())
                    $this->collection[$ns][self::COLLECT_ABSTRACT] = $class;
                else
                    $this->collection[$ns][self::COLLECT_REGULAR] = $class;
            }
        }
        return $this->collections[$ns];
    }
}

That should help you on your way to turn this one-method-function-in-OOP-drag thing into a bona-fide class, that you can use in future projects, as well as your current one.
This is, of course, just a start. This class still needs some more work, and actually, could do with some helper classes, too. As I said, I'm not going to deal with this in great detail, but just as a primer/list of suggestions. The first one, though, is not a suggestion, but an appeal:

Code should document itself
Looking at your code, I'm not at all aware of all the other classes you're using, lest I browse through the actual code. Namespaced class files should start with a series of use statements, that tell me what other classes they depend upon. That way, I can get a rough Idea of what classes I can expect to see in the return values, and which objects are going to be valid as arguments, too.
That, and of course: nobody likes to see all those backslashes throughout the code.

Clear Interface is not optional
A clear-cut, consistent interface isn't just a nice-to-have thing. It's vital. If you want to churn out code you can pass on to others, or re-use without having to refactor half of it, you need to stop and think about what data is likely to be passed to your classes
You're iterating directories, in search of files. So yes, strings are likely candidates for arguments. As are SplFileInfo instances, don't you think?

Now, to add support for these instances, you have 2 choices: Either check the type of the argument that is passed ($arg instanceof \SplFileInfo) or create a wrapper.
IMO, the wrapper is the way to go, as it normalizes your API, allows for type-hints and centralizes all code concerning how you deal with file-paths in a couple of classes. If ever the SplFileInfo class changes, you can update those classes and be done with it all, certain that all other code will work fine.

Read up on what the SplFileInfo class can do for you. If you're interested, write a constructor that accepts either an instance of this class or a string, and -if you receive a string- use that to construct an SplFileObject asap.

Collector is a container
If you do set about writing a File-managing lib, this Collector shouldn't contain anything but processed files. It's a data-container, not a data processor.
You should consider writing some file-parsers (or "Adapters"), that each deal with a specific type of file. They can implement various interfaces, like the ParserInterface for the (and I'm just making these names up as I type) DOMFile, PHPFile and YamlFile classes). This signals users that these classes can process a files content.
Another interface could then be CompressorInterface, which might be useful when dealing with gzipped text files.
Of course, all these classes can extend from a generic abstract class FileAdapter or something, that specifies generic read-write methods, and perhaps allows the user to change the mode to binary, read-only, or even stream... Just go wild here :)

Remember, remember: files are files
After all this talk and suggestions of what you can do, it's important to stress that, while you can do a lot, you can take things to far: these are files, and files mean I/O. If you build an API that lends itself to file usage a bit too well, you could end up with people using files as a replacement for DB storage. Don't.
If people might use your API as an alternative to caching, or simple DB's, then you might have over-done it.

Copy with pride, but credit
A lot of open-source projects do offer some classes to read/process files already. There's nothing wrong with looking at those for inspiration. Copy what you need, provided the license permits it. And of course, feel free to contribute, too :)

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the review and suggestions. I always knew there were a couple of code smells surrounding that class and one like it in my codebase. What I was trying to achieve was an immutable class architecture. A sort of black box, something goes in, something comes out (maybe I'm more suited to functional programming). The reason is sympathy for my client code. One thing I hate to do is have 3 or 4 lines of code mutating a class with setters and then a final getter before doing a task. But your examples were good and this is something I mostly struggle with. \$\endgroup\$ – Dingo Egret Feb 3 '14 at 18:47
  • \$\begingroup\$ @DingoEgret: It's not you, a method/class should be X goes in, Y comes out. Always. That's why I suggested specific classes for each type of output/each action. That's what the SRP is all about. PS: I like functional programming, too, that needn't be a hindrance, though \$\endgroup\$ – Elias Van Ootegem Feb 4 '14 at 7:18

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.