How to make a code review?

Question

1C-Bitrix

Good day to all! We were asked to do a code review:

<?php

if (!defined("B_PROLOG_INCLUDED") || B_PROLOG_INCLUDED!==true) {
    die();
}

use Bitrix\Main\Data\Cache;

$cache = Cache::createInstance();
/*ttl 604800 is week*/
if(!$cache->initCache(604800, 'library_classifiers_result_'.$arParams['SEARCH'] ?? 'nosearch', '/iblock/library_classifiers/')) {
    $cache->startDataCache();
    $tmpArResult = [];
    foreach($arResult as $section => $items) {
        foreach($items as $item) {
            if(isset($arParams['SEARCH'])) {
                $continueFlag = true;
                foreach($item['PROPERTIES'] as $propAr) {
                    if($propAr['CODE'] === 'depth' || $propAr['CODE'] === 'PARENT_CODE') {
                        continue;
                    }
                    if(strpos(strtolower($propAr['~VALUE']), strtolower($arParams['SEARCH'])) !== false) {
                        $continueFlag = false;
                        break;
                    }
                }
                if($continueFlag) {
                    continue;
                }
                if(isset($item['PROPERTIES']['PARENT_CODE']['~VALUE'])) {
                    $tmpParents = [];
                    if(!isset($tmpArResult[$section][$item['PROPERTIES']['PARENT_CODE']['~VALUE']])) {
                        $anParent = $item['PROPERTIES']['PARENT_CODE']['~VALUE'];
                        while (!isset($tmpArResult[$section][$anParent]['SECTION_ID']) && isset($anParent)) {
                            $parent = [];
                            foreach($items as $itemTmp) {
                                if($itemTmp['FIELDS']['CODE'] === $anParent) {
                                    $parent = $itemTmp;
                                    break;
                                }
                            }
                            handleElement($parent, $tmpParents, $section, $arParams['SEARCH']);
                            $anParent = $parent['PROPERTIES']['PARENT_CODE']['~VALUE'] ?? null;
                        }
                        $tmpParents[$section] = array_reverse($tmpParents[$section], true);
                        $tmpArResult = array_merge_recursive($tmpArResult, $tmpParents);
                    }
                }
            }
            handleElement($item, $tmpArResult, $section);
        }
    }
    $cache->endDataCache($tmpArResult);
}
else {
    $tmpArResult = $cache->getVars();
}
$arResult = $tmpArResult;

function handleElement($item, &$tmpArResult, $section, $search = false) {
    foreach($item['PROPERTIES'] as $propAr) {
        if($propAr['CODE'] === 'depth') {
            if($propAr['~VALUE'] > 1) {
                $tmpArResult[$section][$item['FIELDS']['CODE']]['DEPTH'] = $propAr['~VALUE'];
            }
            continue;
        }
        if($propAr['CODE'] === 'PARENT_CODE') {
            $tmpArResult[$section][$item['FIELDS']['CODE']]['PARENT_CODE'] = $propAr['~VALUE'];
            $tmpArResult[$section][$propAr['~VALUE']]['CHILDES'] = true;
            continue;
        }
        $tmpArResult[$section][$item['FIELDS']['CODE']]['VALUES'][] = $propAr['~VALUE'];
        $tmpArResult[$section][$item['FIELDS']['CODE']]['NAMES'][] = $propAr['NAME'];
        if($search !== false && $tmpArResult[$section][$item['FIELDS']['CODE']]['INACTIVE'] !== true && strpos(strtolower($propAr['~VALUE']), strtolower($search)) === false) {
            $tmpArResult[$section][$item['FIELDS']['CODE']]['INACTIVE'] = true;
        }
    }
    $tmpArResult[$section][$item['FIELDS']['CODE']]['SECTION_ID'] = $item['SECTION_ID'];
}

This is a Bitrix component modifier. I don’t have much experience in this matter, so tell me what you should pay attention to? So far, I don't like a lot of ifs here. Why not use else if? In general, what is better in terms of performance, a bunch of if or else if or switch case?

Answer 1

Why were you forced to do a code review if you don't even know how it should be? Read the Symfony code, try to understand why it's written that way... and quit your job where they force you to review the Bitrix code. After all, everything here is shit, starting with the lack of a single entry point into the application and ending with pseudo-OOP in the style of calling static methods.

Answer 2

If index0h doesn't mind, I'll answer with a link They asked me to check the code, what should I look at?
upd: or even https://github.com/index0h/php-conventions

Answer 3

Well... how should I put it. It will be difficult with a review, because absolutely everything is bad here. Well, i.e. in general, every line, except for "use Bitrix\Main\Data\Cache;". I don't think it needs to be rewritten. The rest is under the knife.
Fig knows how to learn to review what you just need to delete and write again, honestly.

Answer 4

I started to write comments in your code, but everything is very bad here.
1. Don't use the constant check approach and die/exit is a vestige from the days when it was fashionable to push executable code into the webserver's public directory.
2. Do not be stingy with variable names, tmpArResult and anParents are names that do not say anything about what you have there or why it is.
3. Move the logic of getting your data into a separate class, without a cache.
4. For complex data, it is quite normal to use DTO / Entity, associative arrays do not have a declared structure.
5. Don't use global variables, it's better to forget about their existence.
Fix it first, then come and see more.