Database query inside a method of a class and return data [closed]

I’m trying to find out what is the right approach to return DB values from a class. This is what I came up with:

class Category {

  public $result;
  public $errors;

  public function __construct( $pdo) {
    $this->pdo = $pdo;
  }

  public function getCategoryNames () {

    $sql = "SELECT category_id, category_name FROM category";

    try {
      $query =  $this->pdo->prepare( $sql );

      if( $query->execute( ) ) {
        if( $query->rowCount() ) {
          $this->result = $query->fetchAll();
          return true;
        }
        return false;
      }
      return false;
    } catch ( PDOException $pe ) {
      trigger_error( 'SQL Error' . $pe->getMessage() );
    }
  }

  public function getUserDetails($userid) {
    $sql = "SELECT * FROM users WHERE user_id = :user_id";

    try {
      $query =  $this->pdo->prepare( $sql );

      if( $query->execute( array( 'user_id' => $userid ) ) ) {
        if( $query->rowCount() ) {
          $this->result = $query->fetch(PDO::FETCH_ASSOC);
          return true;
        }
        return false;
      }
      return false;
    } catch ( PDOException $pe ) {
      trigger_error( 'SQL Error' . $pe->getMessage() );
    }
  }


$category = new Category($pdo);
$category->getCategoryNames();
print_r($result);
$category->getUserDetails('1');
print_r($result);

Is this approach good? Or what is the proper way to do this?

Let me first say that any answer you get here is going to by biased by each person’s personal preference. What you’re doing isn’t dangerous or breaking any rules, so it’s ‘fine’ as far as that’s concerned. So with that in mind I’ll give you my opinion on a few things to consider, and you can decide from there.

Returning true and false to me is a bit old-school and doesn’t take advantage of php’s flexible variable type handling. I typically return FALSE if there’s an error, and return the results if not. The calling code can then just do a if($result !== FALSE) and you’re good to go.

The other thing to consider is the format of your return data.

One method is to dump it into some predefined objects. This is a very clean approach that makes code reliable and easy to read, but it isn’t always that easy to implement. I would only do this if you’re going to be creating a large and complex project.

If you’ve got a simple project, then returning arrays is the fastest and easiest way to go. It’s a little less reliable if your database is likely to change, so watch out for that. I typically massage my arrays in the method prior to returning them. Since in your example you’re breaking out specific operations, you can do custom data formatting in each. This is encouraged since it will make your calling code much easier to create, read, and maintain.

Lastly I wanted to talk about your error handling. What I prefer to do is return FALSE on error only, and return data every other time. You’ve chosen to return FALSE if no results are found which makes a bit of sense as it will simplify the calling code. The change I would suggest you make is to remove your trigger_error method and instead set the error message to a class variable that can be retrieved by your calling code. Your should then return FALSE after the catch. This will allow your code to fail more gracefully and also give you more control in your calling code as to how to handle the error (think MVC).

Of course I am making some assumptions as to what trigger_error does. I see you have an $errors variable so perhaps you’re already on that track. If you are then I recommend always returning FALSE from the method after an error, and adding hasErrors and getErrors methods to your class.