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 1 down vote favorite

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?

share|improve this question

closed as unclear what you're asking by Phrancis, Hosch250, Mast, Ethan Bierlein, ChrisWue Mar 10 at 8:01

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question.If this question can be reworded to fit the rules in the help center, please edit the question.
    
What is the connection between users and categories? I would create 2 different classes, one that knows about users and one that knows about categories. The users class should access only the users table and associated tables if necessary, the categories class should access the categories table and associated if necessary. – pacmaninbw Mar 9 at 15:59
    
@pacmaninbw methods just for an example. the main question here is how to return data? what is the right way. because I want to be able to understand if the method retruns true and the data at the same time. also should I do foreach/while inside the class or outside? – Erdem Ece Mar 9 at 16:03
2  
As of right now it appears you are only returning booleans, are you asking how to change the code to also return data from the database? – Phrancis Mar 9 at 16:07
    
Pin Crash's question is valid, a way to simplify your functions is to return $query->rowCount() rather than check if there is a count. Return 0 if the outer if fails. Problems I see, the class does not have a closing }, $result is only defined because it is still in the unclosed class declaration (use $category->result). – pacmaninbw Mar 9 at 16:17
up vote 1 down vote accepted

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.

share|improve this answer

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