- 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
- validationalthough the signup class can validate the data itself, most of the validation methods must be mademoved into a separate class or done withinfor the same classcode reuse. In the latter caseAlso, it makes a little sense to usecall separate methods for validation. It will take less writing and lead to less errors if all validations would be eprformedperformed 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
<?php
session_start();
require_once 'autoload.php';
$http = new HTTPTransport();
$validator = new Validator();
$database = new DatabaseConnection('localhost', 'database', 'root', 'password');
$user = new SignUp($database, $validator);
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>redirectWithMessage('signup', 'error', $e->getMessage());
}
$users$user->addUser();
$http->returnWithError>redirectWithMessage('login', 'success', 'Registered successfully!');
which implies that your set methods validate the data like this
public function setEmail(string $email)
{
if (!$this->validator->validateEmail($email)) {
throw new SignUpValidationError('Incorrect email');
}
if ($this->findUserByEmail($email)) {
throw new SignUpValidationError('Email already exists');
}
$this->email = $email;
}
