views:

138

answers:

3

I've got a Validator class which creates an instance of a Validations class, which contains all the validation methods. When a validation is performed, __call in Validator is used to dispatch a call Validator->validate_method to Validations->method.

So for instance, there is a method in Validations called length_of. When the following code is run:

$v = new Validator();
$v->validate_length_of(...);

the length_of validation in the Validations class is executed.

In order to ensure that __call doesn't try to dispatch to an invalid or non-public Validation method, I use ReflectionMethod to check the specified method:

$method = new ReflectionMethod($this->validations, $validation_method);
if (!$method->isPublic())
{
  // error
}

I'm pretty sure this is the only way to determine whether or not a method is public, but I'm not sure if Reflection is appropriate to have in production code. Is this a code smell?

+1  A: 

You really shouldn't use Reflection in production code. Why don't you use is_callable here?

if (!is_callable(array('Validations', $validation_method)) {
    throw new LogicException('method does not exist');
}

This checks that the Validations class has a static method $validation_method and ensures that you may call it from current context. Actually this gives you even more flexibility then reflection, because this takes __call methods and similar stuff into account that Reflection doesn't.

nikic
Sorry, I was unclear in my question, the methods in `Validations` aren't static methods.
Daniel Vandersluis
In that case instead of `'Validations'` use `new Validations` (or an existing instance of `Validations`) as first element in the array ;)
nikic
@nikic: I did, see http://www.ideone.com/fnpoQ
Daniel Vandersluis
You forgot the array ;) You must pass an array to `is_callable` (if you use it this way.)
nikic
Ah crap, I did indeed. That was a brainfart, especially since I use callbacks all over the place in my class. :)
Daniel Vandersluis
A: 

If you need the power of the Reflection API in your code, then use it. But Reflection can be slow and a benchmark can never hurt when you use it. In the end only you can decide whether it has too much of an impact on your application. It's still microseconds we are talking about only.

However, for your specific usecase, I dont see why you would use Reflection at all. Just use an interface and make validations into separate classes implmenting that interface. See my example Validation class.

If you want to use __call with that (magic methods are slow too btw), you can use class_implements to check if the validator implements the interface and then you are sure the validate method exists, e.g. in the main Validator class add

public function __call($method, $args)
{
    if ( class_exists( $method )) {
        $validator = new $method;
        if($validator instanceof IValidate) {
            return call_user_func(array($validator, 'validate'), $args);
        }
        throw new BadMethodCallException('Class exists but is not a Validator');
    }
    throw new RuntimeException('Method does not exist');
}

On a sidenote: the Zend Framework already has an extensive number of Validators you can build on. Since ZF is a component library, you can use them without having to migrate your entire application to ZF. PEAR also has a Validation package.

Gordon
A: 

IMHO, I would stray away from using reflection in production code. Instead of trying to call validate_something I would create an interface that has the validate method required. Then for each valiadtor class, you just call $valid->validate(). It would be easier for the interpretor to cache that code.

MANCHUCK