Questions about the design of the code ...

There is such a method:

/** * 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; } }

We see a set of if . I constantly need to check the values ​​that other class methods return and display errors if something is wrong.

Question 1 . The code turns out terrible (I just started it, there should be many such checks). How to do the right thing in such cases?

Question 2 . For example, I have an if( $moduleDirectory ) . The $moduleDirectory can be string or false . Is it worth writing? Or is it better to do this: if( $moduleDirectory !== false ) ?

    2 answers 2

    Make exception classes:

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

    Exception-throwing function:

     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("Конфигурация не найдена"); // штатное продолжение функции }

    The use of a function thrown by exceptions:

     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); }

    In addition, the function is no longer bound to YII (at least in part of the errors), it does not process them. Thus, it is possible to reuse the code in a variety of situations and complete freedom of action for error handling.

      Question 1. The code is terrible (I just started it, there should be many such checks). How to do the right thing in such cases?

      Adhere to the rule: first handle simple cases, and then, lower in code - complex ones.

      Question 2. For example, I have an if ($ moduleDirectory) check. The $ moduleDirectory can be string or false. Is it worth writing? Or is it better to do this: if ($ moduleDirectory! == false)?

      Of course, it is always better that a variable has only 1 data type and that’s it. But if so, then: if the string cannot be empty (the empty string is converted to false) - then it is quite normal, if it can - check for equality / non-equality.

      I would rewrite the code like this:

       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 ) ... }
      • "It is always better, of course, for the variable to have only 1 data type and everything." I have the extractArchive method extractArchive unpacks the zip archive. What to return if unpacking failed? Is the blank line better? Not false? - LostDok
      • one
        @LostDok throw new Exception("Распаковка не удалась :(") . - Sergey