Оптимизация кода

jakiro · Сообщение **jakiro** » 2016.04.12, 12:26

Господа, хочется подоптимизировать код, хочу избавиться от толстых контроллеров
Есть условно некая форма твоара, с набором разных полей + какой-нибудь файл.
Метод "обновления" товара выглядит так

Код: Выделить всё

  public function actionUpdate($id)
  {
    if(Yii::$app->request->isPost)
    {
      $fd = Yii::$app->request->post('FD');
      $fd['approve'] = isset($fd['approve']) ? 1 : 0;
      $fd['is_new'] = isset($fd['is_new']) ? 1 : 0;
      $fd['is_rec'] = isset($fd['is_rec']) ? 1 : 0;

      $row = Products::findOne($id);
      $row->attributes = $fd;
      $row->update();

      if( $file = UploadedFile::getInstanceByName('image') )
      {
        $filenameT = Useful::genName($file->getBaseName()).$file->getExtension();
        $filenameO = Useful::genName($file->getBaseName()).$file->getExtension();

        $pathT = Yii::getAlias('@uploads/').$filenameT;
        $pathO = Yii::getAlias('@uploads/').$filenameO;

        $file->saveAs($pathT, FALSE);
        $file->saveAs($pathO);

        $img = new SimpleImage($pathT);
        $img->thumbnail(150)->save($pathT);

        $img = new SimpleImage($pathO);
        $img->best_fit(1280, 1280)->save($pathO);

        Products::updateAll(['thumb' => $filenameT, 'image' => $filenameO], ['id' => $id]);
      }
      return $this->redirect(Url::to(['index']));
    }

    return $this->render('edit', [
      'row' => Products::findOne($id),
      'categories' => Categories::getFullTree(),
    ]);
  }

Хочется для начала вынести
загрузку файла, как лучше сделать? Статический метод? Или поведение? Или как?
вот эта часть:

Код: Выделить всё

      if( $file = UploadedFile::getInstanceByName('image') )
      {
        $filenameT = Useful::genName($file->getBaseName()).$file->getExtension();
        $filenameO = Useful::genName($file->getBaseName()).$file->getExtension();

        $pathT = Yii::getAlias('@uploads/').$filenameT;
        $pathO = Yii::getAlias('@uploads/').$filenameO;

        $file->saveAs($pathT, FALSE);
        $file->saveAs($pathO);

        $img = new SimpleImage($pathT);
        $img->thumbnail(150)->save($pathT);

        $img = new SimpleImage($pathO);
        $img->best_fit(1280, 1280)->save($pathO);

        Products::updateAll(['thumb' => $filenameT, 'image' => $filenameO], ['id' => $id]);
      }

maleks · Сообщение **maleks** » 2016.04.12, 13:07

1) Вот это:
$row = Products::findOne($id);
надо один раз делать в самом начале экшена и обрабатывать если не найден.
2) А на экшене создания товара вы же тоже грузите файл, когда id еще не известен? Что там дубль всей этой файловой логики?
3) Загрузку и сохранение файла можно перенести в модель и в контроллере останется такого плана:

Код: Выделить всё

if ($row instanceof SomeFileManageInterface) {
   $row->loadFiles();
   //...
   $row->saveFiles();
}

4) Плюс с валидацией у вас тут что то неясно, та же картинка тоже должна валидироваться.

jakiro · Сообщение **jakiro** » 2016.04.12, 13:27

maleks писал(а):1) Вот это:
$row = Products::findOne($id);
надо один раз делать в самом начале экшена и обрабатывать если не найден.
2) А на экшене создания товара вы же тоже грузите файл, когда id еще не известен? Что там дубль всей этой файловой логики?
3) Загрузку и сохранение файла можно перенести в модель и в контроллере останется такого плана:
Код: Выделить всё
if ($row instanceof SomeFileManageInterface) {
   $row->loadFiles();
   //...
   $row->saveFiles();
}
4) Плюс с валидацией у вас тут что то неясно, та же картинка тоже должна валидироваться.

1. Если я так сделаю, то обновленные данные увижу только после перезагрузки страницы. В данном случае редирект, согласен, но часто приходится обновлять записи не уходя со страницы. Поэтому взял уже за правило.
2. Почему дубль? В порядке очереди все. Форма пришла, с файлом. Обновил запись в БД, загрузил файл
3. Количество файлов может быть переменно, как и пережатие картинки (если это картинка вообще) SimpleImage class

maleks · Сообщение **maleks** » 2016.04.12, 13:44

1. Только этот ваш метод не учитывает момент когда модель не проходит валидацию.
2. Код экшена создания покажите?
3. Ну это уже несущественные детали, обработать можно как угодно.

jakiro · Сообщение **jakiro** » 2016.04.12, 14:27

maleks писал(а):1. Только этот ваш метод не учитывает момент когда модель не проходит валидацию.
2. Код экшена создания покажите?
3. Ну это уже несущественные детали, обработать можно как угодно.

