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

Question

[Богдан Хвалько]

Привет, сделайте 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);
    }
}

Comments

[Дмитрий]

Добрый вечер.
На Ваш предыдущий вопрос я дал ссылку на полезный блог по yii2.
Читайте все статьи и смотрите все видео по yii2 и не только.

[SagePtr]

Комментарии писать на английском, українську мову знають не усі

[FanatPHP]

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

public function __construct($fileName)
    {
        $this->fileName = $fileName;
    }

а все начиная с beginTransaction вынести в отдельный метод

DevMan верни плиз тег РНР, тут вопрос не про yii.

Answer 1

[FanatPHP]

Я остановлюсь на одном аспекте этого кода, но совершенно анекдотическом.

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

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

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

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

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

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

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

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

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

Надо обязательно различать пользовательские и системные сообщения об ошибках:

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

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

Comments

[Богдан Хвалько]

Спасибо большое) Это не оправдание но объяснение. Дело в админке. Полностью ошибку показывать не хочеться, а вот розбить на блоки с ошибками было прикольной идеей, как мне казалось. Т.е. админ увидит ошибку "Unable to open file" и оповестит программиста который полезет в код и выведет всю инфу которая ему нужна. Ведь он будет знать где именно проблема.

[FanatPHP]

Нет, не будет. "Мест" на самом деле много, и бегать по всем замучаешься. Именно поэтому исходное сообщение об ошибке содержит имя файла и номер строки где смотреть

А главное - он не будет знать, какая конкретно ошибка. Файл может не читаться по миллиону причин. При том что конкретная причина указана в сообщении об ошибке. И если ты не будешь лезть в обработку ошибок руками, то Yii запишет полную информацию об ошибке в файл. Где программист её и найдет.

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

[FanatPHP]

Вот я не понимаю эту манию делать двойную работу
сначала писать лишний код, который усложняет работу программисту
потом заставлять программиста "лезть в код и выводить всю инфу" по-новой. ЗАЧЕМ, если он может просто сразу прочесть "всю инфу" если только дать фреймворку записать её?

[FanatPHP]

А что если ошибка "плавающая"? Когда юзер заходит она есть, а когда программист - нету. Что она будет "выводить" тогда?