PHP: Learning OOP

Question

Below is one of the first classes I've built. I started creating my functions like so:

public function getCourseInfoByID($id) { } 

Requiring the user to pass a course ID. I've recently modified it to assign the course ID (and $course_info - an array to reference each element from the DB) in the constructor, and I'd like to get some feedback if I'm doing it properly (I figured $course->getCourseInfo(); was much better than $course->getCourseInfoByID(1);):

class Course {
    var $course_id;
    var $course_info;

    // The constructor just sets the database object
    public function __construct($mysqli, $course_id = NULL) {
        global $error;

        $this->mysqli = $mysqli;

        // If course_id is present (referencing a course instead of creating one, let's validate it and assign the course_id to the object)
        if($course_id != NULL) {
            if(! $course_info = $this->getCourseInfo($course_id)) {
                $error[] = "Invalid Course ID";
                setError();
                return FALSE;               
            }

            $this->course_id = $course_id;
            $this->course_info = $course_info;
        }
    }

    public function getCourseInfo($id = NULL) {

        if($id == NULL) {
            $id = $this->course_id;
        }

        $result = $this->mysqli->query("SELECT courses.*, students.first_name, students.last_name, course_types.name as course_type_name
                                    FROM courses as courses
                                    JOIN students as students on students.id=courses.instructor_id
                                    JOIN course_types as course_types on course_types.id=courses.course_type_id
                                    WHERE courses.id='$id'");

        $course_info = $result->fetch_array(MYSQL_ASSOC);

        // If found, return the student object
        if($course_info) {
            return $course_info;
        } 
        return FALSE;
    }

    public function displayCourseInfo($id = NULL) {
        global $error;

        if($id == NULL) {
            $id = $this->course_id;
        }

        // HTML to display each element of $this->course_info[]
        // acccessible anytime by doing: $course->displayCourseInfo() in the script
    }

}

Answer 1

Going from top to bottom:

Constructor

public function __construct($mysqli, $course_id = NULL) {

What kind of object is '$mysqli'? You can probably type-hint it.

    global $error;

Globals are bad. Forget about them (like you were told).

    $this->mysqli = $mysqli;

    // If course_id is present (referencing a course instead of creating one, let's validate it and assign the course_id to the object)
    if($course_id != NULL) {
        if(! $course_info = $this->getCourseInfo($course_id)) {
            $error[] = "Invalid Course ID";
            setError();

Setting errors is so c-style. Use exceptions.

            return FALSE;           

Not even sure this does work. If it does - don't do it. Throw an exception. See this discussion about return-values in constructors.

        }

        $this->course_id = $course_id;
        $this->course_info = $course_info;

You might cast them to the corresponding data type. For example intval($course_id).

    }
}

I would refactor this to (or something likish):

public function __construct(MyDBLayer $db, $courseId)
{
    if($db == NULL)
    {
        throw new InvalidArgumentException('$db must not be null');
    }

    if($userId == NULL)
    {
        throw new InvalidArgumentException('$courseId must not be null');
    }

    $courseInfo = $this->getCourseInfo($courseID);
    if($courseInfo == NULL)
    {
        throw new MyCustomException("No Course found for id " + $courseID);
    }

    $this->dbAdapter = $db;
    $this->courseId = intval($courseId);
    $this->courseInfo = $courseInfo;
}

Or even make it a static method load or a data mapper:

public static function load(MyDBLayer $db, $courseId)
{
    // do all the stuff as in the constructor
    return new Course($courseId, $courseInfo);
}

The other functions

Pretty much the same as for the constructor. Don't use global, use exceptions instead. Don't write the html code for displaying in this class. Every class should only have one responsibility. In this case: representing a course, not displaying it.

Answer 2

Good stuff:

• Passing the DB connection to your constructor, this is dependency injection and is encouraged as it makes objects less tightly coupled and it makes unit testing easier.
• The class appears to be only implementing code to deal with the area for which it's directly responsible. There's always a temptation in OOP to add more code to an object to easily facilitate additional features, but you should only add code that you need, and you certainly should not add code that's not directly related to the area of responsibility of the class.

Bad stuff:

• global $error Global state is BAD. It should be avoided at all costs
• public properties: $course_id and $course_info are public, meaning that any external entity can access them and change their values at any time. An object is responsible for its own state, it shouldn't allow outside entities access it except through controlled channels (getters and setters). Make your properties private or protected, and provide getters as needed.
• No type hinting: You can (and should) use PHP5's type hinting features to prevent inappropriate data being passed into your class methods. For example there's nothing stopping someone from passing a variable into your class that isn't a mysqli database connection.