2

Вопросы по оформлению кода...

Есть такой метод:

/**
 * Activate module from ZIP archive
 * @param $archiveFilePath
 * @return bool
 */
public function activate($archiveFilePath)
{
    if( file_exists($archiveFilePath) ) {
        $moduleDirectory = $this->extractArchive($archiveFilePath);
        if( $moduleDirectory ) {
            $moduleConfig = $this->getConfig($moduleDirectory);
            if( $moduleConfig ) {

            } else {
                \Yii::$app->session->setFlash(self::ERRORS_KEY, self::ERROR_INSTALL_CONFIG_NOT_EXIST);
                return false;
            }
        } else {
            \Yii::$app->session->setFlash(self::ERRORS_KEY, self::ERROR_ARCHIVE_EXTRACT);
            return false;
        }
    } else {
        \Yii::$app->session->setFlash(self::ERRORS_KEY, self::ERROR_ARCHIVE_NOT_EXIST);
        return false;
    }
}

Видим множество if. Мне постоянно необходимо проверять значения, которые возвращают другие методы класса и выводить ошибки, если что-то не так.

Вопрос 1. Код получается ужасный (это я только начал, там много таких проверок должно еще быть). Как в таких случаях поступать правильно?

Вопрос 2. Например, у меня есть проверка if( $moduleDirectory ). В $moduleDirectory может быть string или false. Стоит ли так писать? Или лучше делать так: if( $moduleDirectory !== false )?

2 ответа 2

3

Сделать классы исключений:

class ArchiveNotFoundException extends Exception;
class ArchiveExtractException extends Exception;
class ConfigNotFoundException extends Exception;

Функция, бросающаяся исключениями:

public function activate($archiveFilePath) {
    if (!file_exists($archiveFilePath)) 
        throw new ArchiveNotFoundException("Архив не найден");

    $moduleDirectory = $this->extractArchive($archiveFilePath);
    if (!$moduleDirectory) 
        throw new ArchiveExtractException("Ошибка распаковки архива");

    $moduleConfig = $this->getConfig($moduleDirectory);
    if (!$moduleConfig) 
        throw new ConfigNotFoundException("Конфигурация не найдена");

    // штатное продолжение функции
}

Применение функции, бросающейся исключениями:

try {
    // начало
    activate($archiveFilePath);
    // штатное продолжение программы
} catch (ArchiveNotFoundException $e) {
    \Yii::$app->session->setFlash(self::ERRORS_KEY, self::ERROR_ARCHIVE_NOT_EXIST);
} catch (ArchiveExtractException $e) {
    \Yii::$app->session->setFlash(self::ERRORS_KEY, self::ERROR_ARCHIVE_EXTRACT);
} catch (ConfigNotFoundException $e) {
    \Yii::$app->session->setFlash(self::ERRORS_KEY, self::ERROR_INSTALL_CONFIG_NOT_EXIST);
}

Кроме того функция теперь не привязана к YII (по крайней мере в части ошибок), сама не занимается их обработкой. Таким образом появляется возможность повторного использования кода в самых разных ситуациях и полная свобода действий по обработке ошибок.

3

Вопрос 1. Код получается ужасный (это я только начал, там много таких проверок должно еще быть). Как в таких случаях поступать правильно?

Придерживаться правила: сначала обрабатывать простые случаи, а потом, ниже по коду - сложные.

Вопрос 2. Например, у меня есть проверка if( $moduleDirectory ). В $moduleDirectory может быть string или false. Стоит ли так писать? Или лучше делать так: if( $moduleDirectory !== false )?

Всегда, конечно, лучше чтобы переменная имела всего 1 тип данных и все. Но если уж так, то: если строка не может быть пустой(пустая строка преобразуется в false) - то так вполне нормально, если может - проверять уже на равенство/не равенство.

Я бы код переписал так:

public function activate($archiveFilePath) {
    if( !file_exists($archiveFilePath) ) {
        \Yii::$app->session->setFlash(self::ERRORS_KEY, self::ERROR_ARCHIVE_NOT_EXIST);
        return false;
    }

    $moduleDirectory = $this->extractArchive($archiveFilePath);
    if( !$moduleDirectory ) {
        \Yii::$app->session->setFlash(self::ERRORS_KEY, self::ERROR_ARCHIVE_EXTRACT);
        return false;
    }

    $moduleConfig = $this->getConfig($moduleDirectory);
    if( !$moduleConfig ) {
        \Yii::$app->session->setFlash(self::ERRORS_KEY, self::ERROR_INSTALL_CONFIG_NOT_EXIST);
        return false;
    }

    // тут основной код - тот что внутри if( $moduleConfig ) ...
}
2
  • "Всегда, конечно, лучше чтобы переменная имела всего 1 тип данных и все" У меня метод extractArchive распаковывает zip-архив. Что возвращать, если распаковка не удалась? Пустую строку лучше? Не false?
    – LostDok
    30 сен 2016 в 22:46
  • 1
    @LostDok throw new Exception("Распаковка не удалась :(").
    – Sergey
    1 окт 2016 в 1:35

Ваш ответ

Нажимая на кнопку «Отправить ответ», вы соглашаетесь с нашими условиями использования и подтверждаете, что прочитали и поняли наши политику конфиденциальности и нормы поведения.

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.