Collecting database file operations

[GoogleCodeExporter]

The code relative to the $database array is spread a bit here and there: main 
flow, dir_tree(), checkDbName(), isManagedDb...

I've tried to collect *most* of the relative operations in a class representing 
a list of database files. Attached is a partial version, I still need to add 
read/write checks and some lists.

The functions above are now methods: addDirectory, canCreate, contains. There 
are also more ways to fill the set: a single filename, a directory, and the 
current configuration format.

The second file attached is a small example/test:
  trunk/classes/DatabaseSet.php
  trunk/test-dbs.php

Any feedback or ideas around this part of the code?

Original issue reported on code.google.com by dreadnaut on 4 Apr 2013 at 7:16

Attachments:

• DatabaseSet.php
• test-dbs.php

[GoogleCodeExporter]

In general: Yes, good idea to bring this together in a class. And good 
implementation, especially using realpath().

What I noticed:
1. allowed_extensions: Why public & no setter? I think some write protection to 
this would be good as it is security relevant. Maybe it should only be possible 
to set it using the constructor.

2. addDirectory: It's a matter of taste, but I think I would have implemented 
this a bit differently. Not that your implementation is bad at all. Just my 
thoughts: 

First, I prefer to implement recursive stuff recursively. Why building up a 
stack yourself if the language can do it for you? Okay, might be more efficient.

Second, I would use DirectoryIterator instead of glob('*'). Mainly because it's 
more readable. (And glob() has such ugly notes in the documentation: "Note: 
This function isn't available on some systems (e.g. old Sun OS)." - this sounds 
like it is still safe on most systems, but could also mean it isn't.)

Third, I would call addFile() from addDirectory, mainly for readability and 
maintainability (e.g. realpath() only once in the code). Your solution is 
probably more efficient, though.

Fourth, I don't see a good reason for $new_files. Why not directly writing to 
$this->_files? In rare cases, you might overwrite some 
$this->_files[realpath($f)] with another "unreal" filename pointing to the same 
file. Who cares? Why is the first "unreal" filename better then the second? Why 
do you keep the "unreal" paths at all?

As I said, just my thoughts. Your code is fine, if you don't feel like 
discussing, just go ahead ;-)

Original comment by crazy4ch...@gmail.com on 26 Apr 2013 at 8:54

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

(Maybe I shouldn't start too many discussions like this about 20 LOC methods 
that do what they are supposed to only because I would have implemented them 
differently...)

Original comment by crazy4ch...@gmail.com on 26 Apr 2013 at 8:58

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

I like these discussions, usually better code comes out of them :)

I'd love to use DirectoryIterator (and RecursiveDirectoryIterator, and 
RecursiveIteratorIterator to avoid recursion or queues) but I find them 
actually less readable, and they feel to expensive for what we are doing. 
Things get a bit better with FilesystemIterator, which however requires 5.3.0+.

For example, RecursiveDirectoryIterator allows you to specify the SKIP_DOTS 
flag, while DirectoryIterator doesn't —so long, code reuse. At that point we 
need to call ->isDot() anyway to check —at that point, let's stick to 
opendir(). Plus, a bunch of objects allocated to list a few files, each one 
opening and closing a file handle —vs opening the directory and calling 
stat() a few times.

Readability, er... starting to look like Java :D

try {
    if ($recursive) {
        $iter = new RecursiveIteratorIterator(new DirectoryRecursiveIterator($path, FilesystemIterator::SKIP_DOTS));
    } else {
        $iter = new DirectoryIterator($path /* sorry, no flags accepted here */);
    }
} catch (UnexpectedValueException $e) {
    return false;
}

foreach ($iter as $file) {
    $filename = $file->getFilename();
    /* same code anyway */
}

If glob is a problem, I'd probably use opendir() at that point. The SPL is 
usually nice, but I'm not sold on these classes.

Apart from this, I've changed the interface a bit compared to the initial code, 
and I can now accept these formats:

/* configuration examples */
$databases_example = array (

// a single database
    'dir/storage.sqlite',

// a single database, with an alias
    'ShortName' => 'mystuff/database_with_a_long_name.sqlite',

// a single database, with an alias (long form, current configuration style)
    array(
        'path' => 'directory/something.sqlite',
        'name' => 'My Something DB',
    ),

// specific files in a directory
    'some_directory/*.{sqlite,db}',

// all the files in a directory
    'some_directory/',

// all the files in a directory, recursively
    array(
        'path' => 'just_a_directory/',
        'recursive' => true,
    ),

);

But I was wondering: which other operations would we need?  Does it make sense 
to include deleting/renaming files here?

Original comment by dreadnaut on 1 May 2013 at 4:28

• Added labels: ****
• Removed labels: ****