I created a Backup class because I couldn't find one that worked well on Windows and remotely, to do this I have combined a lot of questions on stackoverflow with a lot of trial and error to get it to work on these two platforms. I'm fairly new to OOP, and I really just want to get some tips on handling passing error messages, and I feel like I'm returning a lot (because some functions shouldn't continue if previous ones have failed), and general thoughts on the class. I have included setup stuff at the top, and some functions the class uses on the bottom (obviously I couldn't include them all). As you may notice the script backs up both user specified tables in a database (and I know I should be using PDO, but ignore that for now) as well as user specified folders and whats cool is the zippy function.
<?php
if (!defined('script_access')) {
exit('No direct script access allowed');
}
define('APP_PATH', realpath(dirname(__FILE__)) . DIRECTORY_SEPARATOR);
define('ROOT', realpath(dirname(dirname(dirname(__FILE__)))));
define('CMS_DIR_NAME', 'neou_cms');
define('CMS_DIR', ROOT . DIRECTORY_SEPARATOR . CMS_DIR_NAME . DIRECTORY_SEPARATOR);
define('FRONTEND_IMAGE_UPLOAD_PATH', ROOT . DIRECTORY_SEPARATOR . 'images' . DIRECTORY_SEPARATOR . 'uploads' . DIRECTORY_SEPARATOR);
define('BACKEND_IMAGE_UPLOAD_PATH', CMS_DIR . 'images' . DIRECTORY_SEPARATOR);
$backup_core = new Backup();
$backup_core->dir_backup = CMS_DIR . 'backup' . DIRECTORY_SEPARATOR;
$backup_core->dir_sql = $backup_core->dir_backup . 'sql' . DIRECTORY_SEPARATOR;
$backup_core->dir_files = $backup_core->dir_backup . 'files' . DIRECTORY_SEPARATOR;
$backup_core->dir_unpack = ROOT;
$backup_core->folders_to_backup = array(
FRONTEND_IMAGE_UPLOAD_PATH,
BACKEND_IMAGE_UPLOAD_PATH . 'users' . DIRECTORY_SEPARATOR
);
class Backup
{
/************* BACKUP CONFIGURATION ******************/
// folder locations
public $dir_backup;
public $dir_files;
public $dir_sql;
// unpack directory, should be at the first writable directory
public $dir_unpack;
// array of folders
public $folders_to_backup;
public $backup_amt;
// tables
public $tables_to_backup = NULL;
// don't change
private $sqlDirTemp;
private $msg;
private $totalSiteSize;
/*************** ZIP CONFIGURATION *******************/
// settings
public $sqlDirInZip = 'sql_temp'; // sql dir inside zip
private $overwrite = true; // overwrite file if exists (just incase unlink doesnt work)
// really imp!
private $offsetLocal = -1; // local has fagard_designs after doc root remove one directory
private $offsetRemote = 0; // remote files start in doc root (no offset)
// files and folders to ignore
public $ignored_files = array('.DS_STORE');
public $ignored_folders = array('_notes');
public $ignored_ext = array('');
// don't change
private $zip;
private $SQLinit = false;
private $offset;
public function __construct()
{
ini_set('upload_max_filesize', '200M');
$this->msg = Messages::getInstance();
$this->totalSiteSize = !Localization::isLocal() ? folderSize(ROOT) : '';
// Note: zip will extract in ROOT and put sql there as well
$this->sqlDirTemp = ROOT . DIRECTORY_SEPARATOR . $this->sqlDirInZip . DIRECTORY_SEPARATOR;
extension_loaded('zip') ? $this->zip = new ZipArchive() : '';
$this->offset = Localization::isLocal() ? $this->offsetLocal : $this->offsetRemote;
}
/*------------------------------------------- BACKUP FN'S ------------------------------------------- */
/**
*
* Backup
*
* Creates SQL, and zip backups
*
* @param string $filename
* @param array $tables tables to backup
* @return returns true on success and errors on failure
*
*/
private function backupPkg($filename)
{
$err = '';
$return = '';
// get all of the tables
if (!isset($this->tables_to_backup)) {
$tables = array();
$result = Nemesis::query('SHOW TABLES');
if (!$result) {
return 'Could not gather table names';
}
while ($row = $result->fetch_row()) {
$tables[] = $row[0];
}
// or if the user defines the tables
} elseif (is_array($this->tables_to_backup)) {
$tables = explode(',', $this->tables_to_backup);
} else {
return 'If you are specifying tables to be backed up, you need to provide function ' . __FUNCTION__ . ' with an array.';
}
// cycle through each provided table
foreach ($tables as $table) {
$result = Nemesis::select("*", $table);
$result_q = Nemesis::query("SHOW CREATE TABLE {$table}");
if (!$result && !$result_q) {
return 'SQL dump was unsuccessful. Could not run queries.';
}
$num_fields = $result->field_count;
// first part of the output - remove the table
$return .= 'DROP TABLE ' . $table . ';<|||||||>';
// second part of the output - create table
$result_q_row = $result_q->fetch_row();
$return .= "\n\n" . $result_q_row[1] . ";<|||||||>\n\n";
// third part of the output - insert values into new table
for ($i = 0; $i < $num_fields; $i++) {
while ($row = $result->fetch_row()) {
$return .= 'INSERT INTO ' . $table . ' VALUES(';
for ($j = 0; $j < $num_fields; $j++) {
$row[$j] = addslashes($row[$j]);
$row[$j] = preg_replace("#\n#", "\\n", $row[$j]);
if (isset($row[$j])) {
$return .= '"' . $row[$j] . '"';
} else {
$return .= '""';
}
if ($j < ($num_fields - 1)) {
$return .= ',';
}
}
// add an identifier for explode later
$return .= ");<|||||||>\n";
}
}
$return .= "\n\n\n";
}
if (!is_bool($mkdir = $this->createSaveDir())) {
return $mkdir;
}
$sqlFile = $this->dir_sql . $filename . '.sql';
$handle = @fopen($sqlFile, 'w+');
if (!$handle) {
$err = "Could not open: {$sqlFile}";
}
if (empty($err) && @fwrite($handle, $return) === FALSE) {
$err = "Could write: {$sqlFile}";
}
if (empty($err) && $this->zippyConstructor($this->dir_files . $filename . '.zip', $this->folders_to_backup, $sqlFile) == FALSE) {
$err = "Zip could not complete successfully.";
}
@fclose($handle); // we have to close the sql file so we have permission to delete (if required)
if (!empty($err)) {
@unlink($this->dir_sql . $filename . '.sql');
@unlink($this->dir_files . $filename . '.zip');
return $err;
} else {
return true;
}
}
/**
*
* Creates necessary dirs for save
*
* @returns true on success and errors on failure
*
*/
private function createSaveDir()
{
$dirs = array(
$this->dir_backup,
$this->dir_files,
$this->dir_sql
);
if (!is_bool($mkdir = mkdirIterator($dirs))) {
return $mkdir; // errors
} else {
return true;
}
}
/**
*
* Do Backup
*
* Calls backup
*
* @return adds messages
*
*/
public function doBackup()
{
$filenameFormat = 'dbbackup_' . date(str_replace('-', '.', DATE_FORMAT) . '_H_i_s');
if (!is_bool($output = $this->backupPkg($filenameFormat))) {
$this->msg->add('e', "Backup failed:<br>{$output}");
} else {
$this->msg->add('s', "Backup {$filenameFormat} successfully created.");
}
}
/**
*
* Do Restore
*
* @param string $id restore id
* @param boolean $fromFile if we are restoring from file this should be true
*
*/
public function doRestore($id, $fromFile = false)
{
$err = '';
// remove old folders
foreach ($this->folders_to_backup as $folder) {
rmdirRecursive($folder);
}
if (!$this->zipExtract($this->dir_files . $id . '.zip', $this->dir_unpack)) {
$err = "An error occurred while extracting backup files from zip for backup {$id}!";
}
if (empty($err)) {
if ($fromFile) {
if (!@copy($this->sqlDirTemp . $id . '.sql', $this->dir_sql . $id . '.sql')) {
$err = "Failed to copy SQL file to: {$this->dir_sql}";
}
}
if (empty($err)) {
$open = $this->dir_sql . $id . '.sql';
$handle = @fopen($open, "r+");
if (!$handle) {
$err = "Could not open: {$open}";
}
$sqlFile = @fread($handle, @filesize($open));
if (empty($err) && !$sqlFile) {
$err = "Could not read: {$open}";
}
if (empty($err)) {
$sqlArray = explode(';<|||||||>', $sqlFile);
// process the sql file by statements
foreach ($sqlArray as $stmt) {
if (strlen($stmt) > 3) {
$result = Nemesis::query($stmt);
}
}
}
@fclose($handle);
$fromFile ? rmdirRecursive($this->sqlDirTemp) : '';
}
}
if (empty($err)) {
return "Backup {$id} successfully restored.";
} else {
return "Backup restore failed:<br>{$err}";
}
}
/**
*
* Restores from file upload
* Checks file is zip
* Moves file to $dir_files directory
* Checks zip by extracting to tmp directory
* On success, deletes tmp dir, extracts to root
*
*/
public function doRestoreFromFile()
{
$isCorrect = false;
$filename = $_FILES["zip_file"]["name"];
$source = $_FILES["zip_file"]["tmp_name"];
$type = $_FILES["zip_file"]["type"];
$accepted_types = array(
'application/zip',
'application/x-zip-compressed',
'multipart/x-zip',
'application/x-compressed'
);
foreach ($accepted_types as $mime_type) {
if ($mime_type == $type) {
$isCorrect = true;
break;
}
}
$name = explode(".", $filename);
$continue = strtolower($name[1]) == 'zip' ? true : false;
if (!$isCorrect && !$continue && $_FILES['zip_file']['error'] != 1) {
return 'The file you are trying to upload is not a .zip file. Please try again.';
}
if (!is_bool($mkdir = $this->createSaveDir())) {
return $mkdir;
}
$files = $this->dir_files . $filename;
// move and begin validating contents for sql file
if (@move_uploaded_file($source, $files)) {
$filenameNoExt = self::removeExt($filename);
// make a temp dir to extract and test for sql file
$tmpDir = $this->dir_files . 'temp_' . md5(time()) . DIRECTORY_SEPARATOR;
@mkdir($tmpDir);
// could not extract
if (!$this->zipExtract($files, $tmpDir)) {
return 'An error occurred while extracting files.';
}
// check for sql file
$search_sql = $tmpDir . $this->sqlDirInZip . DIRECTORY_SEPARATOR . $filenameNoExt . '.sql';
if (!@is_file($search_sql) && !@is_readable($search_sql)) {
// remove bad upload
@unlink($files);
// remove temp dir
rmdirRecursive($tmpDir);
rmdirRecursive($files);
return 'There is an error in your file.';
}
// now the zip file must be valid
rmdirRecursive($tmpDir);
if (strpos($output = $this->doRestore($filenameNoExt, true), 'successfully') !== FALSE) {
return rtrim($output, '.') . ' and uploaded!';
} else {
return $output;
}
} else {
return 'There was a problem with the upload. Please try again.';
}
}
/**
*
* Removes extension from file
*
* @param string $filename
* @return string $filename without extension
*
*/
public static function removeExt($filename)
{
return preg_replace('/\\.[^.\\s]{3,4}$/', '', $filename);
}
/**
*
* Gets extension from file
*
* @param string $filename
* @return string $filename extension
*
*/
public static function getExt($filename)
{
return substr(strrchr($filename, '.'), 1);
}
/**
*
* Maintenance
*
* Handles creation (if frontpage = true) in the event that
* the last backup was over a day old since login and deletion
* of old backups, as defined by $backup_amt
*
* @param bool $frontpage if true then this script can be run on the homepage
*
*/
public function maintenance($frontpage = false)
{
if ($frontpage && isAdmin()) {
// make sure the dirs exists
if (!is_bool($mkdir = $this->createSaveDir())) {
return $mkdir;
}
// if the dir is empty create a backup
if (isDirEmpty($this->dir_sql)) {
$this->doBackup();
// otherwise, do the normal routine
} else {
$path = $this->dir_sql;
$latest_ctime = 0;
$latest_filename = '';
$entry = '';
$d = dir($path);
while (false !== ($entry = $d->read())) {
$filepath = $path . DIRECTORY_SEPARATOR . $entry;
// could do also other checks than just checking whether the entry is a file
if (is_file($filepath) && filectime($filepath) > $latest_ctime) {
$latest_ctime = filectime($filepath);
$latest_filename = $path . $entry;
}
}
/* check if latest backup is over a day old
if it is create a new backup and run the regular
maintenance scripts below */
if (is_file($latest_filename) && date('d', filectime($latest_filename)) < date('d')) {
$this->doBackup();
}
}
}
$list = listDirDate($this->dir_sql);
$handle = opendir($this->dir_sql);
$file = readdir($handle);
$ct = count($list);
if (is_numeric($this->backup_amt) && $ct > $this->backup_amt) {
/* we delete the user specified backup by the backup amt.
say we have 5, user specified only 2 backups to be kept
we delete the last 3 backups */
$list = array_slice($list, 0, $ct - $this->backup_amt);
foreach ($list as $file => $timestamp) {
$filenameNoExt = self::removeExt($file);
@unlink($this->dir_sql . $filenameNoExt . '.sql');
@unlink($this->dir_files . $filenameNoExt . '.zip');
$cleanup[] = $filenameNoExt;
}
}
closedir($handle);
if (!$frontpage && !empty($cleanup)) {
$passed = implode(', ', $cleanup);
$message = "Deleted: {$passed}";
$this->msg->add('w', $message);
}
}
/*------------------------------------------- ZIP ------------------------------------------- */
/**
*
* zippy constructor
*
* Called to create zip files
*
* @param string $destination where the file goes
* @param array $sources array
* @param string $SQLfile if set will include an sql file in the default zip location inside zip
* @return boolean true or false on success or failure, respectively
*
* Note: it is possible to provide sources as $sources = array('ROOT') where ROOT is the root
* directory of the site
*
*/
public function zippyConstructor($destination, $sources = array(), $SQLfile = NULL)
{
if (!extension_loaded('zip')) {
return false;
}
if (file_exists($destination)) {
@unlink($destination);
}
if ($this->zip->open($destination, $this->overwrite == true ? ZIPARCHIVE::OVERWRITE : ZIPARCHIVE::CREATE) !== TRUE) {
return false;
}
if (isset($SQLfile) && is_file($SQLfile)) {
// zip sql file in sql directory
$this->SQLinit = true;
$SQLfile = str_replace('\\', DIRECTORY_SEPARATOR, realpath($SQLfile));
$this->zip->addEmptyDir($this->sqlDirInZip);
$this->zip->addFromString($this->sqlDirInZip . '/' . basename($SQLfile), file_get_contents($SQLfile));
}
foreach ($sources as $source) {
if (!$this->zippy($source)) {
$this->zip->close();
@unlink($destination);
return false;
}
}
if ($this->SQLinit) {
if (!$this->verifySQL($destination)) {
// couldn't verify that sql made it we close dir and remove it, return a false
$this->zip->close();
@unlink($destination);
return false;
}
}
unset($this->SQLinit);
$this->zip->close();
return true;
}
/**
*
* zippy
*
* Creates a zip file from a $source directory
* $include_dir = true means we get the parent directories up until the root directory
* otherwise, we just put in the $source dir and its children.
*
* @param string $source
* @param boolean $include_subs includes sub directories, limited by offset to root
* @param int $cOffset custom offset, $include_subs must be false to work
* default value of -2
*
*/
public function zippy($dir, $include_subs = true, $c0ffset = -2)
{
if (!extension_loaded('zip')) {
return false;
}
$docRootArr = explode('/', rtrim($_SERVER['DOCUMENT_ROOT'], '/') . '/');
$docRootCount = count($docRootArr);
$sOffset = $docRootCount - (1 + $this->offset);
if ($sOffset < 0) {
return false;
}
$dir = realpath($dir);
$files = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($dir), RecursiveIteratorIterator::SELF_FIRST);
foreach ($files as $file) {
if (is_dir($file)) {
continue;
} elseif (is_file($file)) {
// ignore is in the order of most taxing on the system for efficency
$filename = basename($file);
// ignored files
if (in_array($filename, $this->ignored_files)) {
continue;
// ignored extensions
} elseif (in_array(self::getExt($filename), $this->ignored_ext)) {
continue;
// ignored folders (str_replace removes filename which we filtered first)
} elseif (self::strposArray(str_replace($filename, '', $file), $this->ignored_folders)) {
continue;
}
$s1 = str_replace('\\', '/', $file); // normalize slashes
$s2 = explode('/', $s1); // explode the dir by slashes
if ($include_subs) {
// include directories until root (modified by offset)
$s3 = array_slice($s2, $sOffset);
} elseif (isset($c0ffset)) {
// @ -2 just include the file and the directory (go from back of array 2 in (file + dir))
$s3 = array_slice($s2, $c0ffset);
} else {
return false;
}
$s4 = str_replace('//', '/', implode('/', $s3)); // implode and remove double slashes
$this->zip->addFromString($s4, file_get_contents($file)); // add file
}
}
return true;
}
/**
*
* Count Slashes
*
* Given a $str will return the amount of slashes in the string
* We can use this to find the diff (new func needed) between
* /HTML/ and where we are now, and then send that number to minus
*
* @param string $str
* @return int number of slashes in $str
*
*/
public static function countSlashes($str)
{
return substr_count($str, '/');
}
/**
*
* strposArray
*
* Array usage of strpos
* Returns false on first true matching result from the given array
*
* @param string $haystack
* @param string || array $needle
* @param int $offset
* @return boolean
*
*/
public static function strposArray($haystack, $needle, $offset = 0)
{
if (!is_array($needle)) {
$needle = array(
$needle
);
}
foreach ($needle as $query) {
if (strpos($haystack, $query, $offset) !== false) {
return true; // stop on first true result
}
}
return false;
}
/**
*
* verifySQL
*
* Called to check that the $sqlDirInZip exists
* Returns false on failure to affirm the above
*
* @return boolean
*
*/
private function verifySQL()
{
// the '/sql' dir exists
// we want to use '/' instead of directory sep bc windows looks up in zips / instead of \
if ($this->zip->statName($this->sqlDirInZip . '/') !== false) {
return true;
} else {
return false;
}
}
/**
*
* Zip Extract
*
* Extracts a ZIP archive to the specified extract path
*
* @param string $file The ZIP archive to extract (including the path)
* @param string $extractPath The path to extract the ZIP archive to
* @return boolean True if the ZIP archive is successfully extracted
*
*/
public function zipExtract($file, $extractPath)
{
if (!extension_loaded('zip')) {
return false;
}
if ($this->zip->open($file) === TRUE) {
$this->zip->extractTo($extractPath);
$this->zip->close();
return true;
} else {
return false;
}
}
/*------------------------------------------- STATS ------------------------------------------- */
/**
*
* Percentage Overflow
*
* Child function of findBackupPercentage
* Determines if the backup dir is too large
*
* @return bool true if the backup dir is greater than 25% of the entire site size
*
*/
public function percentageOverflow()
{
if (!Localization::isLocal()) {
$stmt = str_replace('%', '', $this->findBackupPercentage());
// return true if backup folder percentage is higher than 25% (1/4) of the total site
if ($stmt > 25) {
return true;
} else {
return false;
}
} else {
return 'NaN';
}
}
/**
*
* Find Backup Percentage
*
* Determines the percentage of the backup dir
* relative to the entire site size
*
* @param int $precision (default 2 decimal points)
* @return percentage or NaN
*
*/
public function findBackupPercentage($precision = 2)
{
if (!Localization::isLocal()) {
$total_backup_folder = folderSize($this->dir_backup);
$percentage = ($total_backup_folder / $this->total_site_size) * 100;
return round($percentage, $precision) . '%';
} else {
return 'NaN';
}
}
/**
* Find Backup File Percentage
*
* Determines the percentage of a backup file
* relative to the entire site size
*
* @param int $precision (default 2 decimal points)
* @return percentage or NaN
*
*/
public function findBackupFilePercentage($filename, $precision = 2)
{
if (!Localization::isLocal()) {
$size_files = filesize($this->dir_files . $filename . '.zip');
$size_sql = filesize($this->dir_sql . $filename . '.sql');
$total_file = $size_files + $size_sql;
$percentage = ($total_file / $this->total_site_size) * 100;
return round($percentage, $precision) . '%';
} else {
return 'NaN';
}
}
}
?>
functions from other classes / functions:
public static function isLocal()
{
$whitelist = array(
'localhost',
'127.0.0.1'
);
if (in_array($_SERVER['HTTP_HOST'], $whitelist)) {
return TRUE;
} else {
return FALSE;
}
}
function rmdirRecursive($dir, $empty = false)
{
if (substr($dir, -1) == "/") {
$dir = substr($dir, 0, -1);
}
if (!file_exists($dir) || !is_dir($dir)) {
return false;
} elseif (!is_readable($dir)) {
return false;
} else {
$handle = opendir($dir);
while ($contents = readdir($handle)) {
if ($contents != '.' && $contents != '..') {
$path = $dir . "/" . $contents;
if (is_dir($path)) {
rmdirRecursive($path);
} else {
unlink($path);
}
}
}
closedir($handle);
if ($empty == false) {
if (!rmdir($dir)) {
return false;
}
}
return true;
}
}
function folderSize($dir)
{
if (is_dir($dir)) {
$total_size = 0;
$files = scandir($dir);
$cleanDir = rtrim($dir, '/') . '/';
foreach ($files as $t) {
if ($t <> "." && $t <> "..") {
$currentFile = $cleanDir . $t;
if (is_dir($currentFile)) {
$size = foldersize($currentFile);
$total_size += $size;
} else {
$size = filesize($currentFile);
$total_size += $size;
}
}
}
return $total_size;
} else {
return false;
}
}
function mkdirIterator($dirs)
{
$err = array();
if (is_array($dirs)) {
foreach ($dirs as $dir) {
if (!is_dir($dir)) {
if (!mkdir($dir)) {
$err[] .= "Directory '{$dir}' could not be created.";
}
}
}
// check to see if errors occured in the making of more than one directory
if (count($err) > 1) {
$err[] .= "More than one directory could not be made. Please check if you have permission to make folders on the server";
return implode('<br>', $err);
// we must have only one error then...
} elseif (!empty($err)) {
return $err[0];
} else {
return true;
}
} elseif (isset($dirs)) {
if (!is_dir($dirs)) {
if (!mkdir($dirs)) {
return "Directory {$dir} could not be created.";
}
}
}
}
1 Answer 1
This is a lot of code, so this is not a complete review, just a couple of points:
Overall Program Structure
Class
Your Backup
class is a bit over 700 lines long. Without even looking at the code, it's a good guess that it does too much.
What your class does:
- execute sql queries
- write to a file
- read from a file
- zip a file
- unzip a file
- create directories
- something called maintenance
- count slashes
- check size of a directory
Most of those things would deserve their own class. At least three new classes would already be helpful: for reading/writing, zipping/unzipping, and handling of directories.
Methods
Your methods do too many things as well (a method should really only do one thing). Take your backupPkg
method as example: It's 80 lines long (as a rule of thumb: if it doesn't fit on your screen, it's probably too long), and it does multiple things. Even your comment states this: It creates the SQL, it writes it to a file, and it zips the file.
But even if you extract those functionalities, your method to create the SQL would still be too long. I would create a function which is responsible for creating the SQL for inserting data into one table. That way, at least your for - while - for - if - else - if
code is separated from the rest of the method.
Details
Early Return
I'll take your doRestore
function as example. You ask if (empty($err))
a lot here, and this leads to very deep nesting. It's very confusing to read, and it will lead to bugs quickly. Instead of using your approach, it's better to just return as soon as you get an error (if there was an error, you are not doing anything else anyways).
So your code would look something like this:
public function doRestore($id, $fromFile = false) {
$err = '';
$errPrepend = "Backup restore failed:<br>{$err}";
// remove old folders
foreach ($this->folders_to_backup as $folder) {
rmdirRecursive($folder);
}
if (!$this->zipExtract($this->dir_files . $id . '.zip', $this->dir_unpack)) {
return $errPrepend . "An error occurred while extracting backup files from zip for backup {$id}!";
}
if ($fromFile && !@copy($this->sqlDirTemp . $id . '.sql', $this->dir_sql . $id . '.sql')) {
return $errPrepend . "Failed to copy SQL file to: {$this->dir_sql}";
}
$open = $this->dir_sql . $id . '.sql';
$handle = @fopen($open, "r+");
if (!$handle) {
return $errPrepend . "Could not open: {$open}";
}
$sqlFile = @fread($handle, @filesize($open));
if (!$sqlFile) {
return $errPrepend . "Could not read: {$open}";
}
$sqlArray = explode(';<|||||||>', $sqlFile);
// process the sql file by statements
foreach ($sqlArray as $stmt) {
if (strlen($stmt) > 3) {
$result = Nemesis::query($stmt);
}
}
@fclose($handle);
$fromFile ? rmdirRecursive($this->sqlDirTemp) : '';
return "Backup {$id} successfully restored.";
}
This is still not perfect, but we did get rid of the deep nested ifs.
Nested if
in rmdirRecursive
This pattern is not optimal:
if ($empty == false) { if (!rmdir($dir)) { return false; } } return true;
It takes a while to figure out that true
will be returned if $empty
is false and rmdir
is false
as well. This is clearer:
better:
if (!$empty && !rmdir($dir)) {
return false;
} else {
return true;
}
And then you can just do this:
return !(!$empty && !rmdir($dir))
Or
return !$empty || rmdir($dir)
Naming
backupPkg
: what is Pkg? The name doesn't really tell me this, and neither do the comments.verifySQL
: this method doesn't verify SQL (which is what I would have expected), but checks if a directory exists.
Comments
/** * * Backup * * Creates SQL, and zip backups * * @param string $filename * @param array $tables tables to backup * @return returns true on success and errors on failure * */ private function backupPkg($filename) {
This comment doesn't tell me all that much.
And what it tells me is at least partially wrong (there is no $tables
argument). Comments should always be up-to-date and they should tell me what the method does (which yours does partly; it doesn't tell me that it writes the SQL to a file), what side-effects it has (like your doRestore
method, which deletes directories), what parameters it expects and what values they can have (does the file name has to end in .sql
? can or should I prepend a path to the filename? where is the file stored?; are the table names case sensitive?; etc) and what is returned.
-
\$\begingroup\$ Thank you for your input, it is much appreciated! For some reason I felt sort of dirty returning so much, and I do agree my functions do a lot of different things. I think by splitting them up I would remove alot of the nested ifs. \$\endgroup\$Alex– Alex2014年09月02日 01:25:58 +00:00Commented Sep 2, 2014 at 1:25
-
\$\begingroup\$ Do you feel like I should be handling errors diffrently? Like via try/catch? \$\endgroup\$Alex– Alex2014年09月02日 01:30:16 +00:00Commented Sep 2, 2014 at 1:30
-
\$\begingroup\$ @Alex yes, I think it would be an improvement. For example, the
doRestore
method returns string right now, and then you check if the string containssuccess
or not. This is not good. If instead you throw exceptions indoRestore
, then you don't need to do that. Also, you could try to handle the error by type (couldn't open file, couldn't write to file, etc). \$\endgroup\$tim– tim2014年09月02日 09:03:35 +00:00Commented Sep 2, 2014 at 9:03 -
\$\begingroup\$ This app has certainty changed since '14 :) \$\endgroup\$Alex– Alex2018年03月05日 06:48:27 +00:00Commented Mar 5, 2018 at 6:48
Explore related questions
See similar questions with these tags.