Object-oriented shopping cart

Question

I have just started to try and learn 'OOP' but it appears I'm doing this wrong, according to the people on Stack Overflow. The code below is far from object-orientated, but I'm finding it hard as I'm self teaching my self and everyone does everything differently.

I'm building a shopping cart and I need to be able to have both the admin and user login, so obviously both user types can use the same login and logout functions. I'm guessing I could use inheritance there, but I just need some one to tell me what I have done wrong or if anything is done right to confirm this.

Below is my create user page and my database and user class:

//Sanitize User Input
$username = $database->sanitize_admin_input($_POST['username']);
$password = $database->sanitize_admin_input($_POST['password']);

//Check if user already exists.
$exists = $database->check_user_exists($username);

//Creates a salt, then passes the salt in to create a secure password.
$salt = $database->createSalt(); 
//Create the password hash using the salt.
$password_hash = $database->make_password_hash($password,$salt);

//Add Admin user.
$admin->add_admin($username, $password_hash, $salt);

Admin Class

<?php

require_once('../includes/class.database.php');

class Admin
{
    public $id;
    public $username;
    public $password;
    public $first_name;
    public $last_name;

    public $number_of_users; //Holds the number of users in the DB.

    function __construct()
    {
        $this->number_of_users = $this->number_of_users();
    }

    private function number_of_users()
    {
        global $database;
        $results = $database->query("SELECT COUNT(*) FROM users");
        $rows = mysqli_fetch_row($results);
        $number = $rows[0];
        return $number;
    }

    public function add_admin($username,$password_hash,$salt)
    {
        global $database;
        $result = $database->query("INSERT INTO users (username, password, salt ) VALUES ('{$username}','{$password_hash}','{$salt}')");
        return $result;
    }
}//END OF CLASS
?>

Database Class

<?php
require_once('config.php');

class Database
{
    private $connection;
    public $last_query; //stores the lastquerys, measn we can call this from the DB.

    function __construct()
    {
         $this->open_connection();
    }

    public function open_connection()
    {
         $this->connection = mysqli_connect(DB_SERVER, DB_USER, DB_PASS, DB_NAME);
         if(!$this->connection)
         {
              $output =  "A connection to the database has failed, please check the config file.";
              $output .= "<br />" . mysqli_connect_error();
              die($output);
         }
    }

    public function close_connection()
    {
         if(isset($this->connection))
         {
              mysqli_close($this->connection);
              unset($this->connection);
         }
    }

    public function query($sql) //Takes in a paramater (sql query)
    {
         $this->last_query = $sql;
         $result = mysqli_query($this->connection,$sql);
         $this->confirm_query($result);
         return $result;
    }

    private function confirm_query($result)
    {
         if(!$result)
         {
              //IF the result failes, takes the connection error and displays that, on a new line it displays the last query used.
              $output = "This query has failed" . mysqli_error($this->connection);
              $output .= '<br />' . $this->last_query;
              die($output); //Kills the script and outputs the error message
         }
    }

    public function sanitize_admin_input($data)
    {
        $data = trim($data);
        $data = filter_var($data, FILTER_SANITIZE_STRING);
        $data = ereg_replace("[^A-Za-z0-9]", "", $data );
        return $data;
    }

    public function check_user_exists($username)
    {
        global $database;
        $results = $database->query("SELECT * FROM users WHERE 'username' ='{$username}'");
        $row_cnt = $results->num_rows;
        return $row_cnt;
    }

    public function make_password_hash($password,$salt)
    {
        $hash = hash('sha256', $password);
        $password_hash = hash('sha256', $salt . $hash);
        return $password_hash;
    }

    public function createSalt()
    {
         $string = md5(uniqid(rand(), true));
         return substr($string, 0, 9);
    }
}

$database = new Database();
?>

Index.php

    require_once('class.userAction.php');
    require_once('class.database.php');
    $database = new Database(); 

    $userOne = new userActions($database, 'marinello12','2312');
    $userOne->create();

Answer 1

First of all don't use global variables or constants. Use comments to explain your intention not your code. Put every class in a separate file.

Afterwards you have to decide if you want to use your Admin/User as a representation of a concrete admin/user (keeping the data of one user) or as a kind of manager providing access to all users and just use simple arrays as your concrete user.

Mixing the public properties with the (actually static) CRUD-method is a bad design and will lead to trouble.

As soon as you have done your first step I would recommend to replace your database abstraction by PDO or any other major library and don't reinvent the wheel.

---

Edit - Just a draft how I would structure your class if I wouldn't use PDO and create my own database abstraction:

<?
class Database
{
    private $conn;
    public function __construct( /*connection parameter*/)
    {
        $this->conn=...
    }

    public function query($sql)
    {
        $result=...
        if ( mysql_error()!='') throw SqlException(...);
        return $result;
    }

