OOP PHP best practices

Question

I've worked with PHP for a few years now and have a degree of understanding about classes / objects, but I've never written my own until now.

In that vein then, I'm looking for a bit of confirmation of the code below and that I am doing things properly before I go any further. Any advice / critique is greatly appreciated.

class People { protected $people = array(); public function __construct() { $this->people = $people; $defaults = array( 'model_type' => 'post_type', 'post_type' => 'post' ); $options = wp_parse_args(get_option('my_options'), $defaults); $this->model = $options['model_type']; $this->type = $options['post_type']; } public function get_people() { switch($this->model) { case 'user' : $people = get_users(); if(!empty($people)) { foreach($people as $person) { $new_person = new Person( $person->ID, $person->first_name, $person->user_email ); $this->people[] = $new_person; } } break; case 'post_type' : $args = array( 'posts_per_page' => -1, 'orderby' => 'post_title', 'order' => 'DESC', 'post_type' => $this->type, 'post_status' => 'publish', ); $people = get_posts( $args ); if(!empty($people)) { foreach($people as $person) { $new_person = new Person( $person->ID, $person->post_title, get_post_meta($person->ID, '_email_address', true) ); $this->people[] = $new_person; } } } return $this->people; } } class Person { public $id; public $name; public $email; public function __construct( $id, $name, $email ) { $this->id = $id; $this->name = $name; $this->email = $email; } } 

Answer 1

protected $people = array(); public function __construct() { $this->people = $people; 

This may not be doing what you want. After doing that, I would expect $this->people to equal null, as you just overwrote the previous value of array() with the value of a previously undeclared variable.

 $this->people[] = $new_person; 

Which makes that line create an array the first time that it is reached.

$this->model = $options['model_type']; $this->type = $options['post_type']; 

This is bad style. You should always declare class properties before using them so that you can give them the proper scope. This also makes them quicker to access (it checks for declared variables first).

 $new_person = new Person( $person->ID, $person->post_title, get_post_meta($person->ID, '_email_address', true) ); 

This looks suspicious to me. Is the post title really a person's name? If so, you should comment and explain why this would be true. Otherwise, someone will later create a bunch of posts whose titles aren't people and wonder why posts are showing up under a list of people.

Alternately, if post titles are not names of people, then this type is misnamed.

You also might want to consider extending your class rather than using a switch in get_people. Then you could have a People class and a Post class, and each could define its own get_people.