# Directory Filter Class PHP

I'd like feedback on the class below, from the perspective of wanting to write professional code that I would be paid to create. I would appreciate comments on details such as commenting, naming and layout as well as the actual functionality.

The code is a class which takes two arguments (directory and extension) and searches the directory for instances of files with the given extension.

The context is of me practicing writing PHP with a view to getting a job doing so. There is the possibility of "unknown unknowns" implied in this question.

<?php
/**
* Filters a directory by file-type
*/
class DirectoryFilter{

/**
* Returns a list of files in a given directory,
* filtered by the extension of the files
*
* @param string $dirname * @param string$extension
*
* @return array
*/
public function filter(string $dirname, string$extension){

// store results in an array
$results = []; // set path$path = __dir__ . "/$dirname"; // check directory exists if(is_dir(($path))){

foreach(glob("$path/*.*") as$filename) {
if(pathinfo($filename, PATHINFO_EXTENSION) ==$extension){
array_push($results,$filename);
}
}

return $results; } // Directory not found return false; } }$test = new DirectoryFilter();
print_r ($test->filter('testdir', 'php'));  • This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. Questions should include a description of what the code does – Vogel612 Jan 19 '17 at 12:15 ## 1 Answer I would probably refactor these things: Return Type Declaration You're using argument type declaration, which is a good thing. You can even take it one step further and add a return type declaration as well: : array  This will force the function to return an array. So the final definition will look like this: public function filter(string$path, string $extension) : array {  Return Values I would return an array in any case. If no matches where found this array would simply be empty: return$results;


Check-ups

I would test a few things:

• Is the directory available?
• Is the extension empty?

If any of these test fail, throw an Exception with a message describing the error. So, other parts have to worry about the validity of the arguments.

An example test for readability of an existing directory:

if (!is_readable($path)) {$message = 'The directory "' . $path . '" is not readble.'; throw new \Exception($message);
}


Case Sensitivity

Currently your code will fail if you look for PDF but the file is called example.pdf. Use strtolower to get rid of this problem:

$extension = strtolower($extension);
$pathinfoExtension = strtolower(pathinfo($fileInfo, PATHINFO_EXTENSION));


FilesystemIterator

Currently you're using glob to iterate over the files. Alternatively use a FilesystemIterator. It has the advantages1:

• it skips dot files .|.. (as opposed to an DirectoryIterator)
• it's extensible

Result

The final result would look like this:2

class DirectoryFilter {
public function filter(string $path, string$extension) : ?array {
$results = []; if (!is_dir($path)) {
$message = 'The directory "' .$path . '" doesn\'t exist.';
throw new \Exception($message); } if (!is_readable($path)) {
$message = 'The directory "' .$path . '" is not readble.';
throw new \Exception($message); } if (!$extension) {
$message = 'An empty string is not a valid extension.'; throw new \Exception($message);
}

$extension = strtolower($extension);

$iterator = new FilesystemIterator($path);

foreach ($iterator as$fileInfo) {
$pathinfoExtension = strtolower(pathinfo($fileInfo, PATHINFO_EXTENSION));

if ($extension ==$pathinfoExtension) {
$results[] =$fileInfo->getFilename();
}
}

return \$results;
}
}

var_dump( (new DirectoryFilter())->filter(__DIR__, 'php') );


Further Thoughts

To reduce the amount of false-positives, like files that have a wrong extension, you should probably use some sort of MIME-type guesser. A good one, based on the PECL-extension, is the one included in the Symfony framework. The guesser uses two components: FileBinaryMimeTypeGuesser and FileinfoMimeTypeGuesser. The Symfony source code is a good starting point to go more into detail:

http-foundation / File / MimeType / MimeTypeGuesser.php

1 You can find more details in the answers of this question "What exactly are the benefits of using a PHP 5 DirectoryIterator over PHP 4 “opendir/readdir/closedir”?".

• Agreee on everything except that null value. Returning null is programming nightmare (heard of null pointer exception?), its much worse than empty array. Every attempt to do something with the result will have to be preceded with null check. And adding this nullable return type has actually extended language misuse posibilities even more (while nullable argument is in effect just a syntactic sugar for default null value, which itself is a hacky way for method overloading). – shudder Jan 19 '17 at 19:14