6
\$\begingroup\$

I am just getting into the following design patterns.

I have created a class that allows for performing a couple of operations on different remote storage systems: drive, dropbox, .etc (I call these drivers although there might be a better name). The main class accepts the driver whether it be one of the previous, and uses common methods to perform actions. All of the drivers conform to an interface.

  • Am I using the adapter pattern correctly?
  • Are there any things I should take into consideration to improve my code?
    <?php
    
    namespace RestorePoint\Adapters;
    
    use RestorePoint\Adapters\RemoteFilesDriver\RemoteFilesDriverInterface;
    use RestorePoint\Traits\FilesystemTrait;
    use RestorePoint\Helpers\Benchmark;
    use RestorePoint\Exception;
    use Psr\Log\LoggerInterface;
    use Symfony\Component\OptionsResolver\OptionsResolver;
    use Symfony\Component\OptionsResolver\Options;
    use Symfony\Component\OptionsResolver\Exception\InvalidArgumentException;
    use Cake\Filesystem\File;
    
    final class RemoteStorageAdapter {
    
        use FilesystemTrait;
    
        public $options;
        private $driver;
        private $driverClass;
        private $benchmark;
        private $log;
    
        public function __construct(RemoteFilesDriverInterface $driver, Benchmark $benchmark, LoggerInterface $log, array $options = []) {
            $this->benchmark = $benchmark;
            $this->driver = $driver;
            $this->log = $log;
            $resolver = new OptionsResolver();
            $resolver->setDefined(array_keys($options));
            $this->configureOptions($resolver);
            $this->options = $resolver->resolve($options);
            $this->driverClass = $this->driver->handle();
        }
    
        public function configureOptions(OptionsResolver $resolver) {
            $resolver->setDefaults([
                'uploadCheckQuota' => true,
                'cleanup' => true,
                'backupAmountRemote' => null,
                'uploadChunkSize' => 1024 * 1024 * 5,
                'uploadFileSizeLimit' => -1,
            ]);
            $resolver
                    ->setRequired('backupFolderName')
                    ->setAllowedTypes('backupAmountRemote', ['null', 'int'])
                    ->setAllowedTypes('uploadCheckQuota', 'boolean')
                    ->setAllowedTypes('cleanup', 'boolean')
                    ->setAllowedTypes('backupFolderName', 'string')
                    ->setAllowedTypes('uploadChunkSize', 'int');
            $resolver->setNormalizer('backupAmountRemote', function (Options $options, $value) {
                if ($options['cleanup'] == true && is_null($value)) {
                    throw new InvalidArgumentException(__CLASS__ . ': When cleanup is true, backupAmountRemote cannot be null.');
                }
                return $value;
            });
        }
        
        /**
         * __call
         * 
         * @param string $method
         * @param array $arguments
         * @throws Exception
         * @return mixed
         */
        public function __call($method, $arguments) {
            if (method_exists($this->driver, $method)) {
                return call_user_func_array(array($this->driver, $method), $arguments);
            } else {
                throw new Exception("Method {$method} does not exist in {$this->driverClass}");
            }
        }
    
        /**
         * Get service folder
         * 
         * @return string
         */
        public function getFolder() {
            if (($folder = $this->driver->folderExists($this->options['backupFolderName'])) === FALSE) {
                $folder = $this->driver->makeFolder($this->options['backupFolderName']);
            }
            return $folder;
        }
    
        /**
         * Adds files available locally to the service
         * This WILL NOT delete old files on the server
         * 
         * @param string $localDir Full path to local files folder
         * @return boolean
         * @throws Exception
         */
        public function sync($localDir) {
            $folder = $this->getFolder();
            $localFiles = $this->listFilesByCreated($localDir);
            $remoteFiles = $this->driver->listFilesByCreated($folder);
            $localFilesCol = array_column($localFiles, 'name');
            foreach ($remoteFiles as $key => $value) {
                $index = array_search($value['name'], $localFilesCol);
                if ($index !== false) {
                    unset($localFiles[$index]);
                }
            }
            $ct = count($localFiles);
            $this->log->debug("Sync determined {$ct} files should be added to {$this->driverClass}.");
            if ($ct == 0) {
                return false;
            }
            foreach ($localFiles as $file) {
                $this->log->debug($this->driverClass . ': Attempting to add ' . $file['name'] . '...');
                $this->uploadFile($localDir . $file['name']);
            }
            return $ct;
        }
    
        /**
         * Will remove old backup files on the server according to backupAmt
         * 
         * The oldest files are the ones furthest in the past that were uploaded to the server.
         * If you have backupAmt set to 1 and just added a backup to the remote folder from 2015,
         * while having a backup file from 2019 on the server, the file from 2015 will technically
         * be the newest file, and thus the one from 2019 will be deleted. 
         * 
         * This is due to APIs marking the time the file was created as the time they were uploaded,
         * despite that not being the time the file was actually generated.
         * 
         * Under normal circumstances, if your local and remote systems maintain sync, this point
         * is not of much concern. Just keep this in mind.
         * 
         * @return int Files removed
         * @throws Exception
         */
        public function cleanup() {
            $folder = $this->getFolder();
            $i = 0;
            if (is_null($this->options['backupAmountRemote'])) {
                $this->log->info("{$this->driverClass}: backupAmountRemote is null. Calling cleanup will have no effect.");
                return $i;
            }
            $files = $this->driver->listFilesByCreated($folder);
            $files = array_slice($files, $this->options['backupAmountRemote']);
            $ct = count($files);
            if ($ct > 0) {
                $this->log->debug("{$this->driverClass}: Cleanup determined {$ct} files should be removed remotely.");
                foreach ($files as $file) {
                    $this->log->debug($this->driverClass . ': Attempting to delete ' . $file['name'] . '...');
                    $this->driver->deleteFile($file['id']);
                    $this->log->debug($this->driverClass . ': ' . $file['name'] . ' successfully deleted.');
                    $i++;
                }
            }
            return $i;
        }
    
        /**
         * Upload file to service
         * 
         * @param string $file The file to upload
         * @return string Name and ext of file uploaded
         * @throws Exception
         */
        public function uploadFile($file) {
            ini_set('max_execution_time', -1);
            $folder = $this->getFolder();
            $file = new File($file);
            if ($this->driver->fileExists($file->name, $folder)) {
                throw new Exception("{$this->driverClass}: {$file->name} already exists.");
            }
            if ($this->options['uploadCheckQuota']) {
                $this->hasSpace($file->size());
            }
            if ($this->options['uploadFileSizeLimit'] > 0 && $file->size() > $this->options['uploadFileSizeLimit']) {
                throw new Exception("{$this->driverClass}: It is likely the script will take too long to complete. Backup file is too big at {$file->size()} bytes to be uploaded to the service.");
            }
            $this->benchmark->mark('uploadStart');
            $ret = $this->driver->addFile($file, $folder, $this->options['uploadChunkSize']);
            $elapsedTime = $this->benchmark->elapsedTime('uploadStart');
            $this->log->debug("{$this->driverClass}: File {$file->name} successfully added. Task took {$elapsedTime} seconds.");
            return $ret;
        }
    
        /**
         * Checks if a file can be added to service based on its size and the quota available
         * 
         * @param int $fileSize Bytes
         * @return void
         * @throws Exception
         */
        private function hasSpace($fileSize) {
            $spaceRemaining = $this->driver->getSpaceRemaining();
            if ($spaceRemaining === -1) {
                return; // -1 = driver doesn't support this command
            }
            if ($spaceRemaining - $fileSize <= 0) {
                throw new Exception("{$this->driverClass}: Not enough space for {$fileSize} bytes. Only {$spaceRemaining} bytes available.");
            }
        }
    
    }

