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.