Что стоит изменить в классе PHP для работы с файлами?

Question

[Максим]

Доброго времени суток.
Хотелось бы получить советы опытных программистов по приведенному ниже классу для работы с файлами.
Понимаю что изобретаю очередной велосипед, но хочется что то свое так сказать для опыта и практики.

<?php

/**
 * MV_Files class file.
 * Class for working with files.
 *
 */

class MV_Files
{

    /**
     * Create new file.
     *
     * @param string $path - the path to the file
     * @param string $name - file name
     * @param int $right - access rights
     * @return boolean return true if success, else false
     */
    public function CreateFile($path, $name, $right = 0777)
    {
        if (!file_exists($path . '/' . $name)) {
            fclose(fopen($path . '/' . $name, 'w'));
            chmod($path . '/' . $name, $right);
            return true;
        } else {
            return false;
        }
    }

    /**
     * Create new file with content.
     *
     * @param string $path - the path to the file
     * @param string $name - file name
     * @param string $text - the contents of the file
     * @param int $right - access rights
     * @return boolean return true if success, else false
     */
    public function CreateFileContent($path, $name, $text, $right = 0777)
    {
        if (!file_exists($path . '/' . $name)) {
            $file = fopen($path . '/' . $name, 'w');
            fwrite($file, $text);
            fclose($file);
            chmod($path . '/' . $name, $right);
            return true;
        } else {
            return false;
        }
    }

    /**
     * Add content to the end of file.
     *
     * @param string $path - the path to the file
     * @param string $text - the contents of the file
     * @return boolean return true if success, else false
     */
    public function AddFileContent($path, $text)
    {
        if (file_exists($path)) {
            $file = fopen($path, 'a');
            fwrite($file, $text);
            fclose($file);
            return true;
        } else {
            return false;
        }
    }

    /**
     * Overwrite file.
     *
     * @param string $path - the path to the file
     * @param string $text - the contents of the file
     * @return boolean return true if success, else false
     */
    public function OverwriteFileContent($path, $text)
    {
        if (file_exists($path)) {
            $file = fopen($path, 'w');
            fwrite($file, $text);
            fclose($file);
            return true;
        } else {
            return false;
        }
    }

    /**
     * Rename file.
     *
     * @param string $oldname - the old name
     * @param string $newname - the new name
     * @return boolean return true if success, else false
     */
    public static function RenameFile($oldname, $newname) {
        if (file_exists($oldname)) {
            return rename($oldname, $newname);
        } else {
            return false;
        }
    }

    /**
     * Delete file.
     *
     * @param string $path - the path to the file
     * @return boolean return true if success, else false
     */
    public function DeleteFile($path)
    {
        if (file_exists($path)) {
            return unlink($path);
        } else {
            return false;
        }
    }

    /**
     * Get file content.
     *
     * @param string $path - the path to the file
     * @return string if success, else false
     */
    public function getFileContent($path)
    {
        if (file_exists($path)) {
            return file_get_contents($path);
        } else {
            return false;
        }
    }

    /**
     * Get file size, convert and return size in Gb, Mb, Kb or Bytes.
     *
     * @param string $path - the path to the file
     * @return string if success, else false
     */
    public function getFileSize($path)
    {
        if (file_exists($path)) {
            (int)$size = filesize($path);
            switch (true) {
                case $size >= 1073741824:
                    $size = round($size / 1073741824 * 100) / 100 . ' GB';
                    break;
                case $size >= 1048576:
                    $size = round($size / 1048576 * 100) / 100 . ' MB';
                    break;
                case $size >= 1024:
                    $size = round($size / 1024 * 100) / 100 . ' KB';
                    break;
                default:
                    $size .= ' Bytes';
                    break;
            }
            return $size;
        } else {
            return false;
        }
    }

    /**
     * Get information about the file.
     * Array(
     *  [dirname] => value
     *  [basename] => value
     *  [extension] => value
     *  [filename] => value
     * )
     *
     * @param string $path - the path to the file
     * @return array if success, else false
     */
    public function getFileInfo($path)
    {
        if (file_exists($path)) {
            return pathinfo($path);
        } else {
            return false;
        }
    }

