How can I improve my php code?

Question

PHP

Hello, do a code review who has more experience. Any criticism is welcome. Plus, you can throw links to articles and books that will help me find problems in my code)

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

Answer 1

I will focus on one aspect of this code, but it is completely anecdotal.
It seems that almost all pahashniks are descendants of smugglers. Well, or sack makers who buy a bag in China for three kopecks, are taken to Khabarovsk, hang up the price tag "Lui Viton" and sell for five. And even if they buy a Real Louis Vuitton, they will still rip off the tag and scribble another one with a ballpoint pen. And it doesn't fit in my head at all.
For some reason, they consider their main task to be to take an error message that ALREADY exists in PHP - relevant, informative, detailed, useful, containing all the information that is needed to fix the problem ... and cover it up with a ballpoint pen, giving out meaningless scribbles instead who nobodyNot needed.
That is, they are simultaneously doing meaningless work - painstakingly creating an error message when it already exists, and at the same time also doing sabotage - removing all useful information that could help fix the problem.
This code of yours looks like someone forced you to learn the rule - "any block of code must be wrapped in a try catch!" And it comes to a joke - if you really do n’t know what to do inside catch ... then you stupidly do throw. Why then do try at all? "And Schaub Bulo!".
Here take the very first check,! file_exists($fileName). If you throw it away, then when you try to open a non-existent file, you will receive a detailed message - WHAT file you are trying to open, and WHY specifically it did not work. That is, the error message ALREADY exists, and is a million times more informative. And what's the point of sticking file_exists everywhere is a MYSTERY. A riddle cleaner than the secrets of the Egyptian pyramids. There at least it is clear why they were fenced.
The same applies to absolutely all other "checks" for errors in this code. A special code is written, the whole task of which is to take the original, informative and useful error message, erase it, and give out unnecessary rubbish to anyone.
The funny thing is that all the writers who diligently display "Error saving language" have absolutely no idea - for whom they are doing this. For the user? Even if he sees this message (for which he needs to beat his hands separately), then he already knows that the problem with preserving the language is what he did. Not to mention 'Unable to open file'. What fillet? Why unable? What to do with it?
For a programmer? Instead of a detailed system error message, read these riddles, seriously?
In general, throw out all these meaningless checks. You are working with a framework, not riveting on your knee. the framework was not written by fools, it will perfectly handle all the errors itself.
And for God's sake, forget this stupid rule to wrap any block of code in a tri-catch. Instead, remember something else:

It is imperative to distinguish between user and system error messages:

• give custom ones to the user only when he can do something. For example: check the file for validity and give the user a message that he sent not CSV, but some kind of nonsense. But this is NOT done by throwing a standard exception. This is done either at the stage of validating incoming data, or by throwing a special exception, which can then be caught in the controller and displayed to the user.
  
• do not touch the system ones with your hands. Let the framework handle them. At the development stage, it will display a similar and beautiful error message with a bunch of information, and on the production server it will accurately write the error to the log and show the user a standard "something went wrong" page.
  

Don't take the harshness of this text personally - it's just a real problem with p-hap writers in general.