1
\$\begingroup\$

I have designed the following input validation implementation with one goal: DRY. I'd like to know whether it is intuitive and what are the possible shortcomings.

$vlad = new \ay\vlad\Vlad();

$test = $vlad->test([ // Test script
    [ // Group of selectors and validators. Each selector is assessed against each validator.
        [ // Selectors
            'foo',
            'bar',
            'baz'
        ],
        [ // Validators/ validation options
            ['fail' => 'break'],
            // Define Validator failure scenario:
            // * 'silent' exclude input from the current validator chain.
            // * 'soft' record an error and progress to the next Validator.
            // * 'hard' (default) record an error and exclude the selector from the rest of the Test.
            // * 'break' record an error and interrupt the Test.
            'required',
            'not_empty',
            ['fail' => 'hard'], // Reset default Validator failure scenario.
            'email',
            ['length', 'max' => 10] // Length Validator with 'max' option.
        ]
    ],
    [
        ['qux', 'quux'],
        [
            // Validate email only if it is non-empty string.
            ['fail' => 'silent'],
            'not_empty',
            ['fail' => 'hard'],
            'email'
        ]
    ],
    [
        ['password'],
        [
            // 'password' selector value must match 'password_confirm' selector
            ['match', 'selector' => 'password_confirm']
        ]
    ]
]);

$result = $test->assess($dummy_input);

The entire library documentation and examples are avaiable at http://anuary.com/vlad.

\$\endgroup\$
4
  • \$\begingroup\$ This question is not a good candidate for Code Review I feel. You are asking us to review a piece of code that only servers to demonstrate the use of library you wrote.. It would make more sense to review the actual library. \$\endgroup\$
    – konijn
    Jan 9, 2014 at 15:41
  • \$\begingroup\$ @tomdemuyt I disagree, interface to the library is just as important (if not more important), than the internal library logic. You usually can change quite easy the internals (without affecting the interface), but not vice-versa. While I would love someone to critique the whole library, I believe this is against the intention of codereview.stackexchange.com. After all, you are asked to post the whole code relavent in the question. \$\endgroup\$
    – Gajus
    Jan 9, 2014 at 16:09
  • \$\begingroup\$ And what if my main issue here is the rather non-obvious array parameter (which is basically the whole thing)? That's an issue with the library and how it takes its input, not with this code. I have absolutely no idea what this code does without reading the docs. That's not good. \$\endgroup\$
    – cHao
    Jan 9, 2014 at 16:14
  • 1
    \$\begingroup\$ DRY is important, but in some cases, repetition improves readability and then, DRY has to take a step back. I much rather use an API that requires me to call $form->addRule(VALIDATE::EMAIL_TYPE, VALIDATE::REQUIRE_VALUE); a couple of times, so that I can later see, from the way I build the form, which fields are required and which aren't. I can also easily add/change/remove or even if(){} rules out. Your API relies on users writing a single well structured array. That's harder to read \$\endgroup\$ Jan 9, 2014 at 16:36

1 Answer 1

2
\$\begingroup\$

Well, since you didn't post your lib code, I'm assuming you're looking for an API-review. I hope you're not going to take this the wrong way, but I wouldn't like to use your validator class at all.
If I were you, I'd provide the users with an API that uses constants for the supported validation types, and -like I said in my comment- DRY is all well and good, but DOYOC (Do not Obfuscate Your Own Code) is more important, still.

I suggest you bash out an API that would allow me to do this:

$validator = new Validator();//basic
//or
$validator = new Validator(
    array(
        Validator::DEFAULT_FAIL => VALIDATOR::FAIL_BREAK,
        Validator::DEFAULT_VALUE => VALIDATOR::VALUE_REQUIRED,
    )
);

So pass an array with the default behaviour to the constuctor, and in case no array is passed: provide a setDefaults method + force the constructor to the strictest settings possible by default. Then the user can specify only those settings he wants to loosen up a little. But in general, when it comes to validation: the stricter the better.

Then, add methods to add what you call a selector, I'd call it an entity, which the user can then use to add a validation rule:

$validator->addEntity(
    $name,//name of entity
    Validator::TYPE_EMAIL,//validation type
    array(
        'value' => Validator::VALUE_OPTIONAL //override defaults here
    )
);

This not only makes the resulting code easier to read/maintain, but also makes it more reusable! This, too is DRY! in your case, the user will need to create a new array if he/she wants to validate things differently. I only have to do this:

$emailOptions = array('value'=>Validator::VALUE_OPTIONAL);
if ($config->requireEmail) $emailOptions['value'] = Validator::VALUE_REQUIRED;

And the rest of the code can remain as-is.
While testing, I can quickly comment out an addEntity call, or change one of them without having to go over that huge array again.

In light of this (changing rules on-the-fly): provide methods like alterRule or removeRule, too

If, in time, you get to writing form-building classes, you'll probably want your API to be able to deal with instances of a given class, and distill from it the rules required. One way to prepare your code to do so is by adding type-hints for Traversable objects.
If your API grows, it might also be worth wile considering writing Argument classes, that need to be passed to your API

I'm not going to write a full example of how I'd structure a validator class, as I've only briefly looked at your code, and I'm tired. Just thought I'd expand a little on my comment.

\$\endgroup\$
5
  • \$\begingroup\$ Just in case, I did link to the library code anuary.com/vlad. There is a github link, github.com/gajus/vlad. I see what you are suggesting. However, there are a number of libraries that do that already (e.g. Zend). Instead, I am trying to innovate the syntax. My argument is that the array syntax has a steeper learning curve, though it is actually easier to use, maintain and re-use in the long term. \$\endgroup\$
    – Gajus
    Jan 9, 2014 at 18:11
  • \$\begingroup\$ @Guy: I'd have to respectfully disagree with you on array syntax having a steeper learning curve. Anyone can write an array. It does have a steeper reading curve, and the larger the array becomes, the harder it is to read (reading-time-complexity of O(log n) actually). My worry is people will be tempted to in-line the rules, and add new ones as their project grows, so your API is actively encouraging project code to grow organically, which ends up with OO-spaghetti IMO \$\endgroup\$ Jan 10, 2014 at 7:58
  • \$\begingroup\$ Even if you are right, there are more reasons for using array syntax. One of the planned features of the library is integration with javascript: Vlad will allow to export rules as JSON to be re-used in the frontend Vlad implementation. This would be impossible with rules built using if conditions. \$\endgroup\$
    – Gajus
    Jan 10, 2014 at 18:31
  • \$\begingroup\$ @Guy: Ok, so suppose I go to a site that uses your lib, and the front-end side is there: I can tell, by looking at the client-side code that your validator is being used, so I know what server-side technology is being used. If I found a security leak in your code, I could then easily exploit that because the client-side code shows me what is validated and how... BTW: JS is a functional language, PHP isn't. Both have their pro's and cons, and both should be used accordingly. 1 on 1 translations rarely work. I'd still opt for an OO interface, and implement toJSON methods \$\endgroup\$ Jan 11, 2014 at 18:01
  • \$\begingroup\$ The possibility of the afore mentioned exploit is negligible. Even for tier 2 merchant PCI DSS requirements, not exposing the name/version of the underlying system is a nice-to-have. Therefore most people should hardly worry about that. PHP is procedural language with OO support. While there aren't many good examples of PHP and JS library interoperability, well – it is a challenge ahead. \$\endgroup\$
    – Gajus
    Jan 11, 2014 at 18:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.