patternphpModerate

PHP Register Form - OOP

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

phpregisteroopform

Problem

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.

Solution

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

Do one thing, and do it well

Don't Repeat Yourself (DRY)

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:

It is the Domain Model representing a User in the database along with some business logic (hashing the password)

It is getting data from the $_POST, coupling this class to the HTTP request layer of your application.

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

It is inserting data into the database.

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.

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.

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.

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:

```
create($_POST);
header($redirect);
}
else
{
$user =

Code Snippets

<?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);
    }
}

<?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}");
    }
}

<?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';
        }
    }
}

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

<?php

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

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

Context

StackExchange Code Review Q#77316, answer score: 15

Revisions (0)

No revisions yet.