0
\$\begingroup\$

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;
}

}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question \$\endgroup\$ – Simon Forsberg Jan 3 '15 at 16:11
  • 1
    \$\begingroup\$ Additionally, as we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. To value quality on Code Review, I have to downvote your question until these two minor issues are addressed. \$\endgroup\$ – Simon Forsberg Jan 3 '15 at 16:12
2
\$\begingroup\$
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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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