2
\$\begingroup\$

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
    }

}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ I agree with all of that except static. Static should almost never appear in object oriented code. It binds the code tightly making testing more difficult. It is the equivalent of referring to a global function (it is completely procedural code). \$\endgroup\$ – Paul Jan 3 '12 at 2:12
  • \$\begingroup\$ @Paul: I totally aggree. However using static is the easy (but not that good) way to seperate the loading functionality from the actual data representation. The prefered way are data mappers of course. \$\endgroup\$ – Fge Jan 3 '12 at 2:15
  • \$\begingroup\$ I'd agree that the only sensible way to tell the outside world that the constructor failed is to throw an exception. However, I'd contend that in general exceptions should be used sparingly. They introduce unpredictable program flow control (they've been described as fancy gotos, while I wouldn't go that far they can lead to confusion if overused), they also have a significant performance impact compared to other flow control structures. I'd reserve exceptions for exceptional events, such as input failing basic sanity checks. For sane but incorrect input I'd return false instead. \$\endgroup\$ – GordonM Jan 5 '12 at 10:18
  • \$\begingroup\$ Exceptions do not pose any signifanct performance issue (check stackoverflow for various posts about that). Exceptions should only be thrown if something goes wrong, having no db-result is probably no exception. Invalid parameters sure are (mostly because it's the programmer's fault). And if the program flow is unpredictable with exceptions you're probably using them in a wrong manner. Actually they make error handling a lot easier at the place of where they can be handled accordingly. \$\endgroup\$ – Fge Jan 5 '12 at 14:03
0
\$\begingroup\$

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.
\$\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.