Поругайте код, мне будет полезно

Господа, буду рад любым предложениям по улучшению этого написанного мною кода.
Поскольку я дилетант, то наверняка делаю это топорно и не изящно.
Поставил себе цель написать сниппет, который будет выводить на экран значения MIGX TV для всех ресурсов, которые будут дочерними к ресурсу родителю, чей id будет задаваться как параметр сниппета.
<?php
// получаем id родительского каталога как параметр
$p=$parent;
// выборка всех ресурсов, у кого родитель имеет переданный id 
// и формируем массив с id ресурсов-детей.
$g=$modx->getCollection('modResource',array('parent'=>$p));
foreach ($g as $g1){
    $where[]=$g1->get('id');
}
// проходим по всем элементам нашего массива, делая выборку одного объекта
// значение contentid которого совпадает с id из массива
// поскольку я задался целью получить только MIGX то прописываю id этого объекта
foreach ($where as $nowhere){
    $result=$modx->getObject('modTemplateVarResource',array('contentid'=>$nowhere,'tmplvarid'=>1));
// добавил сюда проверку на существование $result поскольку не все ресурсы могут иметь заполненные
// MIGX поля
    if ($result){
// $res становится строкой, в которой лежит JSON
    $res=$result->get('value');
//$r становится массивом
    $r=$modx->fromJSON($res);
// перебираем все елементы массива, чтобы добраться до содержимого. 
// У меня поле имеет название text
// в результате получаем вывод на экран перечня значений всех MIGX для всех ресурсов
// дочерних к тому, чей id мы передали в сниппет
    foreach ($r as $r1){ echo $r1['text']."
";}
 } //end if
} // end foreach
Спасибо.
Александр Мельник
25 февраля 2017, 10:25
modx.pro
2
1 874
0

Комментарии: 8

Наумов Алексей
25 февраля 2017, 13:54
0
1. Код должен быть читаемым и понятным, пусть и длиннее.

$p=$parent;
$g as $g1
$where as $nowhere
$result => $res => $r
Не надо так делать.

И вообще, если у вас есть переменная $result, то должно быть return $result, что очень логично)

2. В сниппетах не нужно делать echo, только return.
    Александр Мельник
    25 февраля 2017, 14:07
    0
    Спасибо.
    Насчет названий переменных, Вы правы.
    Насчет echo — тоже, хотя я ставил целью именно вывести на экран.
    Кстати, раз уж мы коснулись echo и return — Вы замечали, что сниппет modx печатает на экран то, что вызывается как return?
    В нем return ведет себя не как в PHP. Поэтому применение return в моем конкретном случае привело бы к показу первого результата.
    Если задаваться целью сделать вывод в чанк, то наверное, правильно было бы в цикле формировать строковую переменную с вызовом getChunk() а уже в конце делать return этой переменной.
      Андрей
      25 февраля 2017, 15:25
      0
      Вместо
      echo $r1['text']

      лучше создать переменную $output и выводить потом ее
      foreach ($r as $r1) {
          $output .= $r1['text'];
      }
      
      //и в конце сниппета
      return $output
Іван Клімчук
26 февраля 2017, 08:28
+3
Про названия переменных уже сказали, это ужасно. Никогда так не делайте, называйте переменные правильно и не придется писать тонны комментариев к коду.
Второ момент, зачем тут такие странные манипуляции?

$g=$modx->getCollection('modResource',array('parent'=>$p));
foreach ($g as $g1){
    $where[]=$g1->get('id');
}
Лучше использовать getIterator, на большом количестве ресурсов памянть не закончится, это раз. Два раза вы делаете проход по массиву, абсолютно бесполезно. Вы же и так перебираете ресурсы, почему не сделать вот так?

foreach ($modx->getIterator('modResource', array('parent' => $parent)) as $resource) {

    $result = $modx->getObject('modTemplateVarResource', array('contentid' => $resource->get('id'), 'tmplvarid' => 1));

    ...
}
Большая вложенность кода в {} ухудшает читабельность кода, поэтому идеальный код — это прямой без ветвлений или стараться их избегать где возможно. Например, вместо проверки, есть ли значение в переменной и выполнения кода внутри if, можно проверить, что если значение пустое, то пропустить остальной код и перейти на следующую итерацию.

if (!$result) {
    continue;
}
$res = $result->get('value');
Про return из сниппета вам уже сказали, лучше собирать весь вывод в буфер в виде массива или строки и потом уже выводить одним return. Это даст возможность в дальнейшем, когда условия задачи изменятся, еще раз пройтись по массиву или строку и применить дополнительные действия над результатом.

Простой пример, у вас сниппет в зависимости от параметра должен выдавать или строку на экран или json в случае ajax-запроса. В таком случае можно сделать так:

$output = [];
foreach ($r as $r1) { 
    $output[] = $r1['text'];
}

return $ajax ? 
    json_encode($output)
    join("", $output);
И последнее, несмотря на то, что параметры в снипете уже присутствуют в виде переменных, лучше получать их значения через API MODX. Это в дальнейшем даст возможность использовать Property Sets (предустановленные параметры) и значения по умолчанию.

$parent = $modx->getOption('parent', $scriptProperties, null);
    Александр Мельник
    26 февраля 2017, 20:16
    0
    Спасибо, вот именно по этому я и говорю, что явно решаю вопрос топорно в силу своих слабых знаний.
    Илья Уткин
    27 февраля 2017, 10:27
    0
    Для получения значений ТВ-параметров у ресурсов MODX есть метод getTVValue. Он делает то же самое, но код становится чище, опрятнее и гораздо более читабельным:
    $res = $g1->getTVValue('tv_name');

    Ну и в дальнейшем нужно углублять знания SQL и xPDO.

    В этом коде делается много запросов к базе данных: сначала получаем все нужные ресурсы (1 запрос), и потом на каждый ресурс делаем дополнительный запрос в базу, чтобы получить значение TV-параметра. При большом количестве ресурсов сниппет будет создавать нехилую нагрузку.

    Чтобы такого избежать, используем LEFT JOIN:
    // Выборка ресурсов сразу с ТВ-шками
    $q = $modx->newQuery('modResource', array('parent' => $parent));
    $q->leftJoin('modTemplateVarResource', 'MIGX', 'modResource.id = MIGX.contentid AND MIGX.tmplvarid = 1');
    $q->select('`MIGX`.`value` AS `text`');
    $g = $modx->getIterator('modResource', $q);
    
    // Получение нужных полей из JSON
    foreach ($g as $g1){
        $res = $g1->get('text');
        if (!$res) {
            continue;
        }
        $r = $modx->fromJSON($res);
        foreach ($r as $r1){
            $output[] = $r1['text'];
        }
    }
      Илья Уткин
      27 февраля 2017, 10:30
      0
      Ой, ну и ограничить выборку, конечно, надо
      $q->where(array(
          'MIGX.value:!=' => '',
          'MIGX.value:!=' => '[]',
          'MIGX.value:IS NOT' => NULL
      ));
      Авторизуйтесь или зарегистрируйтесь, чтобы оставлять комментарии.
      8