Везде ли нужно проверять на существование элемента в массиве?

Question

[Sergey Ilichev]

Всем привет коллеги.

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

[
    'a' => $this->someParam['a'] ?? ''
]

Коллега говорит, что плохо код загрязнять лишними проверками. Типа ты и так знаешь, что у someParam всегда есть в массиве ключ 'a'. Мне кажется, что такие надежды на то что есть, а чего нету слишком смелые. С другой стороны уловить ошибку в таком случае сложнее. Допустим, кто-то зачем-то удалил ключ 'a' из массива, а у нас тут идет оператор ?? и просто подпихивает пустую строку. Код работает и ошибки не видно. С другой стороны, если обязательный параметр для передачи куда-то, то можно запихнуть все в dto и там делать проверки любые, валидацию и так далее, кидать грубый эксепшен, если нет нужного в массиве ключа. Но тут коллега опять ворчит, типа еще и dto создавать для такого пустяка. В общем его позиция, что пусть просто падает код и все, если нет нужного ключа в массиве, типа так понятней. Моя позиция что нужно такие вещи обрабатывать как-то.

Что думаете ребята?

Comments

[twobomb]

пусть падает

[FanatPHP]

Моя позиция что нужно такие вещи обрабатывать как-то.

Надо.
Вот только код 'a' => $this->someParam['a'] ?? '' - это что угодно, только не обработка.
Это скорее заметание мусора под ковёр.
А по-другому ещё называется "подавление ошибки". Делает ровно то же самое, что и оператор @
Можешь спросить у коллег, что это за оператор

[TheAthlete]

Смотри, тут несколько моментов:
1. Валидация нужна в любом случае, перед этим кодом
2. Бывают случаи, когда с одной стороны null допустимое значение, а с другой стороны в последующем коде нужна работа со строкой. Если такой код как у вас не написать, то скорее всего будет ошибка (если некая функция ожидает строку, а получает на вход null). В вашем коде вы по сути выставляете значение по-умолчанию. В JS и Perl это достаточно распространенная идиома. Например, в Perl

my $name = $c->param('name') || '';
my $email = $c->param('email') || '';

Answer 1

[Алексей Уколов]

Правы оба.
Лучше раньше упасть, чем обработать кривые данные так, как будто они нормальные. Но и просто бездумно падать в самую базовую ошибку неправильно - потом задолбаетесь выяснять при каких обстоятельствах код упал. Лучше провалидировать и кинуть осмысленное исключение, которое поможет отловить проблему.

Comments

[FanatPHP]

Хороший ответ, именно в плане валидации.

Answer 2

[profesor08]

На мой скромный взгляд, проблема несколько шире. Вот почему someParam это массив, а не осмысленный объект? Пока его суть остается неопределенной, будут возникать неопределенные ситуации, когда непонятно что делать, падать в ошибку или городить полотно проверок. А ведь можно вынести всю валидацию на этап создания объекта, там же выбрасывать ошибки, так будет явно понятно, что ошибка в данных, а не где-то в логике, где данные уже должны быть.

Comments

[xfg]

Абсолютно такая же мысль посетила при прочтении, что первое, что нужно сделать перестать использовать ассоциативные массивы для описания состояния объекта, а использовать для этого собственно сами объекты.

Answer 3

[MikUrrey]

Привет!
Если массив является частью входных данных, будь то user input, или какой-то файл, или БД, то всё верно - проверка нужна (даже обязательна). А вот в таком случае и подобных точно нет:

$this->someParam = [
     'a' => 1,
     'b' => 2,
];
//...
//...
//...
$a = $this->someParam['a'] ?? '';

Такая проверка уже сродни

if (TRUE !== FALSE) {
     //...
}

Comments

[Sergey Ilichev]

Ну там не совсем 'a', там идет $this->operator['groupName'], это я для примера привел 'a') Меняет дело или нет на ваш взгляд?

[MikUrrey]

Sergey Ilichev , да, это понятно. Суть та же, нет необходимости проверять в скрипте, правильно ли работает сам РНР)

Answer 4

[Konstantin]

Точно такой же вопрос возникал неоднократно и у меня с коллегами. Кто-то за то, чтобы при каждой выборке из бд, (в случае когда работаешь с массивами, а не объектами) нужно проверять наличие полей, у меня это вызывает недопонимание.
ИМХО правил тут не может быть, нужно подходить с точки зрения благоразумности. И да, лучше использовать DTO по возможности.

Answer 5

[Михаил Ливач]

иногда такие проверки бессмысленны. Допустим, Вы обращаетесь к функции, которая возвращает список, каждый элемент которого - массив определённой структуры, например ['id'=> ..., 'name' => ..., 'timestamp'=> ...,]
Понятно, что не надо у каждого элемента проверять, есть ли там эти ключи - функция-источник гарантирует это.

Answer 6

[Vitsliputsli]

Вы описываете 2 стандартных подхода:
1) падаем в ошибку при любой непонятной ситуации;
2) при непонятной ситуации пробуем продолжить работу, если это возможно.
1 подход применяется в тех местах которые работают критически важными бизнесово данными. 2 подход для маловажных вещей, которые не способны исказить вещи из предыдущего предложения.
Т.е. в большинстве случаев нужно падать в ошибку, и только если знаете, что ошибка не несет никакого серьезного аффекта (к примеру выводить текст на экран жирным шрифтом или нет), то можно продолжать работу.

Answer 7

[Pavel Omelchenko]

Посмотрите в сторону таких инструментов как phpstan psalm phan. Там можно объявлять какие ключи должны быть в массиве. Это поможет при написании кода - не забыть что-то важное. Но в рантайме, да - больше проверок к чему-то важному это всегда хорошо. Особенно если массивы кастить в дто. У объектов и api приятней. Всегда же удобней работать с методами/свойствами чем вспоминать корректное название ключа

Answer 8

[Запретили Задавать Вопросы!]

Самый Правильный и Сжатый Ответ - Абсолютно Всегда!