Return to Revisions

This code is rather good if you look at each separate line, but rather bad from the organizational standpoint.

Each separate code snippet is indeed nearly flawless but it's the way they are glued together is wrong. SignUp is, so to say, a "Diety Object" as it's not as omniscient as a God Object, but definitely it does way too much:

• connects to a database
• validates input
• directly interacts with a database
• directly interacts with HTTP client
• it formats the output

All these tasks must be delegated to separate classes or even components.

You need to learn about separation of concerns and some basic MVC. Some pointers to get it right:

• a signup process is not a database connection. You never extend the former from the latter, just like you never extend a Human class from a Potato class in order to make a Human to eat a Potato. Besides, what if your page will require some other database interaction and use another class extended from database connection? It will create two database connections. And another class and so on. Which will eventually lead to the infamous "Too many connections" error. A connection has to be made only once and then passed to all classes as a constructor parameter
• validation can be made into a separate class or done within the same class. In the latter case it makes little sense to use separate methods for validation. It will take less writing and lead to less errors if all validations would be eprformed in the set methods. Just like setPassword() for some reason does not following the other methods' example.
• either way, the validation code should never format the HTML output. It is not only makes a designer to dig into your PHP code to change one class to another but also a severe case of a code repetition. Imagine you'd like to add some fancy icon to your flash messages. Going to go though all the PHP code changing it in hundreds lines, seriously?
• neither should it otherwise interact with a client. the validation code should only return a user interactions error while it should be conveyed to a user by some other mechanism
• the actual insert query can be made into a separate class but can be kept in this one. Either way, it must be fed with all the data required, not trying to get some data magically from a superglobal array. Remember that at some point you'd want to register users from a command line utility where no variable like ['HTTP_CLIENT_IP'] will ever be present
• I am sure you already can feel uneasiness when calling signUpPage method which is absolutely alien to all the other code. There must be another class responsible for the HTTP interaction to which this method would belong
• all the output must be formatted in a completely different component called "View". It should have predefined templates for this kind of flash messages and the job of the HTTP component is just to define the message type and text

I would strongly suggest to look at some established PHP frameworks, namely Symfony or Laravel, in order to see how the prosess is done and how the matters are usually get separated from each other.

The refactored version could be like this

<?php

session_start();

require_once 'autoload.php';

$http = new HTTPTransport();
$database = new DatabaseConnection('localhost', 'database', 'root', 'password');
$user = new SignUp($database);

try {
    $users->setName($_POST['username']);
    $users->setEmail($_POST['email']);
    $users->setPassword($_POST['password'], $_POST['password2']);
    $users->setUserAgent($_SERVER['HTTP_USER_AGENT']);
    // same for the IP address and related HTTP headers
} catch (SignUpValidationError $e) {
    $http->returnWithError('signup', 'error', $e->getMessage());
}
$users->addUser();
$http->returnWithError('login', 'success', 'Registered successfully!');