vitaly_74
@vitaly_74

Можете ли оценить код, и сказать как сделать его лучше?

Добрый день. не могли бы вы сделать небольшой код ревью, и указать мне на ошибки (пользуюсь yii2)/
namespace common\models\billing;

use function class_implements;
use common\models\Billing;
use common\models\billing\interfaces\CashInfoInterface;
use common\models\billing\interfaces\CashInterface;
use common\models\User;
use const false;
use Yii;

/**
 * Created by PhpStorm.
 * User: Vitaly
 * Date: 29.09.2019
 * Time: 11:26
 *
 * Как пользоваться:
 * 1. Обьявляем объект где первое значение id пользователя над которым хотим совершить действие
 * $value - значение которое хотим прибавить или вычесть.
 * $cash = new Cash($userId, $value);
 *
 * 2.1. Если пользователь перевел средства на счет, то
 * $cash->addActual();
 *
 * 2.2. Если администратор прописывает некоторые средства на счет
 * $cash->addAsAdmin();
 *
 * 2.3 Если необходимо вычесть некоторую сумму
 * $cash->withdraw()
 *
 * 2.3.1 Если вычесть не удалось
 * $cash->withdrawn() будет равен false в остальных случаях true;
 */
class Cash implements CashInterface
{
    /**@var Billing */
    private $record;
    private $userId;
    private $oldAdded;
    private $oldActual;
    private $value;

    /**
     * @param int $userId - ID пользователя у которого нужно вычеслить баланс
     * */
    public function __construct($userId, $value)
    {
        $this->userId = $userId;
        $this->value = $value;
        $this->installRecord();
        $this->oldActual = $this->record->actual_cash;
        $this->oldAdded = $this->record->added_cash;
    }

    /*
     * Добавляет средства на счет (просто добавляет) использовать когда добавляется через платежную систему
     * */
    public function addActual()
    {
        $this->record->actual_cash += $this->value;
        $this->save();
        new CashLog([
            "record" => $this->record,
            "value" => $this->value,
            "success" => true,
            "description" => 'Ввод средст с помощью платежной системы был удачным',
            "action" => 'Add actual cash',
            "initiator" => $this->initiator()
        ]);
    }

    /*
     * использовать когда добавляют администраторы
     * */
    public function addAsAdmin()
    {
        $this->record->added_cash += $this->value;
        $this->save();
        new CashLog([
            "record" => $this->record,
            "value" => $this->value,
            "success" => true,
            "description" => 'Ввод средст с помощью админа был удачным',
            "action" => 'Add as admin cash',
            "initiator" => $this->initiator()
        ]);
    }

    /*
     * Списывает n-е кол-во рублей со счета (просто вычитает)
     * */
    public function withdraw()
    {
        $balance = new WithdrawBalance($this->actual(), $this->added());
        $balance->solve($this->value);
        $this->record->added_cash = $balance->addedCash();
        $this->record->actual_cash = $balance->actualCash();
        $this->save();
        if($this->withdrawn()){
            $descLog = 'Удачное снятие денег со счета.';
        }else{
            $descLog = 'Не получилось снять деньги со счета.';
        }

        new CashLog([
            "record" => $this->record,
            "value" => $this->value,
            "success" => $this->withdrawn(),
            "description" => $descLog,
            "action" => 'Withdraw cash',
            "initiator" => $this->initiator()
        ]);
    }

    /*
     * Проверяет проводилось ли вычетание и(или) удачно ли вычетание средств.
     * */
    public function withdrawn()
    {
        if ($this->oldAdded == $this->record->added_cash and $this->oldActual == $this->record->actual_cash) {
            return false;
        }
        return true;
    }

    /**
     * @return float
     * */
    private function actual()
    {
        return $this->record->actual_cash;
    }

    /**
     * @return float
     * */
    private function added()
    {
        return $this->record->added_cash;
    }

    /**
     * устанавливает запись из таблицы billing
     * */
    private function installRecord()
    {
        $rec = Billing::find()->where(['user_id' => $this->userId])->one();
        if ($rec) {
            $this->record = $rec;
        } else {
            $this->record = new Billing();
        }
    }


    private function save($bool = false)
    {
        $this->record->save($bool);
        $this->record->update($bool);
    }

    private function initiator($system = true){
        if ($system){
            return User::find()->where(['login'=>'System'])->one();
        }
        return Yii::$app->user->identity;
    }
}
  • Вопрос задан
  • 177 просмотров
Решения вопроса 1
@EvgeniiR
https://github.com/EvgeniiR
1. Type hints для свойств через phpdoc использовать. Или обновиться до 7.4 и использовать type-hints средствами языка.
2. Type hints для аргументов методов и возвращаемых значений использовать.
3. Написать класс Logger и использовать как зависимость. Желательно чтобы он был адаптером для LoggerInterface(psr3)
4. Методы и переменные нормально называть, половину пхп-доков повыпиливать.
5. Логику(ветвления) либо выносить в зависимости, либо писать юнит-тесты на неё. Учитывая что этот класс ещё и в базу лазит через AR, логики(ветвлений) тут лучше не делать.
6. $value передавать не в конструктор а в конкретные методы где оно нужно, чтобы не пересоздавать экземпляр класса для каждого списания.
7.
private function initiator($system = true){
    if ($system){
        return User::find()->where(['login'=>'System'])->one();
    }
    return Yii::$app->user->identity;
}


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

5. это вы имеете ввиду что в методах new используется?

Да, внедряйте зависимости через Dependency Injection. То есть не создавайте new CashLog а требуйте экземпляр логгера с нужными вам методами в конструктор. Если логгеру нужен юзер а у вас в классе только его идентификатор - желательно бы передавать в логгер идентификатор, а дальше пусть его забота будет, чтобы не нагружать этой логикой ваш класс.
В принципе о связности/тестируемости вам рановато пока думать, в будущем стоит присмотреться к принципам Coupling/Cohesion.
Ответ написан
Комментировать
Пригласить эксперта
Ответы на вопрос 1
@galliard
Самая большая ошибка - пользоваться yii2.
Ответ написан
Ваш ответ на вопрос

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

Похожие вопросы