DB abstraction, private methods in OOP PHP library

This library registers a new user.

Questions:

-
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.

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

-
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.?

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

```
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 '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) );
$passwo

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...

• Homemade database access objects almost always lack some sort of functionality.

• They are very likely to be insecure

• 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:

• http://www.php-fig.org/psr/psr-1/ - Basic Coding Standard

• http://www.php-fig.org/psr/psr-2/ - Coding Style Guide

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!

StackExchange Code Review Q#56357, answer score: 7