A method to insert a database entry for a newly registered account

Question

I've started to learn an OOP and I've built a class called accountactions, and I would like to know if I did a good job writing it.

The class is in the file: accountactions.class.php.

<?php

class accountactions(){

    public function register($login, $password, $email){

        //Zapisujemy dane wysłane formularzem
        $this->username = mysql_real_escape_string($login);
        $this->password = mysql_real_escape_string($password);
        $this->email = mysql_real_escape_string($email);

        //Hash password
        $this->password = md5(sha1($this->password));

        $db->simplequery("INSERT INTO radio_users(id, username, password, email) VALUES('', '$this->username', '$this->password', '$this->email')");

    }


}

?>

The register.php file:

<?php

    require_once("accountactions.class.php");

    $account = new accountactions();

    $account->register('samplelogin', 'samplepassword', 'sample@email');

?>

And I am having some issues with this fragment:

$db->simplequery("INSERT INTO radio_users(id, username, password, email) VALUES('', '$this->username', '$this->password', '$this->email')");

How do I join my db class to my account class?

I'd like to keep a model where I can do something like:

$account->register('$_POST['login']', '$_POST['password']', '$_POST['email']');

Unless there is a better way to do this.

Im newbie in OOP so any tips and guidelines are appreciated.

Answer 1

Organization, while not exactly code related, is still very important. Especially when it comes to OOP and especially when you're just getting started. If your code is organized correctly, there is no need for namespaces such as ".class". I spent a solid week perusing different frameworks before finally deciding on one I liked. Then I spent another month fine tuning and tweaking it. Originally, my organizational scheme came from the Zend framework.

application
    configs
    controllers
    models
    views
data
    logs
    cache
public
    images
    css
    js

There's a bit more to it, but I got the most important bits. This scheme specifically caters to an MVC framework, so it may not apply to your situation. You don't have to use this scheme, you don't even have to use a popular one. Hell, you can create your own to fit your own needs, and sometimes that is required. However, a good organizational scheme is hard to find and the popular ones are usually popular for a reason. And one thing a good organizational scheme should do is allow you to avoid namespacing your files. In my scheme my class files would go in one of two folders. The "controllers" or the "models". Knowing these were the only locations for class files means I don't have to namespace them.

Sorry for the mini-rant, let's move along now. I don't really see anything wrong with the OOP, but then again, there's not really much going on here. I do have a few suggestions that might help though.

First, your class is incomplete. You have three class properties and have not declared them. You should always declare your properties, otherwise you risk allowing public access to private properties, such as the password property.

