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 />";
 } 
 }
 }
}
?>

• Carson Myers: This isn't enough for a full answer, but I'd recommend not adding ?> to the end of files like this. If there is any extra whitespace after the closing PHP tag, it will be output to the browser when all you intended to do was include a class for internal (non-presentation) use. This sort of thing can be excruciatingly difficult to track down when you're not sure why there's an extra space in an arbitrary spot on a page.
• Aditya M P: There are many IF statements in this code - you don't want that. Most IF statements can be refactored as inheritance. Reading up on Polymorphism might help.

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.

• jcb: Thank you very much for spending your precious time to reply me. As I mentioned, I just have started to learn the OOP style PHP, and I was concerned about the overall structure. This is an example class to deal with users registeration, some of functions are missing in it as i just copied some parts.
• jcb: I just found out that if i use only function i can access it outside the class, for example on signup.php page, i can call $user->signup(); etc.

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.

• jcb: Thanks, my idea was that when i pass the variable to a function, for example "valid_email", the function valid_email will either return true value or false. Then based on the returned true or false, i can do something, for example if ($this->valid_email($email) ){}