5
\$\begingroup\$

I'm trying to build my first object oriented system. I'm not sure that my register function below is the best way of approaching this within OOP.

class User {
  public $username;
  private $email;
  private $password;

  public function register() {
   //Check if the username input is set.
   if(isset( $_POST['username'] )) {
     //Assign the variables.
     $this->username = $_POST['username'];
     $this->email = $_POST['email'];
     $unhashed_pass = $_POST['password'];

     //Hash the password   
     $this->password = password_hash($unhashed_pass, PASSWORD_BCRYPT);

     $sql = "INSERT INTO 'users' ('id', 'username', 'email', 'password') VALUES (NULL, '$this->username', '$this->email', '$this->password')";
     $results = mysql_query($sql);

     if($results) {
      //Query was successful
      echo "Success";
     } else {
      echo mysql_error();
     }
   }
  }
}

Please give me any feedback. I'm okay with PHP; I'm just trying to learn how to effectively use OOP.

\$\endgroup\$
14
\$\begingroup\$

When approaching object oriented programming, I always try to adhere to a few best practices:

  1. Do one thing, and do it well
  2. Don't Repeat Yourself (DRY)
  3. Favor composition over inheritance

There are many more, but I've found these three practices to be the most useful in creating modular code. That being said, let's explore how your class compares.

Do one thing and do it well

Your User class is doing many things:

  1. It is the Domain Model representing a User in the database along with some business logic (hashing the password)
  2. It is getting data from the $_POST, coupling this class to the HTTP request layer of your application.
  3. It is echo-ing a success flag, coupling this call to the standard output in PHP. Furthermore, if something goes wrong on insert, you can't handle that error. You should be throwing exceptions here instead. See Returning status codes from business layer for additional info
  4. It is inserting data into the database.
    1. Worse yet, you appear to have a SQL injection vulnerability since you are using double quotes and variable interpretation in PHP (see Little Bobby Tables for a humorous explanation). You want MYSQL Prepared Statements.

Don't Repeat Yourself

While no code is repeated inside this class, because it is doing multiple jobs it becomes difficult to reuse the code, which could lead to repetitive code later on.

Favor Composition over Inheritance

Your class doesn't inherit from anything, which is good. It doesn't really need to inherit from a super class. Composition (e.g. holding references to other objects specializing in a different but related task) won't help you here either because about 4 layers of an application have been mashed together into one class.

Splitting your class up into multiple classes

While this will seem like overkill at first, the following code will lay the ground work for a modular and testable application.

user.php

This is your domain model representing the database data. This will also contain methods that implement business logic, for instance, hashing the password. It is both data and behavior.

<?php

class User
{
    private const PASSWORD_BCRYPT = "...";

    private $username;
    private $email;
    private $password;

    public User()
    {
    }

    public User($username, $email, $password)
    {
        $this->set_username($username);
        $this->set_email($email);
        $this->set_password($password);
    }

    public function get_username()
    {
        return $this->username;
    }

    public function set_username($value)
    {
        $this->username = $value;
    }

    public function get_email()
    {
        return $this->email;
    }

    public function set_email($value)
    {
        $this->email = $value;
    }

    public function get_password()
    {
        return $this->password;
    }

    public function set_password($value)
    {
        $this->password = password_hash($value, User::PASSWORD_BCRYPT);
    }
}

users_repository.php

The "Users Repository" is a class whose sole purpose is to perform CRUD operations on the users table (Create, Read, Update and Delete). It implements an interface that you can use later on as a test harness.

<?php

interface IUsersRepository
{
    public function add(User $user);
}

class UsersRepository implements IUsersRepository
{
    private static $connection_string;

    public static function set_connection_string($value)
    {
        UsersRepository::$connection_string = $value;
    }

    private $connection;

    public UsersRepository()
    {
        // $connection = new mysqli(...);
    }

    public function add(User $user)
    {
        $statement = $this->connection->prepare('INSERT INTO "users" ("id", "username", "email", "password") VALUES (?, ?, ?, ?)');
        $statement->bind_param("id", NULL);
        $statement->bind_param("username", $user->get_username());
        $statement->bind_param("email", $user->get_email());
        $statement->bind_param("password", $user->get_password());

        if (!$statement->execute())
            throw new Exception("Execute failed: ({$stmt->errno}) {$stmt->error}");
    }
}

If the INSERT into the database fails, an Exception gets thrown. Failing to save data to the database is a hard stop. You really can't go any further, so an exception is the ideal way to communicate this catastrophic error (yes, failed database operations are catastrophic and your application should blow up in your face).

users_controller.php

This begins the Poor Man's MVC. The UsersController handles the HTTP request.