class accountactions() {
    private
        $username,
        $password,
        $emal
    ;

You should avoid internal comments as they just add clutter to your code. If your code is self-documenting then most comments become unnecessary anyways. Everything else can be relegated to doccomments. So, for instance: "Hash password" is pretty obvious from the context and is unnecessary.

Its a minor violation, but still, there is a violation of the "Don't Repeat Yourself" (DRY) Principle here. As the name implies, your code should not repeat itself. You use the same function on each of your method's parameters in order to sanitize them. The easiest way to do this would be to change the way the method accepts its parameters so that you instead accepted an array. Then you could loop or walk over the array to sanitize its contents. For instance:

public function register( Array $credentials ) {
    array_walk( $credentials, 'mysql_real_escape_string' );

    $this->username = $credentials[ 'username' ];
    //etc...

However, there is a problem with this. For one, these parameters should not be sanitized. Validated: yes; Sanitized: no. You validate for registry; You sanitize for login. If you sanitize them now then you could be changing what that user provided without informing them, which could cause all kinds of confusion. The better thing to do here would be to verify that it has no illegal characters and have the user change it if necessary. Once you validate it, it has essentially been sanitized and you can use it like normal.

Another thing you might consider is using custom sanitizing/validating filters. mysql_real_escape_string() was a very nifty tool when you had nothing better to use, but now there are options such as filter_input() and filter_var(). The former should only be used when accessing raw user input from a form directly, the latter when accepting unknown variables. In this instance, even though you will probably get these values from a form, you will use the latter because your method only sees it as a variable and doesn't know for sure where it comes from.

if( filter_var( $username, FILTER_SANITIZE_STRING ) === $username ) {
    $this->username = $username;
}
if( filter_var( $email, FILTER_VALIDATE_EMAIL ) ) {
    $this->email = $email;
}

You might want to consider adding whitespace for legibility on lines that are too long. The rule of thumb here is 80 characters, including indentation. Both PHP and SQL are very lax when it comes to whitespace, so don't be afraid to use it.

$db->simplequery( "
    INSERT INTO radio_users( 
        id,
        username,
        password,
        email
    ) VALUES (
        '',
        '$this->username',
        '$this->password',
        '$this->email'
    )
" );

Now, as to how you can incorporate your $db class into your accountactions class. The easiest, and best, way is to inject it. This can either be done during construction, or via some sort of setter. I'm going to assume that your database model is still MySQL because I can't find anything to contradict it, so I'll use that to type hint it. By the way, you should really upgrade your MySQL to MySQLi or PDO and take advantage of their prepared statements if you haven't already done so.

public function __construct( MySQL $db ) {
    $this->db = $db;
}

/** or as a setter */
public function setDb( MySQL $db ) {
    $this->db = $db;
}

Hope this helps!

Answer 2

Several problems that you should be aware of:

1. You are using the soon to be deprecated mysql functions. Instead, use mysqli or pdo.
2. You aren't salting your passwords.
3. You should not use md5 for password+salt encryption. Use sha-512 or 256.
4. You have no error handling.

I can not stress the first three problems enough. I would also question the wisdom of storing user credentials at all. You might consider using something like openid.

Now for some general observations:

Consider a camel case naming convention for classes. These are easier to read. e.g. AccountActions instead of accountactions. I also think it's a good idea to use namespaces (if you are using php 5.3 or higher). Put each class in separate file and use a PSR-0 classloader.

You get alot more benefit from OOP if you have interaction between re-usable objects. When starting out, it's tempting to take code and simply dump it into some kind of god class. Consider how these classes work together and how the User class is re-usable.

<?php
class User
{
    private $name;
    private $email;
    private $password;
    private $salt;

    //assume there are methods set/getName, etc

    /*
     * @param string $clearPassword
     */
    public function setPasword($clearPassword)
    {
        $this->salt = base_convert(sha1(uniqid(mt_rand(), true)), 16, 36);
        $this->password = crypt($clearPassword, $this->salt);
    }
}

class Registrar
{
    private $pdo;

    public function __construct(PDO $pdo)
    {
        $this->pdo = $pdo;
    }

    /*
     * @param User $user
     * @throws RegistrarException
     */
    public function registerUser(User $user)
    {
        //todo: All values should be possibly 
        //filtered and certainly validated

        try {
            $sql = "INSERT INTO users set name=?, email=?, pass=?, salt=?";
            $stmt = $this->pdo->prepare($sql);
            $stmt->execute(array(
                $user->getName(),
                $user->getEmail(),
                $user->getPassword(),
                $user->getSalt()
            ));
        } catch (PDOException $e) {
            throw new RegistrarException('insert error', null, $e);
        }
    }
}

class RegistrarException extends Exception {}

Then we use it this way:

<?php
$user = new User();
$user->setName($_POST['name']);
$user->setEmail($_POST['email']);
$user->setPassword($_POST['password']);

try {
    $pdo = new PDO($dsn, $dbuser, $dbpass); //get values from a config
    $registrar = new Registrar($pdo);
    $registrar->registerUser($user);
} catch (Exception $e) {
    //handle the exception accordingly. i.e. log it and display error page
}

Answer 3

I think you have three options:

1. Create a setter: public function setDatabase($database) { ... } and everytime the accountactions class is instantiated call it to provide the database functionality.
2. Make a constructor that will require your database instance: `public function __construct($database) { ... }
3. Create some kind of static factory class/method (e.g. Configuration class) that would instantiate database class - then you could use both above solutions to provide the database or even use the static factory class/method inside the accountactions class (which is the least preferable as far as OOP is concerned)

The most flexible (e.g. for testing) is the 1st solution.

The 2nd solution is what I did in my last project.

The 3rd solution would give you the benefit of writing less code because you wouldn't have to write constructors with the database parameter or instantiate your database class before setting/constructing a particular class. But this is a killer for testing - you should then provide some mechanism of e.g. injection a database prototype factory into the class with the static factory method. Then the real instantiation of database objects would be delegated to a prototype factory that could be replaced by the mock while testing.

In my last small PHP project I've created class Database:

class Database {

private $log;
private $db_host;
private $db_user;
private $db_password;
private $db_name;
private $con = NULL;

public function __construct($db_host, $db_user, $db_password) {
    $this->log = ...;
    $this->db_host = $db_host;
    $this->db_user = $db_user;
    $this->db_password = $db_password;
    $this->db_name = NULL;
}
...
public function connect($db_name = NULL) { ... }
public function close() { ... }
public function dropTable($tableName) { ... }
public function rawQuery($query, $safeQuery = false) { ... }
...
}

And I provided it in a constructor of the particular class:

class OtherClass {
    ...
    public function __construct($db, $param, ...) { ... }
    ...
}