Minimizing code copying files using scandir and unlink functions in PHP

$local_dir = $_SERVER["DOCUMENT_ROOT"].'/all/app/Filescletec/glamorousnap/';
$local_server_dir = $_SERVER["DOCUMENT_ROOT"].'/all/app/Filescletec/copyfiles/';
$files = clean_scandir($local_dir);

if (isset($_POST['copy'])) {
	for ($i=0; $i<count($files) ; $i++) {
		if (!file_exists("$local_server_dir/$files[$i]")) {
			if (copy("$local_dir/$files[$i]","$local_server_dir/$files[$i]")) {
				echo "Succesfully copied"; echo " file #"; echo $files[$i]; echo "<br>";
			}
		}else {
			echo "Record already exists!";
			echo "<br>";
		}
	}
}
if (isset($_POST['purge'])) {
	for ($i=0; $i<count($files) ; $i++) {
		if (file_exists("$local_server_dir/$files[$i]")) {
			unlink("$local_server_dir/$files[$i]");
			echo "File Purged: ";echo $files[$i]; echo "<br>";
		} else {
			echo "No file exists to purge with the name: "; echo $files[$i]; echo "<br>";
		}
	}
}

I have to build a small app that copies file from one folder to another, and it works fine, but is it possible to write better code and minimize what I have written in isset part?

There is some repetition in both conditions, so it could be minimised, perhaps with a function with an action param, then possibly sub functions to perform the particular action.

This looks like a clear case for a foreach loop, rather than for.

foreach( $files as $file) {

You mean purge path or both purge and copy.

Both have the exact same loop, so…
Yes, change both loops…
Or have just one loop and perform the action according to condition.

1 Like

Not to be nitpicky, but this

"$local_dir/$files[$i]"

Is the same thing as this

$local_dir . '/' . $files[$i]

I personally don’t like using double quotes unless I really have to (e.g. when using carriage returns) . To me, having double quotes everywhere is a bit messy and hard to read. All I would really see is double quote this, double quote that. That’s why I tend to use variables for their exact purpose so I can cut down on using unnecessary quotes. That’s just me though.

2 Likes

Personally, I’m a big fan of nitpicking. Squiggly brackets can help keep double quoted string a bit clearer.

"{$local_dir}/{$files[$i]}"

I find it easier to read than bouncing between dots. Especially with an editor that understands the syntax.

2 Likes

How about this?

$src = $_SERVER["DOCUMENT_ROOT"].'/all/app/Filescletec/glamorousnap/';
$dest = $_SERVER["DOCUMENT_ROOT"].'/all/app/Filescletec/copyfiles/';
$files = clean_scandir($src);

if (isset($_POST['copy'])) {
	foreach ($files as $file) {
		if (file_exists("$dest/$file")) {
            echo "Record '$file' already exists!<br>";
            continue;
        }
		
        if (!copy("$src/$file", "$dest/$file")) {
            echo "Could not copy '$src/$file' to '$dest/$file'!<br>";
            continue;
        }

        echo "Succesfully copied file '$file'<br>";
	}
}

if (isset($_POST['purge'])) {
    foreach ($files as $file) {
        if (!file_exists("$dest/$file")) {
            echo "No file exists to purge with the name '$file'<br>";
            continue;
        }
		
        if (!@unlink("$dest/$file")) {
            echo "Unable to purge file '$file'<br>";
            continue;
        }

	    echo "File Purged: ";echo $files[$i]; echo "<br>";
	}
}
  • renamed variables to $src and $dest as that makes it more clear what is being copied where
  • use foreach instead of for to loop over the files
  • output when copy or unlink fails, instead of silently ignoring that
  • invert conditions and use continue to stop processing instead of nesting if statements
  • silence E_WARNING that can come from unlink with @

The code isn’t shorter, but it is easier to read IMHO :slight_smile:

Edit: You probably also want to prevent $_POST['copy'] and $_POST['purge'] both being set, as that makes no sense at all.

2 Likes

Here we go :slight_smile:

<?php declare(strict_types=1);
error_reporting(-1);
ini_set('display_errors', 'true');

// $src = $_SERVER["DOCUMENT_ROOT"].'/all/app/Filescletec/glamorousnap/';
// $dst = $_SERVER["DOCUMENT_ROOT"].'/all/app/Filescletec/copyfiles/';

//==================================
function clean_scandir
(
  string $path
)
:array
{
  $old = getcwd();
  chdir($path);
    $result = glob('*.*');
  chdir($old);

  return $result;  // only file names
}

 // old habit of knowing that all "kill" files can be safely deleted
   $src    = '../kill/';
   $dst    = '../kill-001/';
   $files  = clean_scandir($src);

$copy   = $_POST['copy']  ??  FALSE;
$purge  = $_POST['purge'] ??  FALSE;

if (isset($copy, $purge)) :
  foreach($files as $i2 => $file) :

    if($copy) :
      $msg0 = " ==> {$src}{$file}";php
      $msg1 = "File already exists!";
      if ( ! file_exists("{$dst}{$file}")) :
        $msg1 = 'Failed to copy file: ';
        if (@copy("{$src}{$file}", "{$dst}{$file}")) :
          $msg1 = "Succesfully copied"; 
        endif;
      endif;  
      echo $msg1 .$msg0 .'<br>';

    else: // must be $purge 
      $msg0 = " ==> {$dst}{$file}";
      $msg1 = "No file exists to purge with the name: "; 
      if (file_exists("{$dst}{$file}")) :
        unlink("{$dst}{$file}");
        $msg1 = "File Purged: ";
      endif;
      echo $msg1 . $msg0 .'<br>';
    endif; // $purge

  endforeach;// 
endif;// isset

I agree with what @rpkamp recommended apart from the curly brackets :).

