Писать код не сложно. Сложно писать код так, чтобы его было легко читать и поддерживать. К сожалению, не всегда понятно, как лучше организовать код. Бывает, что при чтении кода спотыкаешься и долго не можешь понять, что же здесь не так.
Есть некоторый код, проверяющий переданные данные. Казалось бы, все хорошо: есть массив данных, метод бежит по нему:
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;
не там, где нужно, или же не поставить там, где это необходимо.