Код: Выделить всё

  public function actionAdd()
  {
    if(Yii::$app->request->isPost)
    {
      $fd = Yii::$app->request->post('FD');
      $fd['approve'] = isset($fd['approve']) ? 1 : 0;
      $fd['is_new'] = isset($fd['is_new']) ? 1 : 0;
      $fd['is_rec'] = isset($fd['is_rec']) ? 1 : 0;

      $m = new Products();
      $m->attributes = $fd;
      $m->save();

      if( $file = UploadedFile::getInstanceByName('image') )
      {
        $filenameT = Useful::genName($file->getBaseName()).$file->getExtension();
        $filenameO = Useful::genName($file->getBaseName()).$file->getExtension();

        $pathT = Yii::getAlias('@uploads/').$filenameT;
        $pathO = Yii::getAlias('@uploads/').$filenameO;

        $file->saveAs($pathT, FALSE);
        $file->saveAs($pathO);

        $img = new SimpleImage($pathT);
        $img->thumbnail(150)->save($pathT);

        $img = new SimpleImage($pathO);
        $img->best_fit(1280, 1280)->save($pathO);

        Products::updateAll(['thumb' => $filenameT, 'image' => $filenameO], ['id' => $m->id]);
        return $this->redirect(Url::to(['index']));
      }
    }

    return $this->render('add', [
      'categories' => Categories::getFullTree()
    ]);
  }

zelenin · Сообщение **zelenin** » 2016.04.12, 14:32

кстати, думаю, $fd['is_new'] = isset($fd['is_new']) ? 1 : 0; можно заменить на $fd['is_new'] = (int)isset($fd['is_new']);
но в целом true/false более говорящая конструкция чем 1/0. Непонятно зачем булево значение приводить к целому?

maleks · Сообщение **maleks** » 2016.04.12, 14:54

ну я про этот дубль кода и говорю, одно и то же в двух экшенах.
Можно перенести в метод модели

Код: Выделить всё

if( $file = UploadedFile::getInstanceByName('image') )      {
    $m->adjustFiles($file);
    return $this->redirect(Url::to(['index']));
}

jakiro · Сообщение **jakiro** » 2016.04.12, 17:27

zelenin писал(а):кстати, думаю, $fd['is_new'] = isset($fd['is_new']) ? 1 : 0; можно заменить на $fd['is_new'] = (int)isset($fd['is_new']);
но в целом true/false более говорящая конструкция чем 1/0. Непонятно зачем булево значение приводить к целому?

Потому что если я не делаю проверку (а это флаг), то по HTTP не передается поле, а значит в массиве не будет ключа, а при любом обращении к нему будет Undefined Index Error
Но ваш мтод тоже интересен, я это писал очень давно, рефакторю по-немногу)

jakiro · Сообщение **jakiro** » 2016.04.12, 17:29

maleks писал(а):ну я про этот дубль кода и говорю, одно и то же в двух экшенах.
Можно перенести в метод модели
Код: Выделить всё
if( $file = UploadedFile::getInstanceByName('image') )      {
    $m->adjustFiles($file);
    return $this->redirect(Url::to(['index']));
} 

Вот я и хочу это как то вынести всю логику, вот только пока не определился как

zelenin · Сообщение **zelenin** » 2016.04.12, 17:37

jakiro писал(а):
zelenin писал(а):кстати, думаю, $fd['is_new'] = isset($fd['is_new']) ? 1 : 0; можно заменить на $fd['is_new'] = (int)isset($fd['is_new']);
но в целом true/false более говорящая конструкция чем 1/0. Непонятно зачем булево значение приводить к целому?
Потому что если я не делаю проверку (а это флаг), то по HTTP не передается поле, а значит в массиве не будет ключа, а при любом обращении к нему будет Undefined Index Error

я вижу, что вы этот массив присваиваете attributes. никакого http.

jakiro · Сообщение **jakiro** » 2016.04.12, 17:57

zelenin писал(а):
jakiro писал(а):
zelenin писал(а):кстати, думаю, $fd['is_new'] = isset($fd['is_new']) ? 1 : 0; можно заменить на $fd['is_new'] = (int)isset($fd['is_new']);
но в целом true/false более говорящая конструкция чем 1/0. Непонятно зачем булево значение приводить к целому?
Потому что если я не делаю проверку (а это флаг), то по HTTP не передается поле, а значит в массиве не будет ключа, а при любом обращении к нему будет Undefined Index Error
я вижу, что вы этот массив присваиваете attributes. никакого http.

В данном случае да, но когда происхоидт обращение например

Код: Выделить всё

if($fd['is_new'])

А флаг не был поставлен, то вываливается Critical Error
Хотя странно, в самом php обращение к несущ. индексу это Notice вроде, т.е. некритическая ошибка (да и по дефолту отключена видимость таких сообщений), а вот yii почему то впадает в лютый ужас и дропает все приложение

zelenin · Сообщение **zelenin** » 2016.04.12, 18:12

jakiro писал(а):А флаг не был поставлен

а почему он не поставлен? как это связано с true/false?

S c · Сообщение **S c** » 2016.04.12, 18:56

