Changing a user's password

I wrote this class for the project I'm currently working on. It's basically used to change a user's password. I'd like to know your thoughts on the basic structure/design of this class and how it can be improved, or if you'd have written it differently.

```
class Recover_password{
//class by sami
private $conn;
private $password_one;
private $password_two;

function initialize_connection($conn){
if($conn instanceof PDO){
$this->conn = $conn;
}else{
echo "Connection problem accured";
}
}

function is_posted(){
if(isset($_POST["change_password"])){
$this->is_empty();
}else{
echo "nothing's being posted!";
}
}

private function is_empty(){
foreach($_POST as $post_value){
if(!empty($post_value)){
$is_empty = false;
}else{
echo "Field/s are empty";
break;
}
}

if(isset($is_empty)){
$this->set_values();
}
}

private function set_values(){
$this->password_one = strip_tags(stripslashes(trim($_POST["password_two"], '"')));
$this->password_two = strip_tags(stripslashes(trim($_POST["password_one"], '"')));

if($this->password_one == $this->password_two){
if( (strlen($this->password_one) > 10) or (strlen($this->password_two) > 10) ){
$this->password_one = md5($this->password_one);
$this->change_password_to_new();
}else{
$this->show_message("The password you provided is too weak");
}
}else{
$this->show_message("Please make sure the passwords match");
}
}

private function change_password_to_new(){
$insert_pass_query = $this->conn->prepare("update registered_users set user_password = :user_password where username = :username");
if( $insert_pass_query->execute

basic structure/design

I would not echo in a class, but throw an exception instead. That way, the calling class can decide how to handle these problems, and your class will thus be more flexible.

Your class also does two things: It changes a password, and it redirects. I would leave the redirection to the calling class as well (but this is probably a matter of taste).

is_empty

The is_empty function doesn't seem to be working correctly.

If only one of the two fields is filled in by the user, Field/s are empty is echoed, but set_values is called anyways.

In this case it doesn't really matter, because it will not do anything. But then your function could be simplified to:

private function is_empty(){
    foreach($_POST as $post_value){
        if(empty($post_value)){
            echo "Field/s are empty";
            break;
        }
    }

    $this->set_values();
}

An alternative way would be to move that check into set_values, where all the other checks also take place:

if ($this->password_one == "" || $this->password_two == "") {
        $this->show_message("Field/s are empty");
    } else if($this->password_one != $this->password_two){
        $this->show_message("Please make sure the passwords match");
    }else if (strlen($this->password_one) show_message("The password you provided is too weak");
    } else {
        $this->password_one = md5($this->password_one);
        $this->change_password_to_new();
    }

Note that I rearranged the if statements for more readability, and used the shoe_message method for all messages, instead of using echo.

Naming

• is_posted doesn't seem like an optimal name, although I have problems coming up with a better name.

• PHP class names should be in CamelCase

Misc

• I would not change a user password. If a user wants their password to be "



\' why not let them? In addition to annoying users, this could also lead to real problems, because you have to use those exact filter functions everywhere where the user supplies a password, as otherwise some users will not be able to login.

• in set_values, the or (strlen($this->password_two) > 10 check is unnecessary, as password_one and password_two are equal and password_one is already checked.

• security: md5 isn't really considered good enough anymore. If possible, you should use bcrypt (password_hash in PHP) or pbkdf2 instead.

StackExchange Code Review Q#79973, answer score: 4