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

AJAX-based PHP server-side form validation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
ajaxphpsidevalidationbasedserverform

Problem

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';
                }
        }
    }
}

Solution

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) {
  //...
}

Code Snippets

// 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) {
  //...
}

Context

StackExchange Code Review Q#9488, answer score: 5

Revisions (0)

No revisions yet.