Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This library registers a new user.

Questions:

  1. Where should the DB class instantiation happen for user class? I tried instantiation in the constructor but that property doesn't seem to be available to the rest of the user class. As a work-around, I'm creating a new instance with every function that needs the DB class. It works, but this must be wrong.

  2. Which methods should be public and which private/protected? I'm thinking encryptUser() should be private, but I'm not sure.

  3. Should my database functions to insert and query be combined into one function or is it fine having two? What would a combined function look like.?

  4. Is there a better way to access my config variables or is putting global $config in the constructor?

<?PHP

error_reporting(E_ALL);
require_once 'config.php';
define('BASEPATH',realpath(dirname(__FILE__))  );
define('APPROOT', 'http://blahblah.com/' );


class User{

  public $data;


  public function __construct(){
    $data=new DB;

  }

  public function createNew(){
    //trim post values
    if(!empty($_POST['email'])&& !empty($_POST['password'])){
       $fname=trim($_POST['fname']);
       $lname=trim($_POST['lname']);
       $userEmail=trim($_POST['email']);
       $password=trim($_POST['password']);

         if(!$this->user_exists($userEmail)){

          //salt, hash then insert new user into database
           $crypt=$this->encrypt_user($password);

           $mydata=new DB;

           $sql="INSERT INTO users (fname,lname, password,salt, email) 
                     VALUES (:fname,:lname, :password,:salt,:email)";
           $query_params=array(':fname'=>$fname,':lname'=>$lname,
                            ':password'=>$crypt['password'],':salt'=>$crypt['salt'],
                            ':email'=>$userEmail);

           if($mydata->dbInsert($sql,$query_params)){

             echo '<br>INSERT SUCCESSFUL';

            }else{die('Insert Failed');}

          }
          else{
               echo 'A user with the email'.$userEmail.' already exists. 
                Please user another or click here to login';
                }



    }

  }/*END CREATE USER*/


 public function user_exists($email){
   $mydb=new DB;
   $query='SELECT email from users WHERE email=:email';
   $params=array(':email'=>$email);

   if($mydb->dbQuery($query,$params)){
      return true;
     }else{
           return false;
          }
    }

 public function encrypt_user($pass){
    $salt=dechex( mt_rand(0,2147483678) ).dechex( mt_rand(0,2147483678) );
    $password=hash('sha256',$pass.$salt);

    for($round=0;$round<65336; $round++){
           $pass_hash=hash('sha256',$password.$salt);
           }

  return array('salt'=>$salt,'password'=>$pass_hash);

 }//*end encrypt_user()


}

//----------**END CLASS USER**------------


class DB {

    private $db;
    private $dbHost;
    private $dbUser;
    private $dbPass;


   public function __construct(){ 
    global $config;

    $this->db=$config['dbName'];
    $this->dbHost=$config['dbHost'];
    $this->dbUser=$config['dbUser'];
    $this->dbPass=$config['dbPass']; 
   }


  public function connect(){
   $dbConn=new PDO('mysql:dbname='.$this->db.';
   host='.$this->dbHost.'',$this->dbUser,$this->dbPass);
   return $dbConn;
  }


 public function dbQuery($query,$params){
    if($dbConn=$this->connect()){
    try{
    $stmt=$dbConn->prepare($query);
    $result=$stmt->execute($params);
    var_dump($stmt);    
    $row=$stmt->fetch(PDO::FETCH_ASSOC);

    }catch(PDOException $e){

     die('Error: '.$e->getmessage());
    }


    if(!empty($row)){

     return $row;
    }else{

    return false;
    }

  }
 }//**END dbQUERY

 public function dbInsert($query,$params){

   if($connDB=$this->connect()){
      try{
            $stmt=$connDB->prepare($query);
            $result=$stmt->execute($params);

            return true;
            }catch(PDOException $ex){
            /*THIS COULD ALSO BE SENT TO A LOG FILE*/
            $ex->getmessage();

            } 
   }

 }//**End dbInsert


} 
//**-----------END CLASS DB-----------


class render {

