AJAX-based PHP server-side form validation

Question

I am still a newbie at coding in general. Over the years the amount of programming that I have done in PHP has always been procedural. I am now working on a project for my course, and was hoping to use it as an opportunity to learn OOP since I come across it everywhere.

I am still trying to really get my head around OOP. I understand the basics (I hope).

Here is my very first attempt. I am working on an AJAX based server-side (PHP) form validation. I have tried to write my validation code along OOP principles.

Does this make sense from an OOP perspective?

    require_once 'db.php';

class Registree {

    /*Some of the properties*/
    var $Username;
    var $Email;
    var $Password;
    var $Password_repeat;

    /*Function to check that the $ProposedUsername is not already taken*/
    function checkUsername($ProposedUsername) {

        $this->$username = $ProposedUsername;

        /*Only start checking once the user has entered 4 or more characters into the username field*/
        if (strlen($ProposedUsername) => 4) {

            /*Make a query from the users table based on the $ProposedUsername*/
            $result = mysqli_query($dblink, "SELECT * FROM users WHERE `username` = '$ProposedUsername'") 
            or die(mysqli_error($dblink));

            /*If any rows were affected, then this username is taken*/
            if (mysqli_affected_rows($dblink)) > 0) {
                echo 'Username already taken';
            } else {
                    echo 'Username available';
                }
        }
    }
}

Answer 1

What I would do:

• Use an autoloading function (basic example, PHP manual) to get rid of require_once
• $this->$username won't work as you expect, replace it with $this->Username
• Avoid magic numbers like if (strlen($ProposedUsername) => 4), instead define constants (or class constants)

But basically, it seems that you have quite well understood the basic principles of OOP.

As a matter of preference, I would:

• Use Allman coding style
• Use // instead of /* */ for one line comments

• throw a UsernameAlreadyTakenException (create it by simply extending Exception) and do nothing if username is available, instead of simply echoing the checking results.
  EDIT: I like exceptions for such "checking" functions, because IMO it's a good way to refactor tons of "if" + error handling code. Calls look like:

  // clean readable checkings, and if something goes wrong, you are informed :-)
  // however and obviously, in production environments, all exceptions should be
  // caught at high level ==> logged + a user-friendly message displayed.
  $user->checkUsernameExists();
  $user->checkEmailExists();
  
  // or in a few cases
  try {
    $user->checkUsernameExists();
  } catch (UsernameAlreadyExistsException $e) {
    //...
  }
  

Answer 2

1. You can try to use mysql binding instead of substituting data in query (did you forget escaping for sql-injections?)
2. When you type $this->$username = $ProposedUsername; and $username='xxx'; it means that you should have 'var $xxx;' before trying to access this variable. Read about variables variable
3. Read Zend Naming Convetion - it's rather widely spread and following it helps to write code understood by anyone. I mean var $Password_repeat; should become var $passwordRepeat; Try to use underscore_case OR camelCase (not AND)

Answer 3

All your var $something; declarations should be public or private, rather than var.

Where you do $this->$username, it should be $this->Username, although it the context you've used it there doesn't seem to be any actual need for storing the parameter into the member variable. Perhaps storing it when you've confirmed the username is available for use would make more sense.

Answer 4

Here are some tips :

1. It's a good practice to have getter and setter methods for fields instead of declaring them as public.(so you can be in control of changes of the state of your object)
2. checkUsernsmeMethod is setting the username and check the validation of purposed name which is not a good practice.It's better to have each function has a very simple responsibility that is recognizable by its name.Besides It writes directly to UI via echo methods which is not a good practice either.You can use two approaches here to report back to client.You can throw an exception if purposed name is not valid (Thus enforce the client to do something to handle this exceptional situation) or you can take a more passive approach and report back the result via a return value (perhaps an object)

Answer 5

The other commenters have covers most of the main points, but I did want to add one of my own. Your checkusername () method doesn't return anything, but instead echos out messages regarding success or otherwise. This is inflexible, because you can't easily change the messages. If you want to reuse this class in a different application that requires different success or fail messages then you'll have to go in and change them manually. And what are you going to do if you want your application to work in languages other than English?

A better approach would be for checkusername to return true or false depending on whether the username is free or not. (I'd also consider renaming the method to something more descriptive of what it tests for in this case, like usernameIsFree ())

public function usernameIsFree ($ProposedUsername) {
    $free = false; 

    $this->$username = $ProposedUsername;

    /*Only start checking once the user has entered 4 or more characters into the username field*/
    if (strlen($ProposedUsername) => 4) {

        /*Make a query from the users table based on the $ProposedUsername*/
        $result = mysqli_query($dblink, "SELECT * FROM users WHERE `username` = '$ProposedUsername'") 
        or die(mysqli_error($dblink));

        /*If any rows were affected, then this username is taken*/
        $free = (mysqli_affected_rows($dblink)) == 0);

    }
    return ($free);
}

Now the message is decoupled from the code necessary to check if a username is free.

echo ($instance -> usernameIsFree ('Fred')? 'Username is free': 'You can`t have that username, it\'s already taken!');

By the way, the username checking method is doing two things (checking that the username meets some validity parameters such as minimum length, and checking that the username is free). I'd be inclined to split this into two independent methods, as a validity check and an availability check are really two different operations.