1
\$\begingroup\$

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'));
\$\endgroup\$
  • 3
    \$\begingroup\$ 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 \$\endgroup\$ – Vogel612 Jan 19 '17 at 12:15
4
\$\begingroup\$

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 directory readable?
  • 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:

  • increases readability
  • 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”?".

2 I removed all comments/PHPDoc to keep the answer compact.

\$\endgroup\$
  • \$\begingroup\$ 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). \$\endgroup\$ – shudder Jan 19 '17 at 19:14
  • \$\begingroup\$ @shudder Sorry for my late reply. Thanks for your feedback. I don't know what I was thinking when I wrote this answer. Of course an empty array should be the return value in this case. I was drifting into a scenario where a single object will be returned. But in this case you're absolutely right and of course even Doctrine is doing it like that. Thanks for reviewing me here. I've updated the answer accordingly. \$\endgroup\$ – insertusernamehere Jan 22 '17 at 18:13

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.