HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Cleaning up validator class

Submitted by: @import:stackexchange-codereview··
0
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

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 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.