current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite 
if (isset($_POST['submit'])) {
    if (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token']) {
        $errors = array();
        $error = 0;

        if (!preg_match('/^[A-Za-z](?=[A-Za-z0-9_.]{3,20}$)[a-zA-Z0-9_]*\.?[a-zA-Z0-9_]*$/i', $_POST['username'])) {
            $errors[] = 'Your username must start with a letter (A-Z) and be between 3-20 characters and may only contain alphanumeric characters (A-Z, 0-9 _ ) or a period (1) (".")';
            $error = 1;
        }

        $STH = $DBH->prepare("SELECT username FROM users WHERE username = ?");
        $STH->execute(array($_POST['username']));

        if ($STH->rowCount() > 0) {
            $errors[] = 'The username is already taken!';
            $error = 1;
        }

        if (strlen($_POST['password']) < 3) {
            $errors[] = 'Your password must be longer than 3 characters!';
            $error = 1;
        } else if ($_POST['password'] != $_POST['passconf']) {
            $errors[] = 'You didn\'t verify your password correctly!';
            $error = 1;
        }

        if (!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)) {
           $errors[] = 'The e-mail is not valid!';
           $error = 1;
        } else {
            $STH = $DBH->prepare("SELECT email FROM users WHERE email = ?");
            $STH->execute(array($_POST['email']));

            if ($STH->rowCount() > 0) {
                $errors[] = 'The email is already taken!';
                $error = 1;
            }
        }
    }
}

This is my current validation for a registration page it works, but I'd like to improve it, give me some tips

share|improve this question
    
I have some doubt on the username regex –  Xavier Combelle Nov 21 '11 at 0:05
    
How could I improve it/change it? Why? –  John Nov 21 '11 at 7:10
    
given some explaination in my answer –  Xavier Combelle Dec 4 '13 at 20:58
add comment

2 Answers

up vote 1 down vote accepted

You have two long if blocks:

if (isset($_POST['submit'])) {
    if (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token']) {
    ...
    }
}

Why not combine them?

if (isset($_POST['submit']) && (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token'])) {
    ...
}

also the way you've structured this, you don't need $error, to see if there are errors you can simply check empty($errors) (true => ok, false => not ok).

Other than that, your code looks fine.

share|improve this answer
    
Thanks, but is really 2 queries needed for my username check and e-mail check? That's what I mainly had thoughts about –  John Nov 21 '11 at 7:10
    
The reason I didn't combine the If statements is because if someone mess with the token / or tries to submit a form from another place, I want to log that someone tried a CSRF attack. If I combine them, I would log everytime I don't submit the form and load the page :P –  John Nov 21 '11 at 7:18
    
@John Well you can get rid of username altogether, and use the email as username. But if you want them both, and they are supposed to be unique then yes you do have to have the two queries... –  Yannis Nov 21 '11 at 11:50
    
Shouldn't I be able to query with a OR in the SQL statement, then try fetch the object or something? –  John Nov 21 '11 at 15:18
    
@John There are ways to combine the two queries. Easy ways where you won't know which of the two clauses failed (just that one of them did as a simple OR query) and more creative ways where it'd be possible to get all the info you want. But the two queries are extremely low cost, combining in any way won't have any distinguishable performance benefit and there are chances that you actually stress the db a little more. Anyways, if it's the sql you wan't reviewed, I'd suggest you add detailed table structures and their expected size on the question, too little info for a good answer as it is. –  Yannis Nov 21 '11 at 15:26
add comment
up vote 0 down vote

I think the correct regex for the login should be

if (!preg_match('/^[A-Za-z][A-Za-z0-9_.]{2,19}$/i', $_POST['username'])) {
            $errors[] = 'Your username must start with a letter (A-Z) and be between 3-20 characters and may only contain alphanumeric characters (A-Z, 0-9 _ ) or a period (1) (".")';
            $error = 1;
        }

EDIT I think that the current regexp allows more thant 30 letters

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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