4
\$\begingroup\$

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';
                }
        }
    }
}
\$\endgroup\$
3
  • 4
    \$\begingroup\$ I see a few problems, var is deprecated you should replace with (public,private,protected). $this->$username should most likely be $this->username to assign the $username property. also you have mixed up => with >= in your condition and also your echoing from your method I think you should use return instead. \$\endgroup\$
    – Lawrence Cherone
    Feb 27, 2012 at 15:04
  • 3
    \$\begingroup\$ @LawrenceCherone - var is no longer considered deprecated. From the docs: "since PHP 5.1.3 it is no longer deprecated." It instead acts as an alias for "public." Of course, I agree that being more specific in property declarations is appropriate. \$\endgroup\$
    – thetaiko
    Feb 27, 2012 at 15:11
  • \$\begingroup\$ I think you are mixing authentication withe validation \$\endgroup\$
    – Robert
    Jun 20, 2017 at 1:56

5 Answers 5

5
\$\begingroup\$

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) {
      //...
    }
    
\$\endgroup\$
5
  • 3
    \$\begingroup\$ Most of this is good advice, so +1. I don't agree with the exception throwing if a username already exists, though. Better to return false in this case as a username already being in use is not an exceptional event. \$\endgroup\$
    – GordonM
    Feb 29, 2012 at 8:53
  • 2
    \$\begingroup\$ Agree with you Gordon — generally good advice here but the method name asks for a boolean return instead of an exception (and the method name should really be checkUsernameExists or checkUsernameIsAvailable). \$\endgroup\$
    – tonyhb
    Feb 29, 2012 at 15:08
  • \$\begingroup\$ Thanks for your comments. => Updated answer (now the "exception suggestion" is in the "matter of preference" section) \$\endgroup\$ Feb 29, 2012 at 15:49
  • \$\begingroup\$ Could you shed some light on the Autoloading function? I've got some class scripts in my frameworks requiring upwards of 20 other scripts and would love to implement an autoloader in the classes. \$\endgroup\$ Feb 29, 2012 at 16:07
  • \$\begingroup\$ => I've added links to resources about autoloading in my answer. + this one, quite useful too: waynemay.com/… \$\endgroup\$ Feb 29, 2012 at 16:42
2
\$\begingroup\$
  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)
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ public or private... or protected. But +1 anyway for pointing that out. \$\endgroup\$ Feb 27, 2012 at 15:08
0
\$\begingroup\$

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)
\$\endgroup\$
6
  • \$\begingroup\$ I wouldn't classify a username already being taken as an exceptional circumstance, just as a failure circumstance. On a really big and popular site, a new user will probably find that their preferred username already is in use to be more common than it being free! I tend to reserve exceptions for truely exceptional circumstance (failed to connect to the DB), or for things that can only arise due to programmer error (passing invalid arguments to a method). \$\endgroup\$
    – GordonM
    Feb 29, 2012 at 9:04
  • \$\begingroup\$ then you can return a result object (the second approach) to clarify the username status thus your UI can show the related message. \$\endgroup\$ Feb 29, 2012 at 9:12
  • \$\begingroup\$ Your program can have built in exceptional circumstances too.Besides as a rule of thumb don't catch system related exceptions unless you are really going to do something about them. \$\endgroup\$ Feb 29, 2012 at 9:14
  • \$\begingroup\$ A full result object might be overkill in this case :) But it's not a bad idea. In this particular case I'd just opt for returning true if the name is available, false if it isn't, and throw an exception if the query fails to execute. \$\endgroup\$
    – GordonM
    Feb 29, 2012 at 10:21
  • \$\begingroup\$ Or you can return an enum object.Here's an article that might be helpful :it.toolbox.com/blogs/macsploitation/… \$\endgroup\$ Feb 29, 2012 at 20:53
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.