@quitting
Junior php

Как я могу улучшить свой php код?

Привет, сделайте code review у кого опыта больше. Любая критика приветствуется. Плюс, можете кидать ссылки на статьи и книги которые помогут мне самому находить у себя в коде проблемы)

<?php

namespace common\models\parse\localization;

use Yii;
use common\models\Language;
use common\models\Localization;
use common\models\LocalizationKey;
use yii\db\Exception;
use yii\helpers\ArrayHelper;
use ZipStream\Exception\FileNotFoundException;


class ImportCsv
{
    /*
    *   Кількість строк до строки мови.
    *   key,status,category,Ukrainian,Russian,English
    */
    const HEADER_PER_LANGUAGE = 4;

    private $fileName;

    private $desc;

    public function __construct($fileName)
    {
        if (! file_exists($fileName)) {
            throw new FileNotFoundException('File not found');
        }

        $this->fileName = $fileName;

        if (! $this->fileOpen()) {
            throw new \yii\base\Exception('Unable to open file');
        }

        $transaction = Yii::$app->db->beginTransaction();
        try {
            $this->clearTables();
            $this->toImport();

            $transaction->commit();
        }
        catch (Exception $e) {
            $transaction->rollBack();
            throw $e;
        }
    }


    private function fileOpen() {
        $this->desc = fopen($this->fileName, 'r');
        return (boolean)$this->desc;
    }


    private function clearTables()
    {
        try {
            Localization::deleteAll();
            LocalizationKey::deleteAll();
            Language::deleteAll();
        }
        catch(\Exception $e) {
            throw new Exception("Error deleting old localization");
        }
    }


    private function toImport()
    {
        try {
            $header = fgetcsv($this->desc, 10000, ",");
            $languagesIds = $this->saveLanguage(array_slice($header, self::HEADER_PER_LANGUAGE));

            while (($data = fgetcsv($this->desc, 10000, ",")) !== false) {
                $id = $this->saveLocalizationKey($data);
                $this->saveTranslation($id, $data, $languagesIds);
            }
        }
        catch (\Exception $e) {
            throw $e;
        }
    }


    public function saveLanguage(array $languages)
    {
        return ArrayHelper::getColumn(
                array_map(function ($element) {
                    $model = new Language();
                    $model->name = stristr($element, ':', true);
                    $model->short_name = substr(strrchr($element, ":"), 1);
                    $model->description = "Language ". $element;

                    try {
                        $model->save();
                    }
                    catch (\Exception $e) {
                        throw new Exception("Error saving language");
                    }

                    return $model;
                }, $languages),
            'id'
        );
    }


    private function saveLocalizationKey($data)
    {
        $localization = new LocalizationKey();
        $localization->key = $data['0'];
        $localization->status = $data['1'];
        $localization->category = $data['2'];
        $localization->status_translated = $data['3'];

        try {
            $localization->save();
        }
        catch (\Exception $e) {
            throw new Exception("Error saving localization key.");
        }

        return $localization->id;
    }


    private function saveTranslation($keyId, $data, $languageIds)
    {
        $countLanguages = count($languageIds);
        for ($i = 0; $i < $countLanguages; $i++) {
            $locKey = new Localization();
            $locKey->id_key = $keyId;
            $locKey->language_id = $languageIds[$i];
            $locKey->name = $data[$i  + self::HEADER_PER_LANGUAGE];

            try {
                $locKey->save();
            }
            catch (\Exception $e) {
                throw new Exception("Error importing translation text.");
            }
        }

        return true;
    }


    public function __destruct()
    {
        fclose($this->desc);
    }
}
  • Вопрос задан
  • 185 просмотров
Пригласить эксперта
Ответы на вопрос 1
FanatPHP
@FanatPHP
Помогаю и новичкам, и старичкам
Я остановлюсь на одном аспекте этого кода, но совершенно анекдотическом.

Такое ощущение что все пэхапешники практически поголовно - потомки контрабандистов. Ну или мешочников, которые в китае покупают сумку за три копейки, везут в Хабаровск, вешают ценник "Луй Витон" и продают за пять. И даже если купят Настоящий Louis Vuitton, то все равно сдерут бирку и нацарапают шариковой ручкой другую. И это совершенно не укладывается в голове.

Главной своей задачей он почему-то считают взять сообщение об ошибке, которое УЖЕ есть в РНР - актуальное, информативное, подробное, полезное, содержащее всю информацию, которая нужна для исправления проблемы... и замазать его шариковой ручкой, выдав вместо него бессмысленные каракули, которые никому не нужны.

То есть они одновременно совершают бессмысленную работу - кропотливо создают сообщение об ошибке, когда оно и так уже есть, и при этом ещё и занимаются вредительством - удаляя всю полезную информацию которая могла бы помочь в исправлении проблемы.

Этот твой код выглядит так, как будто тебя кто-то заставил выучить правило - "любой блок кода надо завернуть в try catch!" Причём доходит до анекдота - если уж совсем не знаешь, что делать внутри catch... то тупо делаешь throw. Зачем тогда вообще делать try? "А шоб було!".

Вот взять самую первую проверку, ! file_exists($fileName). Если её выкинуть, то при попытке открыть несуществующий файл ты получишь подробное сообщение - КАКОЙ файл пытаешься открыть, и ПОЧЕМУ конкретно это не получилось. То есть сообщение об ошибке УЖЕ есть, и в миллион раз информативнее. И какой смысл повсюду втыкать file_exists - ЗАГАДКА. Загадка почище тайны египетских пирамид. Там хотя бы понятно, зачем их городили.

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

Самое забавное, что все писатели, которые старательно выводят "Error saving language", совершенно не представляют себе - для кого они это делают. Для пользователя? Если он даже и увидит это сообщение (за что надо отдельно по рукам бить), то он и так знает, что проблема с сохранением языка - он этим и занимался. Не говоря уже про 'Unable to open file'. Какое филе? Почему унабле? Что с этим делать?
Для программиста? Вместо подробного системного сообщения об ошибке читать эти загадки, серьёзно?

В общем выкидывай все эти бессмысленные проверки. Ты работаешь с фреймворком, а не клепаешь на коленке. фреймворк писали не дураки, он прекрасно обработает все ошибки сам.

И забудь ради бога это дурацкое правило, оборачивать любой блок кода в трай кетч. Вместо него запомни другое:

Надо обязательно различать пользовательские и системные сообщения об ошибках:
  • пользовательские выдавать пользователю, только тогда когда он может что-то сделать. Например: проверить файл на валидность и выдать пользователю сообщение что он прислал не CSV, а какую-то ерунду. Но это НЕ делается путем выброса стандартного исключения. Это делается либо на этапе валидации входящих данных, либо - выбросом специального исключения, которое потом можно поймать в контроллере и вывести пользователю.
  • системные вообще никак не трогать руками. А дать фреймворку самому их обработать. На этапе разработки он выведет подобное и красивое сообщение об ошибке с кучей информации, а на боевом сервере аккуратно запишет ошибку в лог и покажет пользователю стандартную страничку "что-то пошло не так".



Не принимай резкость этого текста персонально - просто это реально проблема с писателями на пэхапе в целом.
Ответ написан
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Войти через центр авторизации
Похожие вопросы