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