current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite
1

I'm learning to program with PHP, so I ask your help to check if my class user.php is ok.

include('password.php');
class User extends Password{

    private $_db;

    function __construct($db){
        parent::__construct();

        $this->_db = $db;
    }

  private function get_user_hash($username){
    try {
      $stmt = $this->_db->prepare('SELECT password FROM members WHERE username = :username AND active="Yes" ');
      $stmt->execute(array('username' => $username));
      $row = $stmt->fetch();
      return $row['password'];
      } catch(PDOException $e) {
      echo '<p class="bg-danger">'.$e->getMessage().'</p>';
    }
  }

  public function login($username, $password){
    $hashed = $this->get_user_hash($username);
    if($this->password_verify($password,$hashed) == 1){
       $_SESSION['loggedin'] = true;
      return true;
      }
    }

  public function logout(){
    session_destroy();
    }

  public function is_logged_in(){
    if(isset($_SESSION['loggedin']) && $_SESSION['loggedin'] == true) {
      return true;
      }
    }

  public function is_logged_admin(){
    if(isset($_SESSION['level']) && $_SESSION['level'] == 2 ) {
      return true;
      }
    return false;
    }  

  public function get_user_info($username){
    try {
      $stmt = $this->_db->prepare('SELECT * FROM members WHERE username = ?');
      $stmt->execute(array($username));
      return $stmt->fetch();
    } catch(PDOException $e) {
      echo '<p class="bg-danger">'.$e->getMessage().'</p>';
      }
  }

  public function members_count(){
    try {
      $stmt = $this->_db->prepare('SELECT * FROM members ORDER BY memberID');
      $stmt->execute();
      $count = $stmt->rowCount();
      print("$count");
      } catch(PDOException $e) {
      echo '<p class="bg-danger">'.$e->getMessage().'</p>';
      }
    }

  public function delete_user($id){
    try {
      $stmt = $this->_db->prepare('DELETE FROM members WHERE memberID = :id');
      $stmt->bindParam(':id', $_GET['id'], PDO::PARAM_INT); 
      $stmt->execute();
      } catch(PDOException $e) {
      echo '<p class="bg-danger">'.$e->getMessage().'</p>';
      }
    }

  public function update_account($username){
    try {
      $stmt = $this->_db->prepare('UPDATE members SET location = ?, bio = ?, relationship = ?, interested = ?, work = ? WHERE username = ?');
      $stmt->execute([$_POST['location'], $_POST['bio'], $_POST['relationship'], $_POST['interested'], $_POST['work'], $username]);
    } catch(PDOException $e) {
      echo '<p class="bg-danger">'.$e->getMessage().'</p>';
      }
  }

//close 
}
share|improve this question
1  
Now that your question looks on-topic, could you try to describe a bit more what the code is doing ? This would help the reviewer understand the code more easily. – Marc-Andre Mar 8 at 13:56
up vote 0 down vote accepted

In terms of the php code itself, the only thing I notice is your use of the $_POST superglobal in your class. This isn't wrong per say but I typically try to avoid use of $_POST in my objects, and instead pass the data in via parameters. Keeping all of your $_POST access in the same place makes it easier to manager changes, especially this the contents of $_POST is so easy to change. Same is also true for $_GET and $_REQUEST.

The same is also kind of true for the $_SESSION variable. It's not as important where it is, but stick to access within a limited set of locations.

Here's a few points to consider in terms of how you've constructed your class. They're not really php things, more good object construction practice.

Make sure you're always returning something from your methods. In many you have a return if things go right, but nothing if things go wrong. This will result in a NULL being returned but it's better practice to define this for reliable and consistent operation.

To take a more MVC approach, and to make your code easier to change and maintain, you shouldn't be echoing your errors in the class. Rather set the message to an error variable that can be retrieved by the calling code if needed, and then return FALSE. Echoing the error as it happens can be good during development but I recommend against it for production. If you still do want to see the error, best to handle it in the calling code so that you can easily change how the error is displayed. You could use a debugMode flag in the class if you want the best of both.

Your code indenting could really be improved.

private function get_user_hash($username){
    try {
        $stmt = $this->_db->prepare('SELECT password FROM members WHERE username = :username AND active="Yes" ');
        $stmt->execute(array('username' => $username));
        $row = $stmt->fetch();
        return $row['password'];
    } catch(PDOException $e) {
        echo '<p class="bg-danger">'.$e->getMessage().'</p>';
    }
}

Lastly I recommend using CamelCase for your method names. The use of lowercase and underscores to me is a bit oldschool and more difficult to read.

private function getUserHash($username)
share|improve this answer

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.