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.

I've a feeling I'm overdoing my Auth class, and that it could be done in a simpler and more understandable way.

Could you give me advice on this, please?

This is for my learning process and I just want to learn. This is why I am 'reinventing the wheel'.

class Auth
{

    protected $pdo;
    protected $session = null;
    protected $error = array();

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

    /**
    * 
    */
    public function authenticate_user($password,$email)
    {
        try
        {
            $select_user = $this->pdo->prepare('SELECT * FROM users WHERE user_email = :email LIMIT 1');
            $select_user->execute(array(':email' => $email));
            $user_data = $select_user->fetch(PDO::FETCH_ASSOC);
            if (password_verify($password,$user_data['user_pass']))
            {
                $this->set_session($user_data);
                return true;
            }
            else
            {
                $this->error['authenticate_user'] = 'Incorrect email or password';
            }
        }
        catch (PDOException $e)
        {
            echo $e->getMessage();
        }
    }

    /**
    * 
    */
    public function set_session($user_data)
    {
        if ($user_data)
        {
            $this->session = $user_data;
        }
    }

    /**
    * 
    */
    public function unset_session()
    {
        if (isset($this->session))
        {
            $this->session =  null;
        }
    }

    /**
    * 
    */
    public function get_session()
    {
        if (isset($this->session))
        {
            return $this->session;
        }
        else
        {
            return false;
        }
    }

    /**
    * 
    */
    public function is_logged_in()
    {
        if (isset($this->session))
        {
            return true;
        }
        else
        {
            return false;
        }
    }

    /**
    * 
    */
    public function get_error($key)
    {
        if (isset($this->error[$key]))
        {
            return $this->error[$key];
        }
        else
        {
            return false;
        }
    }

}

And on the login.php:

  if (!empty($_POST))
  {
      $response = $Auth->authenticate_user($_POST['password'],$_POST['email']);
  }

  session_start();

  if (isset($response))
  {
      $_SESSION = $Auth->get_session();
  }

  print_r($_SESSION);

  echo $Auth->get_error('authenticate_user');

And in the file that loads all the other files (at the top of everything):

if (isset($_SESSION)
{
  $Auth->set_session($_SESSION);
}
share|improve this question
add comment

2 Answers 2

  • It seems to me like it would probably make sense for the members of Auth to be static, though this may be for some reason undesirable to you. Change it as you see fit.

  • Your style contrasts with the industry standard in that you are putting braces on their own lines for control structures (i.e. if). If this is purely a personal project then it's your choice, but if you're committing this to an existing code base or making it available for public use then it may do you some good to read on a style guide, such as the excellent PEAR Coding Standards.

  • Your database query performs a SELECT *. This is, in general, not a good idea. Since you're only using the one column, consider changing it to SELECT "user_pass".

  • It doesn't make much sense to me to have a set_session() method. Do you really mean to export it as public?

  • It also doesn't make sense to be saving your SQL row for later retrieval. (Ignore this point if password_verify() does something special to the $user_data parameter.) This is an Authentication class, not a general User class.

  • You don't need the isset() check in unset_session().

  • Aside: You used an inconsistent amount of leading spaces in your question. Ideally, start all code blocks with *exactly* four spaces. I can't tell if your last block really does use only two spaces for indentation (thus diverging from your previous code blocks). If it does, then you should correct it so that your indentation is consistent throughout your project.
share|improve this answer
2  
contrasts with the industry standard in that you are putting braces on their own lines: how could this be an industry standard? Pretty standard here and here (sect. 4.4) –  Alex L Jul 7 at 20:45
    
@Alex I meant to clarify but I guess I forgot to when I was typing it out. Thanks for pointing it out. –  Schism Jul 7 at 20:47
    
Ah, that makes more sense :) –  Alex L Jul 7 at 20:49
add comment

The code is understandable, but it can be simplified.

Instead of:

public function unset_session()
{
    if (isset($this->session))
    {
        $this->session =  null;
    }
}

You can simply:

public function unset_session()
{
    $this->session =  null;
}

Instead of:

public function get_session()
{
    if (isset($this->session))
    {
        return $this->session;
    }
    else
    {
        return false;
    }
}

You can simply:

public function get_session()
{
    return $this->session;
}

(if you don't mind that it returns a falsy value (null) instead of a real false)


Instead of:

public function is_logged_in()
{
    if (isset($this->session))
    {
        return true;
    }
    else
    {
        return false;
    }
}

You can simply:

public function is_logged_in()
{
    return isset($this->session);
}

Instead of:

public function get_error($key)
{
    if (isset($this->error[$key]))
    {
        return $this->error[$key];
    }
    else
    {
        return false;
    }
}

As of PHP 5.3, using the short ternary operator, you can simply:

public function get_error($key)
{
    return $this->error[$key] ?: false;
}
share|improve this answer
1  
$this->error[$key] on an invalid $key will result in a notice. It will also return a falsey null instead of exactly false, which could be undesirable. –  Schism Jul 7 at 20:46
    
You're right, I amended my answer –  janos Jul 7 at 20:56
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.