4
\$\begingroup\$

I would love some feedback on this little utility function:

/**
 * Copy $src to $dest, renaming it if the desired destination already exists.  The
 * string '.versionX' is inserted before the file extension if a file must be
 * renamed, with X incremented as needed.
 *
 * @return boolean true if copy was successful, false otherwise.
 */
public static function copy_with_rename($src, $dest)
{
    if (file_exists($dest))
    {
        // See if it's the same.
        $existingFileHash = sha1_file($dest);
        $new_file_hash = sha1_file($src);
        if ($existingFileHash != $new_file_hash)
        {
            // File of same name exists, but is different.
            $last_dot_pos = strrpos($dest,'.');
            $file_count = 2;
            // So find a new name.
            do
            {
                $base_name = substr($dest, 0, $last_dot_pos);
                $ext = substr($dest, strlen($base_name)); // with dot
                $new_name = $base_name.'.version'.$file_count.$ext;
                $file_count++;
            } while (file_exists($new_name) && sha1_file($src)!=sha1_file($new_name));
            // And then try the copy again, with the new name.
            return file::copy_with_rename($src, $new_name);
        } else
        {
            return true;
        }
    } else
    {
        $dest_dir = dirname($dest);
        // Create (to any depth) the destination directory, if required.
        if (!file_exists($dest_dir))
        {
            mkdir($dest_dir, 0777, true);
        }
        // And finally do the actual copy.
        return copy($src, $dest);
    }
}
\$\endgroup\$
3
\$\begingroup\$

Original Answer:

I'd put in some extra brackets in your while-loop for radability:

while ( file_exists($new_name) 
        && (sha1_file($src) != sha1_file($new_name))
      );

And you shouldn't set file-permissions to 777 by default (660 or even less). Everything else looks quite ok in my eyes (although I don't like the do/while construct, but I guess that's more a personal thing)

Update for comment:

$srcSH1 = sha1_file($sec);
do
{
    // ...
}
while ( file_exists($new_name) 
        && (sha1_file($new_name) != $srcSH1)
      );

You should move out the sha_file($src) as this doesn't change but is recalculated each iteration. And that's the case why I don't like do/while. Splits the $srcSH1 and the while-condition where it is used.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for reviewing! I might just change the while() to do the two clauses separately, for easier reading: $already_exists = file_exists($new_name); $is_different = sha1_file($src) != sha1_file($new_name); } while ($already_exists && $is_different); I know what you mean about do-while: I guess it's just not all that widely used (and familiar), so it can take a moment to see what's happening. But in this case, it's the cleanest way. And I'll change the permissions to 600 (don't know why the default for mkdir() is 0700; I was using this on Windows anyway, so didn't think about it). \$\endgroup\$
    – Sam Wilson
    May 30 '11 at 3:29
  • \$\begingroup\$ (erg. forgot that comments have crap formatting!) \$\endgroup\$
    – Sam Wilson
    May 30 '11 at 3:30
  • \$\begingroup\$ @Sam: It's fine to have that kind of statements in while-loops. Just make sure to format the loop correctly. \$\endgroup\$
    – Fge
    May 30 '11 at 9:04
  • \$\begingroup\$ Good point about moving the hash out of the loop — and then it can also be used in the if clause above. \$\endgroup\$
    – Sam Wilson
    May 30 '11 at 23:50

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.