So as to avoid posting too much code, here is the interface all of the drivers follow:

    <?php
    
    namespace RestorePoint\Adapters\RemoteFilesDriver;
    
    use RestorePoint\Adapters\Service\ServiceInterface;
    use Cake\Filesystem\File;
    
    interface RemoteFilesDriverInterface {
    
        public function __construct(ServiceInterface $service);
    
        /**
         * Should return a human name version of the driver
         * 
         * @return string
         */
        public function handle();
    
        /**
         * Simply checks if a file exists remotely
         * 
         * @param string $file
         * @param string $folder
         * @return boolean
         */
        public function fileExists($file, $folder);
    
        /**
         * Deletes a remote file
         * 
         * @param string $file
         * @throws Exception
         */
        public function deleteFile($file);
    
        /**
         * Check if a folder exists, if so, return ID or path
         * 
         * @param string $folder
         * @return mixed The ID or path of the folder
         */
        public function folderExists($folder);
    
        /**
         * Makes folder and returns folder ID or path
         * 
         * @param string $folder
         * @return mixed The ID or path of the folder
         */
        public function makeFolder($folder);
    
        /**
         * Gets free space - used space
         * 
         * @return int
         * @throws Exception
         */
        public function getSpaceRemaining();
    
        /**
         * Lists files in order from newest to oldest
         * Conforms file to an associative array with keys
         * name, id, created
         * 
         * Name: somefile.zip
         * Id: /path/to/file or 123123
         * Created: timestamp representation of date created
         * 
         * @param mixed $folder ID or path of folder
         * @return array
         * @throws Exception
         */
        public function listFilesByCreated($folder);
    
        /**
         * Uploads a file to remote service
         * 
         * @param File $file Instance of Cake\Filesystem\File
         * @param mixed $folder Path or ID of folder
         * @param int $chunkSizeBytes
         * @return string filename
         * @throws Exception
         */
        public function addFile(File $file, $folder, $chunkSizeBytes);
        
        /**
         * Not yet implemented
         * 
         * @param mixed $file
         * @param string $destination
         */
        public function download($file, $destination);
    }

I would also argue, based on my understanding, that the drivers themselves are adapters for the service libraries they implement (Google Client Services, Dropbox API) to get a predefined output. But I am unsure of this.

update:

Here is an example driver

