Improving a constructor

Question

Alright, I'm working on making a Member class for my website. I want to make it as optimized as possible. Currently, the constructor can take ($resource) either an int (to grab info from one member in the database, based on id) or an array of ints (to grab multiple members from the database, and store them in an array member variable).

I would like to know if there are any inprovements I can make with my block of code below, before I move on to creating more parts of my website. What could be changed to make it better? Is there a better layout I should follow for doing this kind of thing?

public function __construct($resource) {
  global $database;
     if (is_string($resource) || is_int($resource)) {
            $resource = (int)$resource;
   $query = $database->query("SELECT * FROM members WHERE member_id = {$resource} LIMIT 1");
   $row = $database->get_row($query);

         foreach ($row as $key => $value) {
               $this->field[$key] = $value;
            }
  } else if (is_array($resource)) {
   $query = $database->query("SELECT * FROM members WHERE member_id IN(" . implode(",",$resource) . ")");
   while ($member = $database->get_row($query)) {
    $this->member_list[$member['member_id']] = $member;
   }
  }
 }

Answer 1

Some thoughts:

• Globals are bad
• Database calls should be isolated to a data access layer, ideally of the ORM variety so that you aren't writing manual SQL.
• What if $resource is "ddd", do you want the member with a member_id of 1 ?
• You aren't protecting against SQL injection.
• Shouldn't your member_list be creating new Member objects (or whatever this class is called) rather than simply appending the row data?

Answer 2

Firstly, don't do work in a constructor. Secondly, there are many packages out there that do exactly what you're trying to do, and do it VERY well.

What you're trying to write is called a Model. It's a class that mirrors a table in the database. Use a mature ORM (Object Relational Mapper) package such as Propel or Doctrine for automatically generating classes based off of database schemas. I personally recommend Propel over Doctrine (though they are both great packages).

Also, I'd recommend that you use the php framework Symfony that integrate Propel as it's ORM (again, you can use Doctrine as an alternative ORM with Symfony).

Lastly, don't use globals. Write a class around any resource that access. Making a static call to retrieve it's singleton instance such as Database::getInstance() is a much preferred way of accessing the database (or any other) resource.

Best of luck