    /**
     * Get files list from directory.
     *
     * @param string $dirname - the path to the directory
     * @return array
     */
    public function getFileList($dirname)
    {
        $dir = array_diff(scandir($dirname), ['.', '..']);
        $files = [];
        foreach ($dir as $value) {
            if (is_file($value)) {
                $files[] = $value;
            }
        }
        return $files;
    }

}

Answer 1

[index0h]

Конкретно по коду отписал ниже. Как хелпер - ну может быть, почитайте на досуге php.net/manual/ru/spl.files.php

// Я надеюсь вы знаете про PSR4 и символ подчеркивания тут случайно
class MV_Files
{
    // PSR2: методы пишутся в lowerCamelCase
    // Нет проверки аргументов. что произойдет, если $name будет массивом?
    // Не ясно, на кой вам тут возврат bool? Если что-то не так - бросайте исключение.
    public function CreateFile($path, $name, $right = 0777)
    {
        if (!file_exists($path . '/' . $name)) {
            // Посмотрите http://php.net/manual/ru/function.touch.php
            fclose(fopen($path . '/' . $name, 'w'));
            chmod($path . '/' . $name, $right);
            // PSR2: Перед return перевод строки
            return true;
            // Не ясно, зачем вам тут else?
        } else {
            return false;
        }
    }

    // PSR2: методы пишутся в lowerCamelCase
    // Нет проверки аргументов. что произойдет, если $name будет массивом?
    // Не ясно, на кой вам тут возврат bool? Если что-то не так - бросайте исключение.
    public function CreateFileContent($path, $name, $text, $right = 0777)
    {
        if (!file_exists($path . '/' . $name)) {
            // file_put_contents, не?
            $file = fopen($path . '/' . $name, 'w');
            fwrite($file, $text);
            fclose($file);
            chmod($path . '/' . $name, $right);
            // PSR2: Перед return перевод строки
            return true;
            // Не ясно, зачем вам тут else?
        } else {
            return false;
        }
    }

    // PSR2: методы пишутся в lowerCamelCase
    // Нет проверки аргументов. что произойдет, если $path будет массивом?
    // Не ясно, на кой вам тут возврат bool? Если что-то не так - бросайте исключение.
    public function AddFileContent($path, $text)
    {
        if (file_exists($path)) {
            // file_put_contents($path, $content, FILE_APPEND)
            $file = fopen($path, 'a');
            fwrite($file, $text);
            fclose($file);
            // PSR2: Перед return перевод строки
            return true;
            // Не ясно, зачем вам тут else?
        } else {
            return false;
        }
    }

    // PSR2: методы пишутся в lowerCamelCase
    // Нет проверки аргументов. что произойдет, если $path будет массивом?
    // Не ясно, на кой вам тут возврат bool? Если что-то не так - бросайте исключение.
    public function OverwriteFileContent($path, $text)
    {
        if (file_exists($path)) {
            // file_put_contents($path, $content)
            $file = fopen($path, 'w');
            fwrite($file, $text);
            fclose($file);
            // PSR2: Перед return перевод строки
            return true;
            // Не ясно, зачем вам тут else?
        } else {
            return false;
        }
    }

    // PSR2: методы пишутся в lowerCamelCase
    // Нет проверки аргументов. что произойдет, если $oldname будет массивом?
    // Не ясно, на кой вам тут возврат bool? Если что-то не так - бросайте исключение.
    // Чего вдруг метод статический?
    public static function RenameFile($oldname, $newname) {
        if (file_exists($oldname)) {
            return rename($oldname, $newname);
        } else {
            return false;
        }
    }

    // PSR2: методы пишутся в lowerCamelCase
    // Нет проверки аргументов. что произойдет, если $path будет массивом?
    // Не ясно, на кой вам тут возврат bool? Если что-то не так - бросайте исключение.
    public function DeleteFile($path)
    {
        if (file_exists($path)) {
            return unlink($path);
        } else {
            return false;
        }
    }

    // Нет проверки аргументов. что произойдет, если $path будет массивом?
    // Не ясно, на кой вам тут возврат bool? Если что-то не так - бросайте исключение.
    public function getFileContent($path)
    {
        if (file_exists($path)) {
            return file_get_contents($path);
        } else {
            return false;
        }
    }

