OOP: First Class: Looking for Feed Back

[atrum]

Hello,

 

I have been programing websites with php for about a year now, and I feel like my procedural skills are decent. I am able to read through php scripts and understand what they are doing. So I thought it was time to move on to some OOP php.

 

I have created a validation class, and I want to get peoples opinions on it. I am always trying to improve my understanding.

 

So here is my validation class.

Be honest please, this is to help me improve.

 

<?php
//Class_Name: Validator
//Description: This class deals with validating form requirements such as email syntax,
//password complexity, and string lengths.

//example:
// $val = new Validator();
// $val->valTextOnly($string,$desc);
// $val->valNumbersOnly($numbers,$desc);
// $val->valAlphaNum($alphanum,$desc);
// $val->valPassword($password,$desc,$strength);
// $val->valEmail($email,$desc);
// $val->foundErrors();
// $val->listErrors();

class Validator{
var $errors; //Error Storage

function Validator(){
	$this->errors = array();
}
function valNotEmpty($input, $description){ //Checks for blank values.
	if($input != ""){
		return true;
	}else{
		$this->errors[] = $description. "::is blank";
		return false;
	}
}

function valLength($input,$description,$length=30){ //Checks for valid string lengths. 30 Characters is the default.
	if(strlen($input)<=$length){
		return true;
	}else{
		$this->errors[] = $description." ::is longer than the permissible length of ".$length;
		return false;
	}
}
function valTextOnly($input, $description){ //Alpha Characters Only, spaces not allowed.
	$result = preg_match("/^\S+[a-zA-Z]$/",$input);
	if($result){
		return true;
	}else{
		$this->errors[] = $description;
		return false;
	}
}

function valNumbersOnly($input, $description){ //Numerical Characters Only, spaces are not allowed.
	$result = preg_match("/^\S+[0-9]$/",$input);
	if($result){
		return true;
	}else{
		$this->errors[] = $description;
		return false;
	}
}

function valAlphaNum($input, $description){ //Alpha & Numerical Characters, spaces are not allowed.
	$result = preg_match("/^\S+[0-9a-zA-Z]$/",$input);
	if($result){
		return true;
	}else{
		$this->errors[] = $description;
		return false;
	}
}

function valPassword($input, $description, $strength = 1){ //Validates Password Strength. Default Strength is 1.

	switch($strength){
		case 1: //Low: 6 - 20 Characters Case insensitive.
			$pattern = "/^(?=.*[a-zA-Z0-9]).{6,20}$/";
			break;
		case 2: //Medium: 6 - 20 Characters: Case Insensitive : 1 Number Required.
			$pattern = "/^(?=.*\d)(?=.*[a-zA-Z]).{6,20}$/";
			break;
		case 3: //High: 8 - 20 Characters: Case Sensitive, 1 upper case, 1 lower case, 1 number Required
			$pattern = "/^(?=.*\d)(?=.*[a-z])(?=.*[A-Z]).{8,20}$/";
			break;
		case 4: //Max: Same as High, but requires 1 Special Character
			$pattern = "/^(?=.*!@#\$-_)(?=.*\d)(?=.*[a-z])(?=.*[A-Z]).{8,20}$/";
			break;
	}
	$result = preg_match($pattern,$input);
	if($result){
		return true;
	}else{
		$this->errors[] = $description;
		return false;
	}
}

function valEmail($input, $description){ //Validates correct email syntax.
	$result = preg_match("/^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,3})$/i",$input);
	if($result){
		return true;
	}else{
		$this->errors[] = $description;
		return false;
	}
}

function foundErrors(){ //Checks if any errors where reported.
	if(count($this->errors !=0)){
		return true;
	}else{
		return false;
	}
}

function listErrors(){ //Returns the Errors array.
	return $this-errors;
}

}

?>

[lastkarrde]

A good start.

 

You should rename your methods removing the val prefix from them. It's un-necessary and just makes you type more. You could also use filter_var to add some additional validators to your class (IP address, float, URL).

 

You also have some errors:

 