jakiro писал(а):
maleks писал(а):ну я про этот дубль кода и говорю, одно и то же в двух экшенах.
Можно перенести в метод модели
Код: Выделить всё
if( $file = UploadedFile::getInstanceByName('image') )      {
    $m->adjustFiles($file);
    return $this->redirect(Url::to(['index']));
}
Вот я и хочу это как то вынести всю логику, вот только пока не определился как

А вы пробуйте по разному, не понравится результат (увидите что не устраивает) - еще по другому попробуйте, и так со временем обретете понимание. Сейчас вы не на том уровне, чтоб сходу видеть "идеальное" решение.

jakiro · Сообщение **jakiro** » 2016.04.12, 19:48

zelenin писал(а):
jakiro писал(а):А флаг не был поставлен
а почему он не поставлен? как это связано с true/false?

потому что это checkbox, и если он не отмечен, поле не будет оптравлено на сервер

jakiro · Сообщение **jakiro** » 2016.04.12, 19:50

S c писал(а):
jakiro писал(а):
maleks писал(а):ну я про этот дубль кода и говорю, одно и то же в двух экшенах.
Можно перенести в метод модели
Код: Выделить всё
if( $file = UploadedFile::getInstanceByName('image') )      {
    $m->adjustFiles($file);
    return $this->redirect(Url::to(['index']));
}
Вот я и хочу это как то вынести всю логику, вот только пока не определился как
А вы пробуйте по разному, не понравится результат (увидите что не устраивает) - еще по другому попробуйте, и так со временем обретете понимание. Сейчас вы не на том уровне, чтоб сходу видеть "идеальное" решение.

я пока только понимаю, что на вход хочу отдать имена полей type=file и для каждого объект SimpleImage для пережатия, если надо

zelenin · Сообщение **zelenin** » 2016.04.12, 19:55

jakiro писал(а):
zelenin писал(а):
jakiro писал(а):А флаг не был поставлен
а почему он не поставлен? как это связано с true/false?
потому что это checkbox, и если он не отмечен, поле не будет оптравлено на сервер

а, ок. в yii-шной реализации для чекбокса его предваряет скрытый инпут, который всегда оптравляется https://github.com/yiisoft/yii2/blob/ma ... l.php#L739
то есть вы столкнетесь с такой проблемой, если чекбокс рисуете руками.

jakiro · Сообщение **jakiro** » 2016.04.12, 21:03

zelenin писал(а):
jakiro писал(а):
zelenin писал(а): а почему он не поставлен? как это связано с true/false?
потому что это checkbox, и если он не отмечен, поле не будет оптравлено на сервер
а, ок. в yii-шной реализации для чекбокса его предваряет скрытый инпут, который всегда оптравляется https://github.com/yiisoft/yii2/blob/ma ... l.php#L739
то есть вы столкнетесь с такой проблемой, если чекбокс рисуете руками.

Да, практически все формы рисуются руками, потому что слишком много всяких стилей и часто меняются формы (весртка)

zelenin · Сообщение **zelenin** » 2016.04.12, 21:07

jakiro писал(а):
zelenin писал(а):
jakiro писал(а): потому что это checkbox, и если он не отмечен, поле не будет оптравлено на сервер
а, ок. в yii-шной реализации для чекбокса его предваряет скрытый инпут, который всегда оптравляется https://github.com/yiisoft/yii2/blob/ma ... l.php#L739
то есть вы столкнетесь с такой проблемой, если чекбокс рисуете руками.
Да, практически все формы рисуются руками, потому что слишком много всяких стилей и часто меняются формы (весртка)

это не важно - я подсказал вам правильный костыль, т.к. все поля что-то должны отправлять. А это в свою очередь упростит обработку пришедших в экшн данных.
Ну и использование Html хелпера не ограничивает в плане верстки. ActiveForm - да, сложен в настройке.

Nerf · Сообщение **Nerf** » 2016.04.13, 02:11

Код: Выделить всё

$filenameT = Useful::genName($file->getBaseName()).$file->getExtension();
$filenameO = Useful::genName($file->getBaseName()).$file->getExtension();

$pathT = Yii::getAlias('@uploads/').$filenameT;
$pathO = Yii::getAlias('@uploads/').$filenameO;

$file->saveAs($pathT, FALSE);
$file->saveAs($pathO);

$pathT != $pathO?
Зачем сохранять файлы с последующим переписыванием?

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

Почему? Вы получите те данные, которые загрузили. load() не пробовали?

maleks · Сообщение **maleks** » 2016.04.13, 07:14

Nerf писал(а): Почему? Вы получите те данные, которые загрузили. load() не пробовали?

Он не использует формы с yii инпутами, поэтому даже ничего не именовано.

Код: Выделить всё

      $fd = Yii::$app->request->post('FD');
      $fd['approve'] = isset($fd['approve']) ? 1 : 0;
      $fd['is_new'] = isset($fd['is_new']) ? 1 : 0;
      $fd['is_rec'] = isset($fd['is_rec']) ? 1 : 0;

      $row = Products::findOne($id);
      $row->attributes = $fd;

Yii Framework

Оптимизация кода

Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода

Re: Оптимизация кода