1 Like

That’s not gonna work. If both are false then the body if that if statement will still be executed because isset(false) = true.

Also the error supression should be for unlink, not copy :slight_smile:

2 Likes

You mean here →

if (copy("$local_dir/$files[$i]","$local_server_dir/$files[$i]")) {

I mean your whole entire code. You can cut down on unnecessary characters and use them only when you really need them.

I was trying to create a working test and toggle FALSE with TRUE and it worked OK.

The temporary ‘…/kill-001’ directory had insufficient permissions to copy files and prepending @ to copy stopped the errors but required path permissions to be set to 0777. Once set the @ was not required to precede unlink.

@codeispoetry

Rather than trying to minimize the script I prefer to make it more readable and to reduce the variables to a minimum and to reuse where ever possible:

I assume there are only two options and prefer this method:

<?php declare(strict_types=1);
error_reporting(-1);
ini_set('display_errors', 'true');

$_SPACER = ' &nbsp; &nbsp; &nbsp;  ';
echo $links = "
  <div style='text-align:center'>
    <a href='?mode=copy'> Copy </a>
    $_SPACER
    <a href='?mode=purge'> Purge </a>
    $_SPACER
    <a href='?'> Reset </a>
  </div>
    <hr>
    ";

echo '<pre> $_GET ==> ' .print_r($_GET, TRUE) .'</pre>';
echo '<hr>';

//==================================
// return with an array of files in the supplied path
//==================================
function clean_scandir
(
  string $path,
  string $ext = 'php'
)
:array
{
  $old = getcwd();
  chdir($path);
    $aFiles = glob('*.' .$ext);
  chdir($old);

  return $aFiles;  
}





echo '<h2> Refactored script begins here: </h2>';
  $src    = '../kill/';
  $dst    = '../kill-001/';
  $files  = clean_scandir($path=$src, $ext='php');
  $mode   = NULL; // DEFULT
    if( isset($_GET['mode']) ):
      $mode = $_GET['mode']; // either copy or purge
    endif;
  echo '<b>$mode ==> </b>';  var_dump($mode); echo '<hr>';

  switch($mode) :
    default: 
      // NOT NECESSARY - THERE IS NO DEFAULT
    break;

    case 'copy' : 
    case 'purge' : 
      foreach($files as $i2 => $file) :
        $srcFile  = $src .$file;
        $dstFile  = $dst .$file;

        if('copy'===$mode) :
          $msg0 = $srcFile;
          $msg1 = "File already exists!";
          if ( ! file_exists($dstFile) ) :
            $msg1 = 'Failed to copy file: ';
            if (copy($srcFile, $dstFile)) :
              $msg1 = "Succesfully copied"; 
            endif;
          endif;  

        else: // must be 'purge'=== $mode 
          $msg0 = $dstFile;
          $msg1 = "Unable to purge: "; 
          if (file_exists($dstFile)) :
            $ok = @unlink($dstFile);
            $msg1 = $ok ? "File Purged: " : $msg1;
          endif;
        endif; // $purge
        echo $msg1 .' ==> ' .$msg0 .'<br>';
      endforeach;// foreach($files as $i2 => $file)
    break;
  endswitch;  
echo '<h2> Refactored script ends here: </h2>';

on your machine

1 Like

You could use an anonymous function to encapsulate the processing you do on each file, then call that in a loop; that way you’d only need a single foreach loop. The pseudocode below is a summary:

if $_POST['copy'] is set:
    $operation = function ($file) {
        Validate file can be copied
        and then copy it
    };
else if $_POST['purge'] is set:
    $operation = function ($file) {
        Validate file can be deleted
        and then delete it
    };

foreach $file in $files:
    call $operation with $file

However, you will probably find that this code ends up more and more separated as you continue to work on it and the two paths become more different. I’d advise against trying to shrink the code prematurely.

One thing I would suggest is that you remove the @ error suppression. Errors are great! They tell you that something’s wrong and where you need to start looking. PHP can be configured to output errors to the screen or to log them; all you need to do is make your development environment is set to display them on screen and your production environment is set to log them to a file for you to check later. This document has more information on doing that.

1 Like