Selecting a random image from a subdirectory

Question

I'm looking for code correctness and improvements for the following code. It's my first step into class based programing.

What the class does, is simply to read a directory (path is passed as constructor argument) and find all sub directories matched by the search pattern. Then it returns a random directory (strimg) based on the result set.

I assuming this directory structure:

Directory structure

backend
    img
        kombi-1
        kombi-2
        kombi-3

PHP

/**
 * RandomImages
 * 
 * This class reads based on the passed argument, all directories
 * matched by the pattern ($pattern) and returns a random path
 * from the result.
 */

class RandomImages {

    private $array;
    private $path;
    private $pattern = "/kombis/*";

    /**
     * __construct function.
     * 
     * @access public
     * @param mixed $path
     */
    public function __construct($location) {
        $this->location = $this->cleanLocation($location);
    }

    /**
     * getDirectories function.
     *
     * Return an array of directories matched by $pattern
     * 
     * @access private
     * @return array
     */
    private function getDirectories(){
        return glob( $this->location . $this->pattern , GLOB_ONLYDIR ); 
    }

    /**
     * getRandomKey function.
     * 
     * @access private
     * @return int
     */
    private function getRandomKey() {
        $array = $this->getDirectories();
        return array_rand($array, 1);
    }

    /**
     * cleanLocation function.
     * 
     * Remove safely the last /
     *
     * @access private
     * @param mixed $string
     * @return string
     */
    private function cleanLocation($string) {
        return rtrim($string, '/');
    }

    /**
     * getRandomPath function.
     * 
     * @access public
     * @return string
     */
    public function getRandomPath() {
        $array = $this->getDirectories();
        return $array[$this->getRandomKey()];
    }
}

// Init
$path = new RandomImages('./backend/img');

// Save path to prevent false combinations
$randomPath = $path->getRandomPath();

// Build link and store string 
$pattern = $randomPath . '/pattern.png';
$curtain = $randomPath . '/curtain.png';

UPDATED CLASS

<?php

    /**
     * RandomImages
     * 
     * This class reads based on the passed argument, all directories
     * matched by the pattern ($pattern) and returns a random path
     * from the result.
     */

    class RandomImages {

        /**
         * Basepath
         * 
         * @var string Trailing slash is stripped
         */
        private $location;

        /**
         * __construct
         * 
         * @param string $path
         */
        public function __construct($location) {
            $this->location = $this->stripTrailingSlash($location);
        }

        /**
         * setPattern
         * 
         * @return void
         */
        public function setPattern($pattern) {
            $this->pattern = $pattern;
        }

        /**
         * getPattern.
         * 
         * @return string
         */
        public function getPattern() {
            return $this->pattern;
        }

        /**
         * getDirectories
         *
         * Returns an array of directories matched by setPattern()
         * 
         * @return array
         */
        private function getDirectories(){
            return glob( $this->location . $this->pattern , GLOB_ONLYDIR ); 
        }

        /**
         * stripTrailingSlash
         * 
         * Remove safely the last /
         *
         * @param mixed $string
         * @return string
         */
        private function stripTrailingSlash($string) {
            return rtrim($string, '/');
        }

        /**
         * getRandomPath
         * 
         * @return string
         */
        public function getRandomPath() {
            $array = $this->getDirectories();
            return $array[array_rand($array, 1)];
        }
    }

// Init
$path = new RandomImages('./backend/img/kombis');
$path->setPattern('/kombi*');

Answer 1

While not a problem, you aren't using the $path and $array properties and should remove them. You must call getRandomPath to get the actual path from the object.

$randomImages = new RandomImages('./backend/img');
$path = $randomImages->getRandomPath();

Also, you load the list of directories twice--once to get a random key and again to pull its value. Since you don't expose getRandomKey you may as well move it inside getRandomPath.

public function getRandomPath() {
    $array = $this->getDirectories();
    return $array[array_rand($array, 1)];
}

Here are a few stylistic pointers as well.

• Declare all properties. You are missing a declaration for $location at the top of the class.
• Add PHPDoc comments to properties--even the private ones.
  

/**
 * Base path that must contain the "kombis" directory.
 *
 * @var string Trailing slash is stripped
 */
private $location;

• Don't bother writing "<function-name> function." as the first line of each function's documentation. It adds nothing, takes up visual space, and will be obvious in the generated documentation.
• Drop the @access attributes as they're inferred from the method declarations.

Update

• You're still missing the declaration of $pattern.
• You're not initializing $pattern in the class. What happens if you forget to call setPattern? You could add it as another constructor parameter or assign a default value in the declaration--or both using a parameter default.
  

public function __construct($location, $pattern = '/kombi*') {

• Every method that doesn't return a value can be assumed to return void, however not that assigning that return value to a function will actually assign null. You can omit the @return void unless you want to be really pedantic. :)
• setPattern is missing its @param for $pattern.
• If you still wanted to have getRandomKey or you want to call getRandomPath multiple times, you can cache the array of directories in an instance property and reuse it. Just make sure to clear it when the pattern or base path are changed.
  

/**
 * List of directories that match the pattern.
 *
 * @var array null until initialized
 */
private $directories = null;

/**
 * Sets the pattern for loading and clears the cached directories.
 *
 * @param string $pattern appended to $location and passed to glob()
 */
public function setPattern($pattern) {
    $this->pattern = $pattern;
    $this->directories = null;
}

/**
 * Loads the matching directories and caches them for reuse.
 *
 * @return array list of directories that match $pattern
 */
private function getDirectories() {
    if ($this->directories === null) { // make sure to use triple-equals here
        $this->directories = glob( $this->location . $this->pattern , GLOB_ONLYDIR );
    }
    return $this->directories;
}

Answer 2

A couple of things to add to David's post. Nothing major, he did an excellent job +1

You removed the reference to your $pattern property, but you are still using it. Any properties you are using in a class should ALWAYS be declared. This makes it easier to determine if a property actually belongs to a specific class, or if there were typos. It also ensures that the property is in the right scope. For instance, you wouldn't want a password in the public scope. And eventually this will be required, as I have heard that PHP is decrementing "default" scopes. A step forward in my opinion. Also, when initially declaring class properties, you can combine similarly scoped properties. Some people don't like using this method because they think that you can't add doc comments to them, but you can. For example:

private
    /**@var string Trailing slash is stripped*/
    $location,

    /**@var string Pattern to file*/
    $pattern = '/kombis/*'
;

Your PHPDoc comments should specify proper parameter types, instead of just using "mixed" for everything. I'm assuming these are IDE generated? Mine does the same thing for anything its unsure of, which seems to be everything. Just make sure everything looks right when using any auto generated data. If you have a variable that could be a couple of different things, you can do something along the lines of the following.

* @param string|array $path

Now, about those doc comments. This is something I have been noticing more and more people arguing about recently. Some are of the opinion EVERYTHING needs to be documented, and maybe for public API's this is the case, but I think I'm starting to lean more towards the camp that believes only the unobvious should be documented. For instance, I know, or can guess, what getDirectories() does. However, if there is something unobvious about it, say you decide to start using readdir() instead because glob() uses too much memory (which can be a problem), then you should say so in the doc comments. If your code is self documenting, then there really is no need to redundantly document your code. They are a good thing to have, but sometimes they are unnecessary.

This is more a choice of preference, but you don't have to declare a method's return value to a variable if all you are going to be doing is passing it on to something else. Some people find your way better because it makes it easier to read, some the following way because its smaller, honestly, I have no preference, its just whatever mood I'm in at the time, or whatever looks better :)

private function getRandomKey() {
    return array_rand( $this->getDirectories(), 1 );
}