Technical Issues
public function __construct()
{
require_once 'errors.php';
$this->errorText = $errorText;
}
Will not do what you think it will do.
$val1 = new Validate();
$val2 = new Validate();
The second instance will not include 'errors.php', and thus $errorText will not be defined.
You should probably consider generalizing your error message handling since you might need to have different messages or internationalization.
In other words, your class should probably either return error codes (class constants), or your class should have some kind of facility for requesting a different error message at run time.
If you do stick with your current approach though, consider structuring it something like:
public function __construct()
{
$this->errorText = $this->getErrorText();
}
private function getErrorText()
{
include 'errors.php';
}
Your errors.php would then look like:
<?php
return array(
'something' => 'Some message'
'somethingElse' => 'some other message',
...
);
This would of course mean that the file is included on every object instantiation though. A different approach could be to store the messages in a static variable the first time they're needed.
Also, while I'm talking about this, errors.php could be a problematic path. You should probably use something like dirname(__FILE__) . '/errors.php'; so the path is more resistant to outside factors.
if (!method_exists($this, $rule)) {
throw new Exception("Method {$rule} does not exist");
exit;
}
There's two issues here:
exit will never be reachedexit shouldn't here
When you throw an exception, you bail out of the current scope up until you hit a catch block. No code under the throw will be executed.
function f() {
throw new Exception();
//No code here is ever excecuted
}
function f() {
if (...) {
throw new Exception();
//Code here is never excecuted
}
//Code here will be executed only if the throw above is not reached
}
function f() {
try {
if () {
throw new Exception();
//Code here is never exceuted
}
//Code here is executed if the throw is not reached
} catch (Exception $e) { }
//Code here is always executed
}
It gets a bit more complicated than that, but those examples should illustrate the basic behavior.
exit shouldn't be in there anyway though. Would you actually want script execution to completely stop? Typically exit should only be used if there is no way to continue the program execution without compromising security or stability.
Imagine if instead of throwing exceptions, you had only the exits there. This means that no error message would be displayed. Nothing would be logged. The code would just bail.
Imagine if someone was using your class based only on the API. They would have no idea why the code exited.
Typically low level flow control like exiting or redirecting to a different page, so on, should be handled in dedicated "controller" code and not in classes that have responsibilities unrelated to that.
Throwing an exception is the right thing to do here. It allows the code that called rule to decide how to handle it. rule should only responsible for saying "Hey, ummm, something wen't horribly wrong," not deciding how to handle it. (In fact, an uncaught exception is basically an exit with debugging information included :p. You do have to be careful with uncaught exceptions though as you need to make sure they don't get displayed to end clients. Some exceptions, like PDOException, may contain sensitive data like database credentials.)
Design Issues
Your class does way too much. This is just using a class as a collection of functions. Each validation can be seen as a "responsibility" and thus your class completely violates the single responsibility principle.
Imagine that in the future you want to add a new validation. How can you do this? Well, you have two options, both of which are unpleasant:
Edit the source code. What if many people are using your class though, or you're using it across 10 applications? Suddenly you have a lot of version management going on. Since you control the class, you could just commit it all to git which would alleviate the synchronization, but what about end-consumers of your code? This isn't a viable option for them.
Extend the class. This is also a bad option. You would have to change all of the object types in your code to be a MyValidation instead of Validation. Depending on how widespread this was, this could be a huge chore.
Or, you have a third option. Scrap your current design and abstract each validation into its own class. You could have a very simple interface:
interface Validator
{
/**
* Validates the given $data against whatever rule(s) the implementation of this
* interface enforces. Returns an array of error messages, or an empty array
* if $data is valid.
*
* @param mixed $data
* @return array An array of error messages
*/
public validate($data);
}
And then you could build on this:
class RangeValidator implements Validator
{
private $_from;
private $_to;
const ERROR_INVALID_TYPE = 'type';
const ERROR_OUT_OF_RANGE = 'range';
//You could pull this into an abstract base class to handle code -> message mapping automatically.
//That would also allow for easy internationalization later.
private $_messages = array(
);
public function __construct($from, $to)
{
//You could make this a lot looser and perhaps put it in methods setFrom and setTo.
//Just an example.
if (!is_int($from)) {
throw new InvalidArgumentException("\$from must be an int. Given value: '{$from}'");
}
if (!is_int($to)) {
throw new InvalidArgumentException("\$to must be an int. Given value: '{$from}'");
}
$this->_from = $from;
$this->_to = $to;
}
public function validate($data)
{
if (is_int($data) || (is_string($data) && ctype_digit($data))) {
if ($data >= $this->_from && $data <= $this->_to) {
return array();
} else {
return array(self::ERROR_INVALID_TYPE => $this->_messages[self::ERROR_INVALID_TYPE]);
}
} else {
return array(self::ERROR_INVALID_TYPE => $this->_messages[self::ERROR_INVALID_TYPE]);
}
}
}
This looks very laborious at first, but it gives you a certain degree of flexibility you didn't have before. Consider how you add a new validator now. You just create a new class completely unrelated to any of the other ones. (Technically it would implement Validator, and perhaps extend an abstract base class. What I mean though is that it doesn't change any of the other classes.)
In this system, consumption of your code is much simpler too. How does someone that doesn't have write access to your git repository create their own validator? They just implement validator in their own namespace rather than in yours. (Oh, side note, you should probably be using namespaces.)
For simplicity, you could make a "composite" Validator that just ties a bunch of validators together:
class EvenValidator implements Validator
{
//Imagine that this validates whether or not a number is even
//(Yes, really weird example :p)
}
class CompositeValidator implements Validator
{
$this->_validators = array();
public function __construct(array $validators = array())
{
foreach ($validators as $validator) {
$this->addValidator($validator);
}
}
public function addValidator(Validator $validator)
{
$this->_validators[] = $validator;
}
public funtion validate($data)
{
$errors = array();
foreach ($this->_validators as $validator) {
$errors += $validator->validate($data);
}
return $errors;
}
}
You could then use it like:
$range = new RangeValidator(1, 100);
$even = new EvenValidator();
$comp = new CompositeValidator(array($range, $even));
$comp->validate(5); //Returns an array of one message: 5 isn't odd
$comp->validate(2); //Returns an empty array
$comp->validate(222); //Returns an array of one message: 222 out of range
$comp->validate(223); //Two messages: 223 out of range and odd
This is some pretty clumsy usage here. With more than 5 seconds of thought though, an interface could be designed that allowed for very robust functionality while leaving the code highly maintainable and extensible.
You could even take it a step farther than a composite validator and make a "Form" class that has "Element"s which each have zero or more "Validator"s. (The approach take by Zend Framework, and I'm sure others).
If you're familiar with any PHP frameworks, you might note that all of that above looks eerily familiar. In particular, that's a modified version of the Zend_Validate interface from the Zend Framework.
I would suggest you pick a framework, go through the code, figure out how they designed things, and then ask yourself why they designed them that way: what the alternatives could have been and why they picked the path they chose. If you do look at frameworks for design, make sure you look at more than one. Though most of the major PHP frameworks/libraries are very well written overall, all of them have at least one area where they far from shine.
Also, if you haven't already, read everything you can find about SOLID.