\$\endgroup\$
2
  • \$\begingroup\$ ... or is it a strategy pattern?? \$\endgroup\$
    – Alex
    Aug 2, 2019 at 9:56
  • \$\begingroup\$ Would it be better to use interface rather than if (method_exists(? \$\endgroup\$ Apr 18, 2023 at 7:18

1 Answer 1

2
\$\begingroup\$

The code looks very well documented. It makes good use of type declarations for arguments.

Are there any things I should take in to consideration to improve my code?

Follow PHP Standards Recommendations

While there are no de jure rules about readability conventions and it is up to individuals/teams to decide their conventions, idiomatic PHP code often follows the PHP Standards Recommendations - e.g. PSR-12: Extended Coding Style, which replaced the deprecated PSR-2: Coding Style Guide 9 days after the code here was posted. The code presented follows many of the recommendations though not all.

----------------------------------------------------------------------
FOUND 23 ERRORS AND 5 WARNINGS AFFECTING 26 LINES
----------------------------------------------------------------------
   1 | ERROR   | [x] End of line character is invalid; expected "\n"
     |         |     but found "\r\n"
  15 | ERROR   | [x] Opening brace of a class must be on the line
     |         |     after the definition
  25 | WARNING | [ ] Line exceeds 120 characters; contains 134
     |         |     characters
  25 | ERROR   | [x] Opening brace should be on a new line
  36 | ERROR   | [x] Opening brace should be on a new line
  53 | WARNING | [ ] Line exceeds 120 characters; contains 125
     |         |     characters
  61 | ERROR   | [x] Whitespace found at end of line
  67 | ERROR   | [x] Opening brace should be on a new line
  77 | ERROR   | [x] Whitespace found at end of line
  80 | ERROR   | [x] Opening brace should be on a new line
  81 | ERROR   | [x] TRUE, FALSE and NULL must be lowercase; expected
     |         |     "false" but found "FALSE"
  90 | ERROR   | [x] Whitespace found at end of line
  95 | ERROR   | [x] Opening brace should be on a new line
 120 | ERROR   | [x] Whitespace found at end of line
 124 | ERROR   | [x] Whitespace found at end of line
 125 | ERROR   | [x] Whitespace found at end of line
 128 | ERROR   | [x] Whitespace found at end of line
 131 | ERROR   | [x] Whitespace found at end of line
 135 | ERROR   | [x] Opening brace should be on a new line
 159 | ERROR   | [x] Whitespace found at end of line
 164 | ERROR   | [x] Opening brace should be on a new line
 175 | WARNING | [ ] Line exceeds 120 characters; contains 192
     |         |     characters
 180 | WARNING | [ ] Line exceeds 120 characters; contains 124
     |         |     characters
 186 | ERROR   | [x] Whitespace found at end of line
 191 | ERROR   | [x] Opening brace should be on a new line
 197 | WARNING | [ ] Line exceeds 120 characters; contains 137
     |         |     characters
 201 | ERROR   | [x] Expected 1 newline at end of file; 0 found
 201 | ERROR   | [x] The closing brace for the class must go on the
     |         |     next line after the body

Return type declarations can be added

Presuming the code is running on PHP 7 or later, return type declarations for the methods can be added.

Use strict equality comparisons

It is a good habit to use strict equality operators whenever possible. There are three occurrences of strict equality comparison operators (e.g. ===) used, though some conditional statements use loose equality operators - i.e. ==.

if ($options['cleanup'] == true && is_null($value)) {

Most idiomatic PHP code would simply use $options['cleanup'] instead of ($options['cleanup'] == true.

$ct = count($localFiles);
if ($ct == 0) {
     return false;
}

count() returns an integer so strict equality could be used there also, though again idiomatic PHP code would typically just use if (!$ct).

Keyword else can be avoided in some cases

In the __call() method there is a check to see if the method exists:

   if (method_exists($this->driver, $method)) {
       return call_user_func_array(array($this->driver, $method), $arguments);
   } else {
       throw new Exception("Method {$method} does not exist in {$this->driverClass}");
   }

It could be simplified to not need the else and decrease the indentation level of the return statement by flipping the logic flow:

    if (!method_exists($this->driver, $method)) {
        throw new Exception("Method {$method} does not exist in {$this->driverClass}");
    } 
    return call_user_func_array(array($this->driver, $method), $arguments);

Some documentation is not always correct

The docblock for sync shows:

    /**
    * Adds files available locally to the service
    * This WILL NOT delete old files on the server
   * 
   * @param string $localDir Full path to local files folder
   * @return boolean
   * @throws Exception
   */

Within the method there is a place where a boolean can be returned:

if ($ct == 0) {
    return false;
}

Yet later at the end of the method the return statement is:

return $ct;

Where $ct was assigned the integer returned from calling count. Thus the method can return a boolean or an integer. If the code is running on PHP 8 or later then the mixed type could be used in the method's type declaration.

Manually incrementing in cleanup seems superfluous

The cleanup method appears to attempt to delete files and at the end of the foreach loop increments $i which is returned at the end of the method. It appears there is no code to handle exceptions so if a file was not deleted then $i would still get incremented. Unless I am missing something it can simply return $ct like the sync method does, and there is no reason to declare and increment $i.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.