patternphpMinor
Cleaning up validator class
Viewed 0 times
validatorcleaningclass
Problem
I updated my class with more functionality, but I kind if feel like it's a mess or that it can at least be improved by a bit readability, efficiency and/or DRY wise.
Anybody who could see if that can be done?
Any other suggestions are also welcome.
```
class Validator
{
private $errors = [];
private $rules = [];
public function getErrors($rulename = null)
{
if ($rulename) {
return isset($this->errors[$rulename]) ? $this->errors[$rulename] : null;
}
return $this->errors;
}
# rules will be checked against in order as specified
public function setRule($name, Array $rules, $required = true)
{
if (!array_key_exists($name, $this->rules)) {
foreach ($rules as $rule) {
if (!$rule instanceof IRule) {
throw new Exception('Array must only contain instances of IRule.');
}
}
$this->rules[$name]['rules'] = $rules;
if ($required === true) {
$this->rules[$name]['required'] = 'Must not be null or empty.';
} elseif ($required === false) {
$this->rules[$name]['required'] = false;
} else {
$this->rules[$name]['required'] = (String) $required;
}
} else {
throw new Exception(sprintf('Rule %s already exists.', $name));
}
}
public function validate($rulename, $input)
{
if ($this->rules[$rulename]['required']) {
if (empty($input)) {
$this->errors[$rulename][] = $this->rules[$rulename]['required'];
}
foreach ($this->rules[$rulename]['rules'] as $rule) {
try {
$rule->check($input);
} catch (Exception $e) {
$this->errors[$rulename][] = $e->getMessage();
}
}
if (isset($this->errors[$rulename])) {
return
Anybody who could see if that can be done?
Any other suggestions are also welcome.
```
class Validator
{
private $errors = [];
private $rules = [];
public function getErrors($rulename = null)
{
if ($rulename) {
return isset($this->errors[$rulename]) ? $this->errors[$rulename] : null;
}
return $this->errors;
}
# rules will be checked against in order as specified
public function setRule($name, Array $rules, $required = true)
{
if (!array_key_exists($name, $this->rules)) {
foreach ($rules as $rule) {
if (!$rule instanceof IRule) {
throw new Exception('Array must only contain instances of IRule.');
}
}
$this->rules[$name]['rules'] = $rules;
if ($required === true) {
$this->rules[$name]['required'] = 'Must not be null or empty.';
} elseif ($required === false) {
$this->rules[$name]['required'] = false;
} else {
$this->rules[$name]['required'] = (String) $required;
}
} else {
throw new Exception(sprintf('Rule %s already exists.', $name));
}
}
public function validate($rulename, $input)
{
if ($this->rules[$rulename]['required']) {
if (empty($input)) {
$this->errors[$rulename][] = $this->rules[$rulename]['required'];
}
foreach ($this->rules[$rulename]['rules'] as $rule) {
try {
$rule->check($input);
} catch (Exception $e) {
$this->errors[$rulename][] = $e->getMessage();
}
}
if (isset($this->errors[$rulename])) {
return
Solution
Good work formatting and approaching styles in a fashionable method, it makes reading your code very easy! a couple things to note:
-
It's common to see
-
Regarding your last method,
-
I'm not sure if you left it out, or you just haven't, but it's also a good practice to document your code. That means add comments. Googling PHPDoc will give you a good start, and if your familiar with JavaDocs, that's what I'm talking about.
Be careful making new array's with just
As of PHP 5.4 you can also use the short array syntax, which replaces
array() with [].
That's from the docs. So if anyone using below 5.4 wants your code, they could experience some turbulence setting it up.
The one comment you do have, is started with
I noticed you have
a couple times. You could put this in a function (with appropriate parameters) and simply call the function to reduce multiplicities.
Other than that, all looks well. Just in case you wanted to look at some other validation classes (if you haven't already), here's one to get you started.
-
It's common to see
private variables prefixed with an underscore to improve readability. It's up to you if you want to implement this, and if you use a framework, check their docs to see if it's standard.-
Regarding your last method,
dump(): the name dump could mean many things. Without seeing the source, what's it dumping? It'd be fine to leave it as is, but you may consider renaming to dumpRules() or something of that sort.-
I'm not sure if you left it out, or you just haven't, but it's also a good practice to document your code. That means add comments. Googling PHPDoc will give you a good start, and if your familiar with JavaDocs, that's what I'm talking about.
Be careful making new array's with just
= [].As of PHP 5.4 you can also use the short array syntax, which replaces
array() with [].
That's from the docs. So if anyone using below 5.4 wants your code, they could experience some turbulence setting it up.
The one comment you do have, is started with
#. I believe this style of comments are deprecated and the C style comments (/.../, //) are favorable. I'll try and find a reference to this claim.I noticed you have
foreach ($this->rules[$rulename]['rules'] as $rule) {
try {
$rule->check($input);
} catch (Exception $e) {
$this->errors[$rulename][] = $e->getMessage();
}
}a couple times. You could put this in a function (with appropriate parameters) and simply call the function to reduce multiplicities.
Other than that, all looks well. Just in case you wanted to look at some other validation classes (if you haven't already), here's one to get you started.
Code Snippets
foreach ($this->rules[$rulename]['rules'] as $rule) {
try {
$rule->check($input);
} catch (Exception $e) {
$this->errors[$rulename][] = $e->getMessage();
}
}Context
StackExchange Code Review Q#54874, answer score: 5
Revisions (0)
No revisions yet.