Validation of multiple items in an array

Question

I've got a FileLocations class which stores the path's to specific files.

class FileLocations
{
    /**
     * @var array
     */
    private $files = [];

    /**
     * @param array $files
     *
     * @throws \Exception
     */
    public function __construct (array $files) {
        if (!$this->areValidFiles($files)) {
            throw new Exception;
        }
        $this->files = $files;
    }

    /**
     * @param $files
     *
     * @return bool
     */
    private function areValidFiles (array $files) {
        foreach ($files as $file) {
            return is_file($file);
        }
        return false;
    }

    /**
     * @return array
     */
    public function getFiles () {
        return $this->files;
    }
}

I've wanted to validate each file (with is_file), so i made the areValidFiles function and looped through every array index which it gets. On each array item it's doing an check.

When i run this code like this:

$fileLocations  = new FileLocations(['doesExist.js', 'doesnotExist.js']);
var_dump($fileLocations->getFiles());

It only does the validation for the first file and doesn't even reconize that there is a second file passed in the arguments.

It also doesn't throw an exception.

Questions

How does this come that it only reconizes one file in the validation and not throws an exception on the second file that doesn't even exist?
How can I make it so that i will be able to pass more parameters inside the areValidFiles function?

The return is_file() is causing the foreach to abort after the first time. Remove that is the first step to solve this. What should happen when it finds a file that does not exist? — Bjorn
– Bjorn, Commented Jan 4, 2015 at 13:51
@Bjorn It should throw an Exception in the __construct method. What do you mean with removing it? — Bas
– Bas, Commented Jan 4, 2015 at 13:52
No, it returns true. The return is_file() basically exits the function, stopping it from looping through the rest of the array. — Sven
– Sven, Commented Jan 4, 2015 at 13:53
Why not throw the exception directly? I.e.: foreach $files as $file: if not is_file($file): throw new Exception. Otherwise you could replace the throw new Exception with return false. — Bjorn
– Bjorn, Commented Jan 4, 2015 at 13:54

u_mulder · Accepted Answer · 2015-01-04 13:54:27Z

1

Your areValidFiles function should be rewritten:

private function areValidFiles (array $files) {
    foreach ($files as $file) {
        if (!is_file($file))
            return false;
    }
    return true;
}

answered Jan 4, 2015 at 13:54

u_mulder

54.8k5 gold badges54 silver badges72 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

Bas Over a year ago

Isn't that the same as what i did? Except reversing the true with the false?

u_mulder Over a year ago

No. it's not the same.

Bas Over a year ago

Can i still do this then? : return !is_file($file) ?

Bjorn Over a year ago

Nope. You only want to return when it finds a file that does not exist. You do not want to return on every iteration of the loop.

u_mulder Over a year ago

Understand that when you use return your function stops working immediately.

|

Manmaru · Accepted Answer · 2015-01-04 13:56:48Z

1

Add a watcher to your function instead of using return after is_file

private function areValidFiles (array $files) {
    $are_all_files_valid = true;
    foreach ($files as $file) {
        if (!is_file($file)) {
            $are_all_files_valid = false;
            break;
        }
    }
    return $are_all_files_valid;
}

answered Jan 4, 2015 at 13:56

Manmaru

5783 silver badges8 bronze badges

1 Comment

Bas Over a year ago

This is nice too, but i preffer @u_mulder 's answer :)

ka_lin · Accepted Answer · 2015-01-04 14:00:04Z

1

I`d do something like:

   /**
     * @param $files
     *
     * @return bool
     */
    private function areValidFiles (array $files) {
        $files = array();
        foreach ($files as $file) {
            $files[$file] = is_file($file);
        }
        return $files;
    }
$fileLocations  = new FileLocations(['doesExist.js', 'doesnotExist.js']);
$fileLocations['doesExist.js'] //true
$fileLocations['doesnotExist.js'] //false

OR using exceptions:

    private function areValidFiles (array $files) {
        foreach ($files as $file) {
            if(!is_file($file)){
                throw new Exception('Invalid file: '.$file);
            }
        }
    }

try{
    $fileLocations  = new FileLocations(['doesExist.js', 'doesnotExist.js']);
} catch (Exception $e)
{
    //do something
}

edited Jan 4, 2015 at 14:00

answered Jan 4, 2015 at 13:54

ka_lin

9,4226 gold badges39 silver badges58 bronze badges

3 Comments

Bas Over a year ago

Thanks, but i dont really like the way of looping through an array of boolean's. I'd stick with u_mulder's way :)

ka_lin Over a year ago

Okay, but this way you would know which file is invalid :)

Bas Over a year ago

Indeed. But i dont really need that :)

Collectives™ on Stack Overflow

Validation of multiple items in an array

3 Answers 3

6 Comments

1 Comment

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

6 Comments

1 Comment

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related