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.

I made a class that connects to my DB and inserts some values. Is it secure or how can I protect this further from injections? The object declaration will come from variables with POST from a form, after being validated, ofc. Just want to know if this is secure.

That, and also, should I make a method for every query I need? Or is there a better and secure way?

<?php
include "db/db_info.php";
/*$DBServer
$DBUser
$DBPass
$DBName*/
class WorkDB {
    private $server;
    private $user;
    private $pass;
    private $name;
    private $conn;

    public function __construct( $server, $user, $pass, $name ) {
        $this->server=$server;
        $this->user=$user;
        $this->pass=$pass;
        $this->name=$name;
    }

    public function tryconn() {
        $this->conn = new mysqli(  $this->server, $this->user, $this->pass, $this->name );

    if ( mysqli_connect_error()) {
        die( '*************Connection Error (' . mysqli_connect_errno()  . '):'
                .mysqli_connect_error() );
    }


    else echo 'ok';

}

public function query_register( $user, $pass, $email ) {
    $stmt = $this->conn->prepare( "INSERT INTO `users` (`username`, `password`, `email`) VALUES (?, ?,?)" );
    $stmt->bind_param( "sss", $user, $pass, $email );
    $stmt->execute();
    $stmt->close();
}


}//end of class


$a=new WorkDB( $DBServer, $DBUser, $DBPass, $DBName );
$a->tryconn();
$a->query_register( 'a', 'b', 'c' );


?>
share|improve this question
add comment

2 Answers 2

up vote 6 down vote accepted

What you did well

  • Storing sensitive information, such as the database connection parameters, separately from the source code is a good idea.
  • You used parameterized queries with placeholders, which are not vulnerable to SQL injection.

Things to work on

  • Your indentation is inconsistent.
  • tryconn() should not echo 'ok' on success, as that pollutes the output.
  • $a is a horrible name for your database handle. It's conventionally called $db or something like that.
share|improve this answer
    
The echo'ok' was just for testing purposes. But where should I put my queries? each in every method? And should I make a method that checks if the Sesion id is set? –  George Irimiciuc Jun 15 at 18:37
add comment
  1. I think tryconn() could use a better name. Perhaps just connect().
  2. Avoid using the die function in you code. Consider learning about the different types of exceptions (maybe implement an SPL) and throw an exception instead.

  3. As 200_success said, keep the view (the echo) out of your model class. Larger projects may require you to keep things separated.

  4. Look at your names here: WorkDB, tryconn, and query_register. Try and apply some sort of consistency to your code. Find a style that suits you and the project and stick to it. Here's one to get you started.

  5. query_register() has very limited functionality. Either make the class more specific, so that it only handles CRUD operations for users, or generalize it more, so that it's more of a query builder class.

  6. Comments such as //end of class do not really help the reader. In code where readers may have a hard time figuring out what a section does (algorithms, complex sequences, obscure functions, large functions, etc.) then comments are fantastic. If comments are just thrown in willy-nilly, then they're useless.

  7. $a=new WorkDB could use some breathing space around the assignment. Again, consistency and legibility are important.

  8. Lastly, as 200_success stated, $a is a pretty bad name for a variable. Again, find a coding style and stick to it. Many would say that variables should have some sort of meaning to their name.

share|improve this answer
    
Thanks, will keep in mind. –  George Irimiciuc Jun 15 at 20:54
add comment

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.