    // Нет проверки аргументов. что произойдет, если $path будет массивом?
    // Не ясно, на кой вам тут возврат bool? Если что-то не так - бросайте исключение.
    // "size" обычно трактуется как число, а не форматированная строка.
    public function getFileSize($path)
    {
        if (file_exists($path)) {
            // Конструкция - гавно)). Если хотите интовый $size - то сохраняйте в него инт
            (int)$size = filesize($path);
            switch (true) {
                case $size >= 1073741824:
                    $size = round($size / 1073741824 * 100) / 100 . ' GB';
                    break;
                case $size >= 1048576:
                    $size = round($size / 1048576 * 100) / 100 . ' MB';
                    break;
                case $size >= 1024:
                    $size = round($size / 1024 * 100) / 100 . ' KB';
                    break;
                default:
                    $size .= ' Bytes';
                    break;
            }
            return $size;
        } else {
            return false;
        }
    }

    // Нет проверки аргументов. что произойдет, если $path будет массивом?
    // Не ясно, на кой вам тут возврат bool? Если что-то не так - бросайте исключение.
    // Прочитайте http://php.net/manual/ru/class.splfileinfo.php
    public function getFileInfo($path)
    {
        if (file_exists($path)) {
            return pathinfo($path);
        } else {
            return false;
        }
    }

    // Нет проверки аргументов. что произойдет, если $dirname будет массивом?
    // Прочитайте http://php.net/manual/ru/class.directoryiterator.php
    public function getFileList($dirname)
    {
        $dir = array_diff(scandir($dirname), ['.', '..']);
        $files = [];
        // Рекомендую перед управляющими конструкциями делать перевод строки, так читать проще
        foreach ($dir as $value) {
            if (is_file($value)) {
                $files[] = $value;
            }
        }
        // PSR2: перед return перевод строки
        return $files;
    }
// Лишний перевод строки
}

Comments

[Максим]

Спасибо за "Code Review", прочитал Ваши критерии))

// Нет проверки аргументов. что произойдет, если $path будет массивом?

Подскажите такого вида проверка аргументов подойдет?

if(is_array($path)){
    // throw new Exception;
}

[index0h]

if (!is_string($path)) {
throw new \InvalidArgumentException('Argument "$path" must be string');
} elseif (!empty($path)) {
throw new \InvalidArgumentException('Argument "$path" must be not empty');
}

Answer 2

[Юрий]

1) режут глаз нащвания методов с большой буквы.
2) запись в файл во всех случаях проще было сделать file_put_contents, в том числе для дозаписи есть флаг FILE_APPEND
3) по сути класс - набор отдельных функций, можно было бы даже все методы статическими сделать.
4) но в целом смысла от этих функций мало, единственное что идет проверка существования файлов, но в большинстве случаев в реальном приложении return false далеко не всегда устроит, чаще наобоорт может быть исключение лучше кинуть, но от задачи конечно зависит. в итоге у вас все равно часто будет в коде проверка результата на false.

посмотрите лучше Symfony Finder, как пример полезных функций, как пример ООП класса, как пример как можно использовать цепочки вызовов и тд. https://symfony.com/doc/current/components/finder.html

Answer 3

[Rou1997]

Вот не надо делать "для опыта и практики", такие "велосипеды" уж точно никто не одобрит, работайте и напишите что-то полезное для работе, тогда не придется идти на Тостер и спрашивать что стоит изменить, и про "велосипеды" вам никто ничего не скажет, поскольку в открытый доступ вы его и не будете выкладывать, ну фирма DevExpress же так себя не ведет, и никого ни о чем не спрашивает.
У ваших же утилит просто нет цели, никаких полезных новшеств относительно стандартных функций, применять такое ни в каком проекте не будешь.

Answer 4

[dmitriy]

по большому счету это больше походит на статический Helper класс. Я бы предпочел ОПП подход как тут

Answer 5

[Егор]

Кроме того, что уже написали, бросается в глаза повторение кода. Также нет проверок на возможность создавать файл, а также иметь к нему доступ.
Вообще у вас получился т.н. helper класс, когда более менее связанные функции находятся "под одной крышей". К ООП это отношения не имеет.