 public function regUser(){

  if(!isset($_POST['submit']))
  { //SHOW FORM

   ?>
    <div id="accform">
      <form method="post" action="#" name="newacc">
        <span>First Name</span><input type="text" name="fname" id="FName">
        <span>Last Name</span><input type="text" name="lname" id="LName">
        <span>Email</span><input type="text" name="email" id="<Mail">
        <span>Password</span><input type="text" name="password" id="PWord">

        <input type="submit" name="submit" value="Sign Up">
      </form>
    </div>
  <?
  } else{
      //process information
      $user= new user;
      $user->createNew();

    }
  }//*END register function


 }
//END OF RENDER CLASS
share|improve this question
    
Just a few tips: global + class = bad code. Likewise for $_POST in a class: if a method needs data, or an instance of a class, then that method should take arguments. When writing an abstraction layer (ie PDO wrapper) ask yourself: What would make people want to use my class? In this case, not much. If you make something private think about the implications when you extend from that class. You are using an email address: validate the input! filter_var($email, FILTER_VALIDATE_EMAIL). Lastly: coding standards matter –  Elias Van Ootegem Jul 8 at 11:35
    
Oh, one more thing: there are many, many db-abstraction libs out there, see how they do things. If nothing else, they can inspire you to do better. But to be frank, I think it best to use proven technology. If you don't like something, fork the code, and create a pull request, YaY for open source ;-P –  Elias Van Ootegem Jul 8 at 11:39

2 Answers 2

Question 1: You've partially got the idea. It's acceptable syntax, but it won't work, you need to use $this which references the current class. However, you're not allowing for dependency injection, and keeping you class very tight together. Here's a way that's commonly seen:

public function __construct(DB $database) 
{
    $this->data = $database;
}

Question 2: Perfect read from the docs.

Class members declared public can be accessed everywhere. Members declared protected can be accessed only within the class itself and by inherited and parent classes. Members declared as private may only be accessed by the class that defines the member.

Since you're not using those encryption methods outside of the internal scope, a visibility of private would be acceptable.

Question 3: I advise not making your own database access class because...

  1. Homemade database access objects almost always lack some sort of functionality.
  2. They are very likely to be insecure
  3. They're not community reviewed (most of the time)

I suggest you use someone else's maintained and secure code. They'll often have proper error handling too. Almost every PHP framework has one, and there's a dozen on GitHub. It's too hard to specify one, so I suggets you do your research.

Question 4: Yes. Avoid globals. Give yourself room to expand with a config file or a config class. Preferably the file, and then a class to interpret the data the file holds. Again, dependency injection.


Other than that, there are tons of formatting issues. It's all in the readability of the code. Here ya go:

If you choose to use a framework (i.e. Zend), many come with documented standards and styles that you should follow with that framework.


If you can, get rid of your encryption functions and just use password_hash. It's a lot safer and easier to use.


Separate your classes into different files, and keep the HTML out of the PHP. Mixing the two makings things a hell-of a lot harder to read!

share|improve this answer
    
Thanks Alex. That's great input. I'm trying to demonstrate my current level of proficiency with OOP by not using frameworks on my projects. Which is why im trying to create my own DB class. I have an external file for the config called config.php which is brought in by the require_once. I'll try messing around with implementing that in a class.Are those constants you're passing into the contructor ? PHP--there's always something more to learn :) –  steveBK Jul 7 at 18:58
1  
Ah, well in that case, I highly suggest taking a look at some of the smaller frameworks' source code - just to get a feel for the ways they do it! :) Reading others' code is one of the best things you can do –  Alex L Jul 7 at 19:00
    
Just one more thing Alex, are those constants you're passing to the constructor? Am I missing something ? Would I have to define constants of the same name pointing to my config.php file ? –  steveBK Jul 7 at 20:24
1  
Oh! That's actually PHP's type hinting. It's basically saying, pass a parameter of the object type DB in the variable $database –  Alex L Jul 7 at 20:30
    
"private would be acceptable": I agree, in the sense that it does what it needs to do in this particular case. But an abstraction layer should be extendable. protected might be the better option –  Elias Van Ootegem Jul 8 at 11:40

I'm sorry I don't really know the answer to number 1, 2, 3. I'm a beginner too, but I think I know a better solution for number 4. As far as I know, global variables are bad. So you should create a new class for storing config data, like this:

class DatabaseConfig
{
    private $configOne = null;
    private $configTwo = null;

    public function __construct()
    {
    }

    public function setConfigOne($value)
    {
        $this->configOne = $value;
    }

    public function setConfigTwo($value)
    {
        $this->configTwo = $value;
    }

    public function getConfigOne()
    {
        return $this->configOne;
    }

    public function getConfigTwo()
    {
        return $this->configTwo;
    }
}

Then modify your database class's constructor, to accept this config class, and do operations according to the config.

class DB
{
    public function __construct(DatabaseConfig $config)
    {
        if ($config->getConfigOne() == true)
        {
            // do something
        }
        if ($config->getConfigTwo() == 123)
        {
            // do something
        }
    }
}

Then you can pass the config as a parameter to your Database Class, sample usage:

$cfg = new DatabaseConfig();
$cfg->setConfigOne(true);
$cfg->setConfigTwo(123);

$db = new DB($cfg);

Also, it would be better if you organize the classes into different .php files with different namespaces.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.