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
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:
protected $iterator = null;
public function __construct($path = null)
if ($path) $this->setPath($path);
public function setPath($path)
$this->iterator = new \RecursiveDirectoryIterator(
dirname(__FILE__) . $path
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:
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?
const COLLECT_ABSTRACT = 0;
const COLLECT_REGULAR = 1;
const COLLECT_BOTH = 2;
$iterator = null;
$collection = array();
public function getCollection($gatewayNS, $which = self::COLLECT_ABSTRACT)
throw new \InvalidArgumentException('invalid offset (0,1,2 are allowed)');
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));
$class = new \ReflectionClass($class);
$this->collections[$ns][self::COLLECT_BOTH] = $class;
$this->collection[$ns][self::COLLECT_ABSTRACT] = $class;
$this->collection[$ns][self::COLLECT_REGULAR] = $class;
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
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)
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 :)