Selecting a random image from a subdirectory

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 RandomImage

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." 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;
}

StackExchange Code Review Q#15004, answer score: 7