Please Review My OOP Code

Hey, This is the first class I have designed that is actually useful. I am preaty new to the concepts of OOP so I would appreciate some feedback on my class.

registerClass.php

<?php

class Register {

//Data Memeber
private $username;
private $email;
private $password;
private $errors;

//Contructor Function
public function __construct(){
$this->username = '';
$this->email = '';
$this->password = '';
$this->errors = '0';
}


//--------------------------------------------------------------
//Check Username
public function checkUsername($username) {
if (empty($username))
{
echo $this->errors = 'You forgot to enter your username.';
}
elseif (strlen($username) <2 OR strlen($username) >13 )
{
echo $this->errors = 'Your username must be between 3 and 12 characters';
}
else
{
$this->username = $username;
}
}



//--------------------------------------------------------------
//Check email
public function checkEmail($email){
if (empty($email))
{
echo $this->errors = 'You forgot to enter your email.';
}
else
{
$query = "SELECT user_id FROM members WHERE email='$email'";
$result = mysql_query($query);
if (mysql_num_rows($result) == 0) {
$this->email = $email;
}
else {
echo $this->errors = 'That e-mail is already registerd';
}
}
}


//--------------------------------------------------------------
//Check Password
public function checkPassword($password1, $password2){
if (!empty($password1))
{
if ($password1 != $password2)
{
echo $this->errors = 'Your password did not match the confirmed password.';
}
elseif (strlen($password1) <2 OR strlen($password1) >13 )
{
echo $this->errors = 'Your password must be between 3 and 12 characters';
}
else
{
$this->password = $_POST['password1'];
}

}
else
{
echo $this->errors = 'You forgot to enter your password.';
}
}

//--------------------------------------------------------------
//Enter Info To Database if all is ok

public function enterData($theQuery){
if ($this->errors == '0')
{

// Make the querys to insert new memeber into the database.
$query = $theQuery;
$result = @mysql_query ($query); // Run the query.

if ($result) { // If it ran OK.

// Send an email, if desired.

echo "THANK YOU, you can now log in";

} else { // If it did not run OK.

echo $this->errors = 'You could not be registered due to a system error. We apologize for any inconvenience.</p>';
}


}
}

}


?>


register.php:

<?php
if (isset($_POST['submitted'])) {

include('connectClass.php');
include('registerClass.php');

//Make Database Connection
$makeConnection = new MySqlConnect('root', '', 'localhost', 'textgame');

//Create an Object from Register User Class
$newUser = new Register();

//Check Data Entered in the form
$newUser->checkUsername($_POST['username']);
$newUser->checkEmail($_POST['email']);
$newUser->checkPassword($_POST['password1'], $_POST['password2']);


//Enter New User Into The Database
$newUser->enterData("INSERT INTO members(username, email, password, registration_date) VALUES ('".$_POST['username']."', '".$_POST['email']."', SHA('".$_POST['password']."'), NOW() )");

}

?>

<form action="registration.php" method="post">
<p>Username: <input type="text" name="username" value="<?php if (isset($_POST['username'])) echo $_POST['first_name']; ?>" /></p>
<p>Email Address: <input type="text" name="email" value="<?php if (isset($_POST['email'])) echo $_POST['email']; ?>" /> </p>
<p>Password: <input type="password" name="password1"/></p>
<p>Confirm Password: <input type="password" name="password2"/></p>
<p><input type="submit" name="submit" value="Register" /></p>
<input type="hidden" name="submitted" value="TRUE" />
</form>

Having the class carrying out 'echo' doesn't seem like a good thing, you'd be better of just storing the errors and letting something else worry about displaying them.

Having the length numbers in the code isn't particularly good practice, if you wanted to reuse this class on a site with different restrictions you'd have to change it- perhaps better to use member variables, with defaults, that are what's checked against and what makes up the error message- it looks like you might have changed one but not the other in the username code, as they don't agree with each other.

Passing the SQL into the method doesn't strike me as being that useful, it would make more sense for the Register class to know how to save itself- the name of the table and fields (but these also should probably be variables, in case you want to reuse this later).

You're not escaping the values that are being sent to the database; nothing to do with OOP, but no excuse not to use mysql_real_escape_string() with such code- if you do this inside the method, then you don't need to remember next time you want to use it!

Then just a couple of things that are personal taste: doing all those empty initialisations in the constructor is a bit unnecessary, I would either declare them along with the variables (private $username='';) or just leave them as null. I'm a big fan of classnames being nouns wherever possible, and so here I'd call it 'Registration' rather than register- a minor thing, but "new Register" isn't good english, while "new Registration" is.

Having the length numbers in the code isn't particularly good practice, if you wanted to reuse this class on a site with different restrictions you'd have to change it- perhaps better to use member variables, with defaults, that are what's checked against and what makes up the error message- it looks like you might have changed one but not the other in the username code, as they don't agree with each other.


If you have numbers that will never (and should never) change, use constants (http://www.php.net/manual/en/language.oop5.constants.php) (sometimes called "magic numbers"). In other words do as GJay says, assign the constant values to member variables --or I like to call them "fields" so as not confuse member variables with method variables-- and then make your fields constants.

thanks,
First thing I am doing is using your suggestion to store the database tables/fields in variables so I do no need to pass the query in as a variable. Does this look like a better way of making the class:

what I changed so far:
the class:

//Data Memeber
private $username;
private $email;
private $password;
private $memberTable;
private $memberFields = array();
private $errors;

//Contructor Function
public function __construct($dbTable, $dbfields = array()){
$this->errors = '0';
$this->memberTable= $dbTable;
$this->memberFields = $dbfields;
echo "hello ".$this->memberFields['1']; // used only for testing my array.
}


Calling the constructor:

//Create an Object from Register User Class
$newUser = new Register("members", $fields=array("1"=>'username', "2"=>'email'));

The code you posted in your last post looks good.

This:


private $memberFields = array();


can probably be replaced with:

private $memberFields;

PHP is pretty loosely typed, so you don;t need to initialize the field as an array. You assign the an array to the field in the contructor, so that's probably good enough.

But I'm sort of nitpicking there. ;)