Notice that it has two constructors. The first one, which takes no arguments, instantiates UsersRepository as the object responsible for database operations. The second constructor accepts any object implementing the IUsersRepository interface, which is the interface implemented by UsersRepository. We'll use this later on as a test harness so we can write unit tests for this class without a database.

<?php

class UsersController
{
    private $repository;

    public UsersController()
    {
        $this->repository = new UsersRepository();
    }

    public UsersController(IUsersRepository $repository)
    {
        $this->repository = $repository;
    }

    public function register()
    {
        return new User();
    }

    public function register($params)
    {
        if (isset($params['username'])) {
            $user = new User($params['username'], $params['email'], $params['password']);
            $this->repository->add($user);

            return '/users/index.php';
        }
    }
}

users/create.php

Lastly, this file, which technically does too much here, is the actual "Create new User" page:

<?php

$controller = new UsersController();

if (isset($_POST))
{
    $redirect = $controller->create($_POST);
    header($redirect);
}
else
{
    $user = $controller->create(); ?>

<!DOCTYPE HTML>
<html>
    ...
    <body>
        <form method="post" action="./create.php">
            <input name="username" type="text" value="<?= $user->get_username() ?>">
            <input name="email" type="text" value="<?= $user->get_username() ?>">
            <input name="password" type="password" value="<?= $user->get_username() ?>">
            <button type="submit">Submit</button>
        </form>
    </body>
</html>

<?php } ?>

It instantiates a new UsersController. If the $_POST is set, then it passes that array into the $controller->create method, which performs the actual logic for extracting the user information from the HTTP request and inserting it into the database. Furthermore, it directs the application flow, for instance redirecting the user to another page upon successful insert (which prevents a double-POST of the same information as well).

The UsersController also has a method just called create() which takes no parameters and returns a User object, which gets displayed in the HTML form when the $_POST is not set.

Really, you'd want to take advantage of one of the many fantastic MVC frameworks out there for PHP for the users_controller.php and create.php files, because the MVC application should be responsible for routing an HTTP request to a controller, and then a specific PHP file for the view should provide the web page.

By splitting up your code into layers, you create a modular and testable application:

  • User - Business logic
  • UsersRepository - Data storage and retrieval
  • UsersController - Handle application flow
  • create.php - a not-so-perfect view

Testing your code

Notice that the UsersController has a constructor which accepts an instance of IUsersRepository. This is the interface implemented by UsersRepository. This allows you to write test code that uses a mock or stub user repository instead of going out to a database.

First, create the "mock users repository" class, which implements IUsersRepository

<?php

class MockUserRepository implements IUsersRepository
{
    public $added_users = array();

    public function add(User $user)
    {
        $this->added_users[] = $user;
    }
}

Next, create your unit test. In this case, I'm using PHPUnit:

<?php

class UsersControllerTest extends PHPUnit_Framework_TestCase
{
    public function addsANewUser()
    {
        $repository = new MockUserRepository();
        $controller = new UsersController($repository);
        $params = array(
            'username' => 'testable',
            'email' => 'test@example.com',
            'password' => 'abc123'
        );

        $redirect = $controller->create($params);

        $this->assertFalse(empty($repository->added_users)));
        $this->assertEquals('testable', $repository->added_users[0].get_username());
        $this->assertEquals('/users/index.php', $redirect);
    }
}

Now you can verify the behavior of your application in an automated and repetitive way. Covering your with unit tests allows you to refactor code or change business rules later on with confidence that you won't introduce bugs.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Wow. that's gave me a lot of food for thought. Thanks for the extensive answer, which may I say I was not expecting! \$\endgroup\$ – Ryan Hipkiss Jan 12 '15 at 15:27
  • \$\begingroup\$ Thanks, webdevryan. Also please take note of the SQL injection vulnerability in your code. That is the most important thing to fix, really. \$\endgroup\$ – Greg Burghardt Jan 12 '15 at 15:29
  • \$\begingroup\$ No worries, You've really gone to town on that code which I like. Rather than just telling me the answer (which you could of easily done) you've explained it too, which is always nice :) Yeah someone told me about those pesky SQL injections on another side, and will be using mysqli prepare from now on! \$\endgroup\$ – Ryan Hipkiss Jan 12 '15 at 15:43
  • \$\begingroup\$ Also, about the frameworks. I'm looking to use Laravel but wanted to get my knowledge clear down for OOP before moving onto it so I can understand what's going on fully, as clearly I have a lot to learn! \$\endgroup\$ – Ryan Hipkiss Jan 12 '15 at 15:55
  • \$\begingroup\$ Nice one - $redirect = $controller->create($_POST); should be $redirect = $controller->register($_POST); \$\endgroup\$ – waterloomatt Dec 3 '18 at 13:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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