Deleting a user from a database

Question

I was hoping if anyone could tell me if I'm doing this correctly, I know there are other questions like this but everyone codes different and I just feel that I'm not doing this right. I'm trying to make a modular OOP site so all the files will be called through the one index file which will then route the user to the correct page. All the code below works correctly (I have tested it) but I was curious about a few things.

• Is this the correct way of using OOP and classes (mainly the DB connection)?
• Why (if it is correct) when I use Netbeans PHP that it doesn't pick up the functions? See images below.

db.php (DB connection file)

$host = "localhost";
$username = "root";
$password = "";
$database = "als";

$errors = new errors();

try {
    $db = new PDO("mysql:host=$host;dbname=$database", $username, $password);
} catch(PDOException $ex) {
    trigger_error($ex->getMessage());
    $errors->addLog($ex->getPrevious(), true);
    exit;
}

User.php (class)

class user {
    private $db = null;

    public function __construct() {
        global $db;
        $this->db = $db;
    }

    public function deleteUser($ID) {      
        if(isset($ID) && !empty($ID) && is_int($ID) == TRUE) {
            $deleteUser = $this->db->prepare("DELETE FROM `users` WHERE userid=?");
            $deleteUser->bindParam(1, $ID, PDO::PARAM_INT);
            $deleteUser->execute();
        } else {
            return FALSE;
        }     
    }
}

index.php (main file)

include 'config/autoloader.php';
include 'config/db.php';

$user = new user();

$user->deleteUser(1);

Answer 1

Why (if it is correct) when I use Netbeans PHP that it doesn't pick up the functions? See images below.

As you don't declare types in PHP, Netbeans has no way of knowing that $db is a PDO object. But you can tell Netbeans this:

/* @var $db PDO  */
private $db = null;

For further ways to Type-hint for PHP in Netbeans check out this post I wrote a while ago.

Is this the correct way of using OOP and classes (mainly the DB connection)?

I would pass the PDO object to the user class in the constructor instead of using global.

It looks cleaner, and it would make writing unit tests a lot easier as well (you could pass a connection to a test database without changing the code or mock the PDO class). It also has the neat side-effect that you are able to define the type and thus enable auto complete.

Misc

• classes should start with an upper-case letter (User).
• is_int($ID) == TRUE can be written as is_int($ID).
• I find it a bit odd that you return false for wrong parameters, but not for an unsuccessful delete in deleteUser.
• checking empty has the effect of not deleting the user with id 0. Is this on purpose? If yes, I would make this explicit ($id != 0), if no, just remove it, it doesn't do anything else that is_int wouldn't cover.

So I would probably rewrite it like this:

class User { // upper-case U
    private $db = null;

    public function __construct(PDO $db) { // defining a type allows for auto-complete and makes testing easier
        $this->db = $db;
    }

    public function deleteUser($ID) {      
        if(!isset($ID) || !is_int($ID)) { // no need to check empty. if needed, add $ID != 0
            return FALSE; // wrong parameters, return early
        }
        $deleteUser = $this->db->prepare("DELETE FROM `users` WHERE userid=?");
        $deleteUser->bindParam(1, $ID, PDO::PARAM_INT);
        return $deleteUser->execute(); // returns true on success, otherwise false

    }
}