if(count($this->errors !=0)){

if(count($this->errors) !=0){

 

return $this-errors;

return $this->errors;

[atrum]

lastkarrde thanks a lot for the feed back, and spotting those typos.

 

 

 

Fixed

 

function foundErrors(){ //Checks if any errors where reported.
	if(count($this->errors) !=0){
		return true;
	}else{
		return false;
	}
}

function listErrors(){ //Returns the Errors array.
	return $this->errors;
}

}

[requinix]

Honest, huh? >

 

General comments:

• Try using phpDoc when you can. It's not quite built-in to PHP but it is into many other things - particularly editors and IDEs. Example:
  

  /**
  * Check that an input is not empty
  *
  * @param string $input Input value
  * @param string $description Description of input
  * @returns bool
  */
  function valNotEmpty($input, $description) {

  
  

• Please write for PHP 5.x. Preferably 5.3, but 5.2 is fine for the time being. PHP 4 is dead. 5.x has much better support for OOP stuff.
  

  class Validator {
  
  private $errors;
  
  public function __construct() {
  	$this->errors = array();
  }
  
  public function valNotEmpty($input, $description) {
  // ...
  
  }

  
  

• Why is there an upper limit on password length? Lower limit is important, but the longer the password is the better (kinda).
  
• Look into PHP's filter extension. It and the ctype_* functions can do a lot of the work your class does.
  

 

Design:

• Separate your logic from the representation. In other words, don't build error messages into your Validator and then casually display them later. Instead, let the other code do some of the heavy lifting.
  

  $validator = new Validator(); $errors = array();
  if (!$validator->valTextOnly($_POST["username"])) $errors[] = "Your username can only contain letters";
  if (!$validator->valNumbersOnly($_POST["dob_year"])) $errors[] = "That doesn't look like a year";
  // etc
  
  if (count($errors) > 0) {
  // stop and show errors
  }

 

Other Bugs:

• Your regular expressions are wrong. As they are now they only verify that there aren't any spaces and that there's a letter/number is at the end of the string. Nothing more. To check that the string contains only (eg) letters,
  

/^[a-zA-Z]*$/

• Note how that expression does not require there to be any letters at all - only that, if there is anything, it consists of letters.
  
• Similarly for the regexes used in valPassword.
  
• The email regex isn't very flexible. Doesn't allow for TLDs like .info or .museum. It's really hard to validate email addresses correctly: I suggest you aim for something very simple - "foo@bar.baz" - and send an actual email to verify the address. (Plus, you verify the owner, so two birds with one stone.)

[atrum]

@ requinix

 

Thanks, that's a lot of help.

 

So when you said I should code for php 5.x where you saying that my use of the built in constructor as the class Name is not good practice any more ?

 

 

Now on phpDoc is that basically how you setup what I think is called "Intellisense" ? So applications or IDE's will use that for quick reference pop ups?

 

With the errors, so what your saying is that I should make a class just for errors, and pass the error text from my validator class to my error class?

 

Lastly,  The regex syntax.

 

/^[a-zA-Z]*$/

 

Would I need to change that to

 

 /^[a-zA-Z]*/ 

 

 

?

 

 

Again, good stuff, keep it coming.

[ignace]

Your class has one major disadvantage. Consider the following scenario: you have a login form so you naturally create the methods to validate the data. Your website also has a registration form so you add in the extra validation.. your website has even some more forms and thus an equal number of validation methods, as your class is now 2 or 3k in lines you split them up into smaller chunks and you extend from a base validation class, validation methods that are needed across a number of classes are added in to the base class. However not all derived classes actually need the username/password validation but registration and login does so a lot of classes now have unneeded methods... Sure you can create another branch in your already huge validation hierarchy but maybe it can be simpler?

 

So, how do you mitigate this problem? Let each validation rule be it's own class and have a separate class to chain them together:

 

class Validate_Username implements Validate_Interface { // Username is a mix of validation rules
    public function isValid($username) { /* ..code.. */ }
}

class Validate_Password implements Validate_Interface {
    public function isValid($password) { /* ..code.. */ }
}

class Validator {
    public function addValidator($name, array $validator) { /* ...code... */ }
}

 

$validator = new Validator();
$validator->addValidator('firstname', array(new Validate_Strlen(6, 12), new Validate_InArray(array('John', 'Jane')));
$validator->addValidator('username', array(new Validate_Username()));
$validator->addValidator('password', array(new Validate_Password()));

if($validator->isValid($_POST)) {
    /* ..code.. */
}

 

Instead of having one giant object full of methods that could possibly be needed you can now assemble the code/validation you need, removing pollution.

[atrum]

Wow, I never knew classes could be used like that. Thanks ignace, very good information.