Как можно отрефакторить приведенную php функцию?

Question

[Андрей Соколовский]

Здравствуйте.

Помогите, пожалуйста, с направлением мысли.

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

Если я сделаю отдельно функцию для Start, и отдельно для Finish, тогда у меня все равно будет дублироваться код получения данных из БД (или получение вынести в третью функцию :) ) и сохранение, которое одинаковое и для Start и для Finish. Ни один вариант не выглядит красивым.

public static function setUnits($type, $packageId, $clientSource, $clientDestination = NULL){

        $package = Package::find($packageId);

        if ($type == 'Start') {
            $package->SourceUnitsStartJob = $clientSource->units + $clientSource->cost;
            if ($clientDestination != NULL) {
                $package->DestinationUnitsStartJob = $clientDestination->units + $clientDestination->cost;
            }
        }

        if ($type == 'Finish') {
            $package->SourceUnitsFinishJob = $clientSource->units;
            if ($clientDestination != NULL) {
                $package->DestinationUnitsFinishJob = $clientDestination->units;
            }
        }

        $package->SourceUnitsLimit = $clientSource->unitsLimit;
        if ($clientDestination != NULL) {
            $package->DestinationUnitsLimit = $clientDestination->unitsLimit;
        }

        $package->save();
    }

Буду благодарен за ответ, а также за ключевые слова, которые помогут найти с какими материалами я могу еще ознакомиться, которые позволят лучше писать код :)

Comments

[zorca]

Оберните вашу функцию в тег кода что-ли. Как ЭТО читать то?

[Андрей Соколовский]

zorca, поправили.

[ThunderCat]

public static function setUnits

Это хэлпер? Или модель? Если хелпер, то подобная лапша местами неизбежна. Если это модель - во превых какого там делает статический метод, во вторых, при наличии инициализации объекта(через конструктор или функции инициализации) можно(и нужно) разбить код на более простые универсальные методы, и уже их вызывать, например $this->start(); $this->finish(); и в этих методах активно оперировать внутренними свойствами объекта($this->clientDestination и тд).
Почему, собсно, у вас "старт" и "финиш" еще и с большой буквы? Есть такие классы? Если нет - это вносит некоторую долю неопределенности в код, лучше убрать.
По обрывку кода судить сложно, но в целом смысл такой - статика плохо, маленькие / короткие методы в моделях хорошо.

Answer 1

[smple]

я так понимаю package это active record

Далее у тебя есть класс с методами (костылями вроде Util или Core) в котором есть setUnits, это пример процедурного кода, когда данные лежат отдельно (в package), а работа с этими данным в другом месте.

Как бы я улучшил этот код.
1. Я почти уверен что из package можно получить clientSource и clientDestination это похоже тоже объекты AR, значит я бы добавил связи между моделями
2. Я бы удалил этот метод setUnits
3. Поля SourceUnitsFinishJob и DestinationUnitsFinishJob и DestinationUnitsStartJob и SourceUnitsStartJob я бы сделал чтобы они расчитывались на основе связей с clientSource и clientDestination, если твоя AR этого не позволяет то получение значения этих полей я бы сделал методом

Тогда получение некоторых свойств выглядело было вот так

$package = Package::find($packageId);
$package->clientSource->units; // SourceUnitsFinishJob
// и тд
// Чтобы получить поля DestinationUnitsStartJob которые расчитываются на основе двух параметров я бы сделал динамический атрибут
$package->destinationUnitsStartJob;

В laravel этот атрибут можно создать следующим образом в модели Package

public function getDestinationUnitsStartJobAttribute()
{
   return $this->clientDestination->units + $this->clientDestination->cost;
}

Где clientDestionation это связь
соответсвенно работу работать с package можно было бы с помощью стандартных методов.