I have two classes with the following code. The second has a need for a constructor as does the parent. I named the constructor in the parent the same name as the class and then I call it from the descendant, effectively executing both constructors. It seems to work fine, just wondering if this is the right way to do this?
class MainClass {
protected $retval = array();
protected function MainClass() {
include DB_SERVER_LOCATION;
$this->link = $link;
$this->retval[0] = 0;
$this->retval[1] = 'No Errors';
}
}
class Class2 extends MainClass {
protected $user;
public function Class2(CollUser $user) {
$this->user = $user;
parent::MainClass();
}
}
It is the validator classes. I stripped out all the validation but the main class has several generic validation methods and the inherited class pertains to users so it adds methods specific to users and also handles the validation call and routine. Do you want me to post the whole thing? Happy to if that would help.
<?php
class Validator {
protected $retval = array();
protected function Validator() {
include $_SESSION['dbserver'];
$this->link = $link;
$this->retval[0] = 0;
$this->retval[1] = 'No Errors';
}
function valueExists($value)
{
$isValid = true;
for ($i = 0, $j = count($value); $i < $j; $i++) {
if (!isset($value[$i]) || $value[$i] == '') {
$isValid = false;
break;
}
}
return $isValid;
}
protected function valuesEqual($val1, $val2) {
$retval = true;
if ($val1 != $val2) {
$retval = false;
}
return $retval;
}
protected function isValClean($val) //all bad words
{
$retval = true;
if (preg_match("/****|*****|*****|****|****|*****|*****/")) {
$retval = false;
}
return $retval;
}
protected function fieldValueExists($field, $table, $key, $keyvalue) {
$qt_sql = "select $field from $table where $key = ?";
try {
if (!$prep1 = $this->link->prepare($qt_sql)) {throw new Exception($this->link->error, 1);}
if (!$prep1->bind_param("s", $keyvalue)) {throw new Exception($this->link->error, 2);}
if (!$prep1->execute()) {throw new Exception($this->link->error, 3);}
} catch (Exception $e) {
ini_set('default_mimetype', 'text/plain');
var_dump($e);
die();
}
$prep1->store_result();
$cnt = $prep1->num_rows;
$prep1->free_result();
if ($cnt > 0) {
return true;
} else {
return false;
}
}
protected function validEmail($email)
{
$isValid = true;
$atIndex = strrpos($email, "@");
if (is_bool($atIndex) && !$atIndex)
{
$isValid = false;
}
else
{
$domain = substr($email, $atIndex+1);
$local = substr($email, 0, $atIndex);
$atIndex = strrpos($domain, ".");
$localLen = strlen($local);
$domainLen = strlen($domain);
if ($localLen < 1 || $localLen > 64)
{
// local part length exceeded
$isValid = false;
}
else if ($domainLen < 1 || $domainLen > 255)
{
// domain part length exceeded
$isValid = false;
}
else if ($local[0] == '.' || $local[$localLen-1] == '.')
{
// local part starts or ends with '.'
$isValid = false;
}
else if (preg_match('/\\\\.\\\\./', $local))
{
// local part has two consecutive dots
$isValid = false;
}
else if (is_bool($atIndex) && !$atIndex)
{
// domain part has no dots
$isValid = false;
}
else if ($domain[0] == '.' || $domain[$domainLen-1] == '.')
{
// domain part starts or ends with '.'
$isValid = false;
}
else if (preg_match('/\\\\.\\\\./', $domain))
{
// domain part has two consecutive dots
$isValid = false;
}
else if (!preg_match('/^[A-Za-z0-9\\\\-\\\\.]+$/', $domain))
{
// character not valid in domain part
$isValid = false;
}
}
return $isValid;
}
}
?>
<?php
class ValidatorUser extends Validator {
protected $user;
public function ValidatorUser(CollUser $user) {
$this->user = $user;
parent::Validator();
}
//validate for
public function validateUserSimple() {
$passvar[] = $this->user->username;
$passvar[] = $this->user->password;
$passvar[] = $this->user->password2;
if (!parent::valueExists($passvar)) {
$this->retval[1] = "You must fill in the username and both password fields. Please try again.";
$this->retval[0] = 1;
} else if (!parent::valuesEqual($this->user->password, $this->user->password2)) {
$this->retval[1] = "Both password fields must contain the same value. Please try again.";
$this->retval[0] = 1;
} else if (parent::fieldValueExists('username', 'users', username, $this->user->username)) {
$this->retval[1] = "Someone else is already using the username you typed in. Please try another name. Thanks";
$this->retval[0] = 1;
} else if (!$this->validUsername($this->user->username)) {
$this->retval[1] = "The username you entered is not valid. It must be at least 3 characters long and contain only letters, numbers or an underscore character. No spaces or other characters are allowed. Please try again. Thanks";
$this->retval[0] = 1;
}
return $this->retval;
}
protected function validUsername($uname)
{
$retval = true;
if (preg_match("/[^a-zA-Z0-9_]/", $uname)) {
$retval = false;
}
return $retval;
}
}
?>
The problem as I thought is that your two classes do two completely different things. Your Validator class is essentially a bunch of methods for validating strings, but your ValidatorUser class’s responsibility is validating an object - 2 completely different things. Also your ValidatorUser class should only concerned with validating the various properties of a User object, and recording which properties failed the validations. You have error messages in the class which apply only to messages you would display on a specific form, so that is no good. It might be appropriate for say a FormValidator class, but not just seeing if an object contains valid properties.
One of the primary OO concepts is to “Program to an interface, not an implementation” - so for now stop thinking about implementation and lets just focus on getting the classes and responsibilities clearly defined. Here is an outline of what I think this should look like:
Validator class
what you have is basically fine - this class will contain methods to validate strings passed as arguments
now we know we want to have a class responsible for validating User objects, and since we want to reuse as much of this for other objects, we’ll create 2 more classes:
ObjectValidator
when passed an object in it’s constructor, this will call a validate() method that must be defined in the child classes. Since each object is going to require a different implementation of this validate() method, we can make this class abstract.
UserValidator extends ObjectValidator
all the logic is within the base class, so we simply need to validate the properties of the User (using the Validator object’s methods), and use the base classes methods to mark which properties did not pass validation.
Error messages might require another class, but we can worry about that later. Try writing outlines of the classes above, but don’t include any functionality. For example, your Validator class might look like this:
class Validator {
protected function __construct() {}
public function valueExists($value) {}
public function valuesEqual($val1, $val2) {}
public function isValClean($val) {}
public function fieldValueExists($field, $table, $key, $keyvalue) {}
public function validEmail($email) {}
}
Keep in mind how you will want to create these classes and call their methods - i.e. what do you want the calling code to look like? For example, I would want to call the UserValidator class like this:
$userValidator = new UserValidator($user);
$userValidator->validate();
if ($userValidator->failed()) {
// do stuff here
}
As I understand this, I am doing a couple of things here and for the moment let’s stay with the UserObject. I want to add an item to the UserObject but in my case there are two types of adds, and maybe more, all requiring different validation processes because they require different attributes of the object to be filled in. So I need to validate (several types of validation) and add an object.
Given your input, the structure as I see it is as follows:
CollUser()
user attributes all set to default ('' or 0)
__constructor($userid = 0) (populates the user attributes from the database if passed a userid
setUserId (sets the userid and the $_SESSION['userid'] on an add)
Validator()
common validation routines (valid email, strings match, value exists in db already, etc.)
UserValidator(CollUser $user) extends Validator
validateUserObject() //this one would validate the full object to assure all fields comply
validation routines that are specific to the UserObject (such as acceptable format of a username)
UserObjectGateway()
__construct() (db access)
public function simpleAccountAdd()
logic for handling results of the validation of the UserObject for a simple add (username and pw only)
public function fullAccountAdd()
logic for handling results of the validation of the UserObject (username, pw, email, geographical info)
public function upgradeAccount()
logic for handling results of the validation of the UserObject (all user object information)
public function editAccount()
logic for handling results of the validation of the UserObject (all user object information)
public function resetPassword()
public function changePassword()
PHP Controller
$user = new CollUser and update attributes for information I have
new UserValidator($user) returning a success or fail and a message as to why
new UserObjectGateway() and then call simpleAccountAdd() to handle validation results and the add to the object/db
I’m not sure I understand the separate Validator class. Unless what you are saying is that I can call the Validator Class anyhow and it really adds nothing to my UserValidator? So would I just create a new Validator object from within my UserValidator?
And what is the advantage of the abstract class sitting above the UserValidator since I have to create a separate method for each object type anyhow. Is this consistency of form? So I create an abstract class that accepts an object and has an empty method and then I fill in the object type and the method details in the dependent?
This is looking a lot better IMO. You’re completely on the right track because you are stepping back and outlining the objects and their responsibilities instead of rushing into trying to code the implementation.
Yes - you could use that class within your object validator classes to validate the various properties.
The idea of the abstract base class right now is just planning ahead. However since you’re currently only working on implementing the User object it isn’t very relevant at this point - you would be fine to pack all the implementation into the UserValidator class (for now). Then next time you need to create a Validator class for another object, you will see that a lot of the inner workings of the class will need to be duplicated - that is the point to refactor and put all of that duplicated code into an abstract base class.
