Validating username fields and showing CAPTCHA

Question

I'm new to coding, and especially to OOP PHP. I'll highly appreciate if you expert programmers could review following code and give me your opinion to make it better. Also, how you see this code, is it too horrible and ugly?

<?php
include_once("config.inc.php");

class user{

    var $mysqli;
    var $username;
    var $userid;

     function __construct($mysqli) {
         session_start(); 
         $this->mysqli = $mysqli;
    }

    function valid_length($input){
        if ((strlen($input) > 4 ) AND (strlen ($input) < 16) ){
            return true;    
        } else {
            return false;
        }   
    }

    function valid_email($email){
        if (preg_match("/^[0-9a-z]+(([\.\-_])[0-9a-z]+)*@[0-9a-z]+(([\.\-])[0-9a-z-]+)*\.[a-z]{2,4}$/i" , $email)) {
            return true;
        } else {
            return false;
        }
    }

    function alpha_numeric($input){
        if (eregi("^([0-9a-z])+$", $input)){
            return true;        
        }else{
            return false;       
        }   
    }

    function username_exists($username){

        if ($stmt = $this->mysqli->prepare("SELECT username FROM users WHERE username=?")) {
            $stmt->bind_param("s", $username);
            $stmt->execute();
            $stmt->store_result();
            $count=$stmt->num_rows;
            $stmt->close();
        }
        return ($count > 0 ? true : false);
    }

    function signup($username, $password, $email, $capt_error){

    //validate username.
    if(!$this->valid_length($username)){
            $this->signup_error = "Username should be 5 - 15 characters. <br />";   
    }else if(!$this->alpha_numeric($username)){ 
            $this -> signup_error ="Only letters, numbers";     
    } else if($this->username_unallowed($username)){ 
            $this -> signup_error ="Not allowed.";
    } else if ($this->username_exists($username)){
            $this -> signup_error ="Username already exists. <br />";
    }

    //validate password.
   if (!$this->valid_length($password) ){   
            $this -> signup_error .="Password should be 5 - 15 characters. <br />";
        }

    //validate email.
    if (!$this->valid_email($email) ){  
            $this -> signup_error .="Invalid email.";
    }else if($this->email_exists($email)) {
            $this -> signup_error .="Email exists.<br />";  
        }

    //captcha error
    if($capt_error == 1){
    $this -> signup_error .="Invalid captcha. <br />";  
    }

    //no any error, proceed.
    if ($this->signup_error == ""){
        $hashed_password = $this->hash_password($password);
        if ($this->insert_new_user($username, $enc_password, $email)){
        $_SESSION['username']=$username;    
        header("Location: welcome.php");
        }else {
        $this->signup_error = "Unknown error. <br />";
        }       
    }

  }


}
?>

Answer 1

I've got a few suggestions, but all in all this code is a long way from being horrible and ugly.

You're using PHP 4-style class members (var $mysqli) when you could be using PHP 5 members with access control (public, private etc.) The same goes for methods, which should be qualified with public or private to indicate who should be able to have access to them.

In general, you should make all data members of a class private unless there's a really good reason for them to be public. The $mysqli handle, for example, is purely an implementation detail and no client of the class should need to have access to it.

The $userid member doesn't seem to be used at all, that I can see. I'm assuming it's an incomplete implementation.

The biggest problem is it's not clear to me what the user class does. Aim to give each class a small, well-defined set of responsibilities. At the moment the class appears to be a collection of all the code that is used for working with users, whether or not it has anything to do with the concept of a user as such. For example, the alpha_numeric function is a completely generic concept that you might want to use elsewhere, so it doesn't belong on the user class. Nor does valid_email. The valid_length method should probably be private.

When you design a class, it can be helpful to start from a description of just the public interface. Decide what you want people to be able to do with instances of your class. Aim for something that can be described simply and will be obvious to other people working with the code. Any functionality that doesn't fit into this interface should be in a separate class. Write documentation and / or unit tests to this interface, and you'll see whether it makes sense to a user. Once you've got the public interface fixed, you can add whatever private methods you need to make that work, but resist the temptation to change the public interface unless you've discovered something really important that you didn't think of before.

Another small detail is that your indentation is very inconsistent. I don't know if that's an artifact of copying and pasting to the site.

Answer 2

Why not use the built-in PHP e-mail validation?

filter_var($email, FILTER_VALIDATE_EMAIL)

Why do

if (lol())
  return true;
else
  return false;

when you can do

return lol();

Same goes for lol() ? true : false which you do too.

Furthermore the signup_error stuff is ugly. I'd suggest exceptions. At least make signup_error into an array instead, and just add new elements when you get an error. You can join() the array when outputting.

Also, you risk $count not being defined if $stmt fails.