Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 5 of 5
  1. #1
    Regular Coder
    Join Date
    Jul 2007
    Posts
    571
    Thanks
    25
    Thanked 28 Times in 28 Posts

    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 Code:
    <?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) <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) <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 Code:
    <?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>

  • #2
    Senior Coder
    Join Date
    Sep 2005
    Posts
    1,791
    Thanks
    5
    Thanked 36 Times in 35 Posts
    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.
    My thoughts on some things: http://codemeetsmusic.com
    And my scrapbook of cool things: http://gjones.tumblr.com

  • #3
    Senior Coder chump2877's Avatar
    Join Date
    Dec 2004
    Location
    the U.S. of freakin' A.
    Posts
    2,794
    Thanks
    19
    Thanked 156 Times in 147 Posts
    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 (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.
    Last edited by chump2877; 12-31-2007 at 06:14 PM.
    Regards, R.J.

    ---------------------------------------------------------

    Help spread the word! Like my YouTube-to-Mp3 Conversion Script on Facebook !! :)
    [Related videos and tutorials are also available at my YouTube channel and on Dailymotion]
    Get free updates about new software version releases, features, and bug fixes!

  • #4
    Regular Coder
    Join Date
    Jul 2007
    Posts
    571
    Thanks
    25
    Thanked 28 Times in 28 Posts
    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:
    PHP Code:
        //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:
    PHP Code:
        //Create an Object from Register User Class
        
    $newUser = new Register("members"$fields=array("1"=>'username'"2"=>'email')); 
    Last edited by srule_; 12-31-2007 at 06:43 PM.

  • #5
    Senior Coder chump2877's Avatar
    Join Date
    Dec 2004
    Location
    the U.S. of freakin' A.
    Posts
    2,794
    Thanks
    19
    Thanked 156 Times in 147 Posts
    The code you posted in your last post looks good.

    This:

    PHP Code:
    private $memberFields = array(); 
    can probably be replaced with:

    PHP Code:
    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.
    Regards, R.J.

    ---------------------------------------------------------

    Help spread the word! Like my YouTube-to-Mp3 Conversion Script on Facebook !! :)
    [Related videos and tutorials are also available at my YouTube channel and on Dailymotion]
    Get free updates about new software version releases, features, and bug fixes!


  •  

    Posting Permissions

    • You may not post new threads
    • You may not post replies
    • You may not post attachments
    • You may not edit your posts
    •