2
\$\begingroup\$

I've just begun creating a class to get User information, and I'd like for it to be improved.

The global variable $oConni is the database connection.

class UserInfo {

public $id, $oConni;

public function __construct($id, $oConni) {
    $this->id = $id;
    $this->oConni = $oConni;
}

public function getInfo() {
    $userInfo = array();
    $cSQL = "SELECT name, last_name, email, use_gravatar, pic_profile FROM users WHERE id = ?";
    $stmt = $this->$oConni->prepare($cSQL);
    $stmt->bind_param('i', $this->$id);
    $stmt->execute();
    $stmt->store_result();
    $stmt->bind_result($name, $lastName, $email, $gravatar, $picProfile, $admin);
    if ($stmt->fetch()) {
        $userInfo['name'] = $name;
        $userInfo['lastName'] = $lastName;
        $userInfo['email'] = $email;
        $userInfo['gravatar'] = $gravatar;
        if($gravatar == 1){
            $size = 500;
            $grav_url = "http://www.gravatar.com/avatar/".md5(strtolower(trim($email)))."?s=" . $size;
            $userInfo['picProfile'] = $grav_url;
        }else if(strpos($picProfile, 'http') === 0 || strpos($picProfile, 'https') === 0){
            $userInfo['picProfile'] = $picProfile;
        }else{
            $pic = 'http://www.example.com/'.$picProfile.'?id='.RAND(0,999999);
            $userInfo['picProfile'] = $pic;
        }
    }
    $stmt->close();

    return $userInfo;
}
}
$userInf = new UserInfo($cmntUser, $oConni);  
$userData = $userInf->getInfo();
print_r($userData);
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Structure

I don't really like returning a magic array. The caller has to guess or read up on the keys, which is annoying. I know that this is often done in PHP, but I think a better approach would be to move the retrieving of information to the constructor, store the result in class fields, and then access those fields with explicit getter (or if you don't want to completely rewrite your class, store the array as field, and add getters for each key of the array).

This could be used like this:

$userInfo = new UserInfo($userId, $dbConnection);  

<div>Name: <?php echo htmlentities($userInfo->getName()); ?></div>
<div>Email: <?php echo htmlentities($userInfo->getEmail()); ?></div>
[...]

For debug purposes you could still add a toString method with returns a string of all array values.

If you do it like this, not only is your class a lot easier to use (without having to read the documentation and/or knowing of the inner workings of the class), the class now actually is more than a wrapper for a method that could just as well be a static function.

Misc

  • You bind a result to $admin which never exists.
  • your fields should be private.
  • naming: oConni could be pdo or database, cSQL could be query, and grav_url gravatarUrl (you are using camelCase everywhere else).
  • your indentation and spacing is sometimes off, you can use an IDE to fix this easily.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.