Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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

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

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

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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

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

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

Source Link
Fge
  • 1.4k
  • 7
  • 14

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.