Проверка данных. Насколько целесообразно делать так?

Question

[dev400]

public function validChangeData($array,$uid) {
        if(empty($array['email']))  {
            return false;
        }
        if(!preg_match("/^[a-z0-9](([_\.-]?[a-z0-9]+)*){0,19}@([a-z0-9]([_-]?[a-z0-9]+)?\.){1,3}[a-z]{2,6}$/i", $array['email'])) {
            return false;
        }
        $this->updateUserData($array,$uid);
    }

Два условия, и если не false то вызывается приватный метод, который записывает в базу. Или бред?

Comments

[ThunderCat]

а почему не в 1 строчку?

[Евгений]

а что без регулярки мыло не проверить?

[dev400]

ThunderCat: не совсем понял

Answer 1

[index0h]

false в вашем случае - это что-то пошло не так. Это не информативно. Используйте исключения.

Что касается проверок - их не достаточно.
Обязательно проверяйте типы аргументов для простых типов, для обьектов - лучше тайп хинтинг. Например что произойдет, если вместо массива вам влетит обьект?

Могу порекомендовать либу для быстрой валидации аргументов: https://github.com/ko-ko-ko/php-assert
В вашем случае проверка будет такая:

/**
 * @param array $array
 * @param int   $uid
 * @throws InvalidArgumentException
 * @throws InvalidArrayException
 * @throws InvalidIntException
 * @throws InvalidIntOrFloatException
 * @throws InvalidNotEmptyException
 * @throws InvalidNotObjectException
 * @throws InvalidRegexpPatternException
 * @throws InvalidStringException
 * @throws NumberNotPositiveException
 * @throws StringNotMatchRegExpException
 */
public function validChangeData($array, $uid)
{
    Assert::assert($array, 'array')->isArray()->notEmpty();
    Assert::assert($uid, 'uid')->int()->positive();
    
    if (!array_key_exists('email', $array)) {
        throw new \InvalidArgumentException('Argument "$array" has no element "email"');
    }
    
    $email = $array['email'];

    Assert::assert($email, 'email')->string()->notEmpty()->match(
        '/^[a-z0-9](([_\.-]?[a-z0-9]+)*){0,19}@([a-z0-9]([_-]?[a-z0-9]+)?\.){1,3}[a-z]{2,6}$/i'
    );

    $this->updateUserData($array, $uid);
}

Comments

[hbuser]

Есть ли смысл в библиотеке?.. Наверное, лучше порекомендовать какой-нибудь php-фреймворк. Но т.к. человек - новичок, то едва ли и одно и другое на данном этапе необходимо.

[index0h]

hbuser
Смысл есть. Фреймворк - это каркас для бизнес логики, ему как бы все равно, как вы ее реализуете.
Либа же предназначена для легкого и быстрого выполнения проверки аргументов. Это не сравнимые вещи.

[dev400]

index0h: в случае использования библиотеки нужно указывать Array? (Array $array, $uid)

[index0h]

В случае тайп хинтинга - необходимости проверять тип нету. Для php7 и выше рекомендую использовать тайп хинтинг всюду.
Для проверки именно значения переменной - стоит использовать библиотеку. Учитывая, что данную либу рекомендую использовать в каждом методе с аргументами простых типов - требования к производительности очень высокие. Быстрее на данный момент только нейтивный код.

[dev400]

index0h: благодарю!

[hbuser]

"Это не сравнимые вещи." Ну, как же? В любой относительно серьезный фреймворк входит компонент для валидации. Если совсем придираться, то фреймворк - это контейнер инструментов (в числе прочего), если так можно выразиться. А ваша библиотека - инструмент. Фреймворк - гипероним, что ли, по отношению к "вашей" библиотеке.. В этом плане - да, несравнимые. Но если не придираться, то библиотека, как и фреймворк - это уже некая автоматизация, стандартизация действий. Для начала надо сделать руками хотя бы один раз, чтобы понимать что это и с чем это едят. А человек - новичок. Возможно, он не понимает как эту библиотеку подключить, как использовать, что она делает и каким образом... Вы не согласны?..

[index0h]

hbuser
Да не вопрос)) Назовите хотя бы один фреймворк, содержащий в себе функционал, на подобии этой либы. Обращаю внимание на 2 вещи:
1. Не validate, а assert, в смысле бросание исключений
2. Максимальная производительность. Этот функционал используется в каждом методе для проверки входящих аргументов простых типов (за исключением тестов разве что).

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

> Для начала надо сделать руками хотя бы один раз, чтобы понимать что это и с чем это едят. А человек - новичок.
Дык тоже не вопрос)) эта либа покрывает далеко не все возможные кейсы, при этом расширить ее собственными очень легко, достаточно заэкстендить, написать свое исключение и свою проверку, все.

Рекомендую я ее не потому, что "вот смотрите, какой я молодец", а потому, что это приведет реальному снижению количества возможных ошибок и повышению безопасности кода. Чем раньше придет это понимание - тем лучше.

[dev400]

Трудности были небольшие. Но в итоге внедрил и узнал кто такие namespaces

Answer 2

[ThunderCat]

if(!empty($array['email'])&&
preg_match("/^[a-z0-9](([_\.-]?[a-z0-9]+)*){0,19}@([a-z0-9]([_-]?[a-z0-9]+)?\.){1,3}[a-z]{2,6}$/i", $array['email']))  {
        $this->updateUserData($array,$uid);       
}

Зачем такой огород городить? Код должен читаться ну, как фраза что-ли. Внятно читаться. "Если емайл не пустой или совпадает с шаблоном - записать изменения".

Answer 3

[hbuser]

Что именно вы имеете в виду под бредом? Производительность, красота, логичность, минимальное количество строк кода, отсутствие повторений, ... .
Можно было бы записать немного по-другому, но и то что есть - не сказать, что это бред. Просто код. Обыкновенная проверка.
Это не какой-то требовательный к ресурсам код, не что-то особенное. Повторюсь... просто код.
P.S. Если недостатки на ровном месте искать, то это только если фен-шуй за уши подтягивать...

Comments

[dev400]

да я только учу php, вдруг что то серьезное пропустил.

[hbuser]

Все уже неплохо, т.к. используете ООП.

[hbuser]

И да, серьезного ничего не пропустили, вам сейчас будут оптимизировать код, говорить как строчки лучше переписать, где местами поменять, запись в одну строку, без пробелов и пр. В общем - рабочий код. По поводу empty и isset читайте здесь - stackoverflow.com/questions/7191626/isset-and-empt...

[dev400]

hbuser: С ооп сразу и начал учить php, относительно недавно. Спасибо!