Рефакторинг: разбиваем смешанные типы проверок

Писать код не сложно. Сложно писать код так, чтобы его было легко читать и поддерживать. К сожалению, не всегда понятно, как лучше организовать код. Бывает, что при чтении кода спотыкаешься и долго не можешь понять, что же здесь не так.

Есть некоторый код, проверяющий переданные данные. Казалось бы, все хорошо: есть массив данных, метод бежит по нему:

private function checkData()
{
    foreach ($this->rawData as $codeType => $items) {
        foreach ($items as $key => $item) {
            if (!preg_match('/[A-Z\d]{8}/', (string)$key)
                && !preg_match('/\d+;\w+/', (string)$key)
            ) {
                $this->Logger->logError('Ключ ' . $key . ' не является допустимым для обработки');
                return false;
            }

            if (!isset($item['main_data_1']) && !isset($item['main_data_2'])) {
                $this->Logger->logError('Элемент ' . $key . ' не имеет полей main_data_1 и main_data_2');
                return false;
            }

            foreach ($item as $dataType => $itemData) {
                if (!isset($itemData['url'])) {
                    unset($this->rawData[$codeType][$key][$dataType]);
                    $this->Logger->logError('Для элемента ' . $key . ' не указан источник');
                    continue;
                }

                if (empty($itemData['currency_id'])) {
                    $this->Logger->logError('Для элемента ' . $key . ' не указана валюта');
                }

                if (empty($itemData['data'])) {
                    unset($this->rawData[$codeType][$key][$dataType]);
                    $this->Logger->logError('Для элемента ' . $key . ' не обнаружено данных');
                    continue;
                }

                if (is_array($itemData['data'])) {
                    foreach ($itemData['data'] as $date => $nav) {
                        if (!preg_match('/\d{4}-\d{2}-\d{2}/', $date)) {
                            unset($this->rawData[$codeType][$key][$dataType]);
                            $this->Logger->logError(
                                'Поле ' . $date . ' элемента ' . $key . ' не соответствует ожидаемому формату даты'
                            );
                        }
                    }
                }
            }
        }
    }

    return true;
}

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

Разводим проверки. В первом методе оставляем все критичные проверки. Обратите внимание на то, что результат каждой из проверок — возвращаемое булево значение:

private function checkCriticalDataIssues()
{
    foreach ($this->rawData as $codeType => $items) {
        foreach ($items as $key => $item) {
            if (
            	!preg_match('/[A-Z\d]{8}/', (string)$key)
                && !preg_match('/\d+;\w+/', (string)$key)
            ) {
                $this->Logger->logError('Ключ ' . $key . ' не является допустимым для обработки');
                return false;
            }

            if (!isset($item['main_data_1']) && !isset($item['main_data_2'])) {
                $this->Logger->logError('Элемент ' . $key . ' не имеет полей main_data_1 и main_data_2');
                return false;
            }
        }
    }

    return true;
}

Если вернуться к первоначальному варианту, то видно, что в начале метода есть блоки кода с ретурнами, потом большой кода (внутренний foreach) без ретурнов и в конце еще один ретурн.

Во второй метод попадают остальные проверки. Этот метод ничего не возвращает, он удаляет из переданного массива невалидные данные. Поскольку основная задача метода — отфильтровать невалидные данные, отразим это в названии:

private function filterData()
{
    foreach ($this->rawData as $codeType => $items) {
        foreach ($items as $key => $item) {
            foreach ($item as $dataType => $itemData) {
                if (!isset($itemData['url'])) {
                    unset($this->rawData[$codeType][$key][$dataType]);
                    $this->Logger->logError('Для элемента ' . $key . ' не указан источник');
                    continue;
                }

                if (empty($itemData['currency_id'])) {
                    $this->Logger->logError('Для элемента ' . $key . ' не указана валюта');
                }

                if (empty($itemData['data']) && !is_array($itemData['data'])) {
                    unset($this->rawData[$codeType][$key][$dataType]);
                    $this->Logger->logError('Для элемента ' . $key . ' не обнаружено данных');
                    continue;
                }

                foreach ($itemData['data'] as $date => $nav) {
                    if (!preg_match('/\d{4}-\d{2}-\d{2}/', $date)) {
                        unset($this->rawData[$codeType][$key][$dataType]);
                        $this->Logger->logError(
                            'Поле ' . $date . ' элемента ' . $key . ' не соответствует ожидаемому формату даты'
                        );
                    }
                }
            }
        }
    }
}

Почему такое разведение кода по двум методам важно? Потому как это снижает риск поставить

return false;

не там, где нужно, или же не поставить там, где это необходимо.