    public function fetch($result) {...} //fetch_object or fetch_array

    public function execute($query) 
    {
       ...
       return  mysql_affected_rows();
    }

    public function lastId() {...}
}

Separate file:

<?
public class UserManager    //Or UserRepository
{
    ...
    public function __construct($connection) {...}

    public function create ($username, $plainTextPassword, $isAdmin, ...)
    {
        $this->checkUniqueName($username);
        list($password,$salt)=$this->hash($plainTextPassword);
        $sql="INSERT ...";
        $success=$this->conn->execute($sql);
        if ($success!=1) throw SomeException(...);
        //maybe return a User instance
    }

    private function checkUniqueName($username)
    {
        $query="SELECT COUNT(*) FROM ...";
        ...
    }

    private function hash($password)
    {
        ...
        return array($password,salt);
    }

    public function delete(User $user) {...}
    public function update(User $user) {...}
    /*
     @return User
    */
    public function findByUsernameAndPassword($username,$password) {...}  //e.g. used for checking the login.
}

And maybe a class for the User instance:

<?php
class User
{
     private ...

     public __construct(array $row) //your database row
     {
         ...
     }
     //some getter/setter
}

Answer 2

but it appears I'm doing this wrong

I'm afraid you're right about this. OOP isn't only about a different way of writing things. It's about different thinking! In OOP every object is represented by a class. These objects have some properties (firstname, lastname...) and actions that are used for a mutual communication = methods.

Some examples from your code:

Method number_of_users() - how did you come to the conclusion it belongs to the class Admin? It doesn't make sense. You don't get number of users from the Admin, but rather from the database (so it should be a part of the user repository or something like that).

Method make_password_hash($password,$salt) - again, why do you think the Database should be responsible for this? It should be a part of a class either containing multiple algorithms or class responsible for a custom way of hashing.

Forget about the procedural php programming, try to think in a completely different way. Try to see the relations between objects, try to separate them. You need to learn how to separate different layers, how to decide what belongs to which object etc. Yes, it can be a little hard in the beginning, but it's definitely worth it.

After few years of experience with OOP I can't really imagine how would I create an e-shop for instance without classes and object oriented way of thinking....

On the more positive note, you got something right - your methods are fairly short :) That's a good thing!

Answer 3

First of all you are using globals (global $database;) which is something you should pretty much always avoid. The purpose of OOP is exactly to protect data therefore having something global, especially a database connection, is not a really good idea. I suggest you to take a look at the Dependency Injection design pattern which will suggests you to pass the database connection class Database to the classes that uses them Admin via their constructor.

Second, in the Database class you are mixing some application logic with the sanitize_admin_input, check_user_exists, createSalt and make_password_hash functions. You should stick to the Single Responsibility principle in this case: one class server one and only one purpose. You expect a database class to give an abstraction level over the database, therefore making queries and retrieving results easy for the user of that class. I would never expect a Database class to create a random salt, for example. Therefore I suggest you to move each of those functions to their proper classes.

sanitize_admin_input => Admin (static)
check_user_exists => User (static)
createSalt / make_password_hash => Hash or Auth

On a side note you should stick with any naming convention you decided in the first place and you really shouldn't mix camelCase methods with underscore_named methods in check_user_exists and createSalt for example. Either checkUserExists and createSalt or check_user_exists and create_salt.

Finally I suggest you to remove the instantiation of the object $database in the Database class definition file. Instantiation and class definition should always be in two separated files.

Answer 4

You asked about why not to use global variables.

Procedural problem: Every piece of code was written for one purpose, and was tied to that project. Lifting this code for use elsewhere is time consuming.

So we made objects. The purpose of objects is like best described as 'lego bricks'. A 'lego brick' has connectors but also properties that make it unique, also there can be many of these bricks. But the key property of a 'lego brick' is it acts dependently of all other bricks.

So why not use 'global variables'? Objects should act like 'lego bricks' independent of each other. In order to access the properties of an object you should use 'private variables' that are accessed via functions of that object. Also commonly referred to as 'getters' and 'setters'.

private variable;

get_private_variable() set_private_variable()

You can now have complete control of these variables in just one place. Any time they are set you can check they are set to what they should be expected to be set to. And when you get them you are not giving the person access to the variable to change itself but rather a copy of it forcing them to call the 'set' method if they want to change it.

(*If you need to get and set without anything else having access in between then you implement this notion of 'locks'.

private lock_variable; private variable;

This is a huge topic as well that could not be fully covered here but the basic idea is in order to change something you need the key that only person can have at one time.

)

Model-View-Controller

This is just a fancy way of saying: Look here, change there and remember what.

Model; where all the data is stored and managed.

View; what the user sees.

Controller; where all operations should be.