Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following Student class:

class Student {
    public $user_id;
    public $name;

    public function __construct($user_id) {
        $info = $this->studentInfo($user_id);
        $this->name = $info['name'];
        $this->is_instructor = $info['is_instructor'];
        $this->user_id = $info['id'];
    }

    public function studentInfo($id) {
        global $db;

        $u = mysql_fetch_array(mysql_query("SELECT * FROM $db[students] WHERE id='$id'"));
        if($u) {
             return $u;
        }
    }        

    public function getCoursesByInstructor() {
        global $db;

        return mysql_query("SELECT courses.*, course_types.name FROM $db[courses] as courses 
                            JOIN $db[course_types] as course_types ON courses.course_type_id=course_types.id
                            WHERE instructor_id='$this->user_id'");
    }
}

Ideally, I'd do:

$u = new Student(1);
$courses = $u->getCoursesByInstructor();

Does anyone have any critiques on this class? Also, how would you recommend me adding functionality to "add" a student?

share|improve this question
    
    
please try to have "descriptive" title. –  TheIndependentAquarius May 21 '12 at 5:47

2 Answers 2

up vote 14 down vote accepted

mysql_* functions

Stop using them, immediately! They are essentially deprecated, as they refer to MySQL before version 4.1. They are not removed as to not brake backward compatibility, but there's no point in using them.

The drop in replacement is myslqi_* functions, which are based around a newer MySQL driver that works better with MySQL 5.x. You can easily replace most references to myslq_* functions by just adding the extra i.

But a far better solution is PDO, an object oriented database agnostic interface. Since you are starting out in object oriented PHP you'll definitely appreciate the object orient part, and as for the database agnostic part, it means that you can easily switch databases (with minor modifications).

I would advise against an ORM solution at this point, ORM's are extremely useful but can be easily abused if you aren't 100% certain on what you are doing.

global $db;

Never ever do that again. Forget about the global keyword completely. It has its uses, but in most situations it's an extremely bad habit. The object oriented way to achieve the same effect would be:

class Student {
    private $db;

    public function __construct($db, $user_id) {
        $this->db = $db;

        ... 
    }
}

That's a minor example of Dependency Injection. In sort, you should feed your object what it needs, and not depend on something being available in the global namespace. You can't always be absolutely sure of that.

Public properties

Public properties violate the object oriented concept of encapsulation. You really don't want to do that, trust me :)

The most common object oriented workaround are setters and getters:

class Student {
    private $user_id;
    private $name;

    ...

    public function setUserID($user_id) {
        if( !is_int($user_id) ) {
            throw new InvalidArgumentException();
        }

        $this->user_id = $user_id;

        return $this;
    }

    public function setName($name) {
        if( !is_string($name) ) {
            throw new InvalidArgumentException();
        }

        $this->name = $name;

        return $this;
    }    

    public function getUserID() {
        return $this->user_id;
    }

    public function getName() {
        return $this->name;
    }

    ...

}

Wow! So much more code for such a simple thing! Before you get discouraged, notice that I also validate the input on the setters and I'm throwing exceptions if something is wrong. That's a very good reason to use setters instead of public properties, that way you have the ability of checking what the parameter is, and if it's not the one you are expecting you can deal with it accordingly.

In the instances above I'm throwing an InvalidArgumentException, which is one of the many pre-built SPL extensions. Learn about the SPL, it offers quite a few core object oriented interfaces and classes, such as iterators.

A far better way to validate common datatypes is the filter functions, I'm not using them in the example above to not confuse you any further. And, when you feel confident enough, you can minimize the code via magic Property overloading. Fair warning, don't go there unless you know exactly what you're doing.

But never ever again have public properties, always write getters and setters. And of course, the $db property from my previous example should have its own setter. No need for a getter, unless of course you actually need to pass around the database connection object.

Lastly, notice how I return $this on the setters? This isn't something you have to do, it's a trick to allow method chaining:

$student = new Student( ... );
$student->setUserID(1)->setName("Yannis");

Or, if you prefer a slightly more readable version:

$student->setUserID(1)
        ->setName("Yannis");    

Naming

As @jiewmeng writes, studentInfo should be renamed getStudentInfo. You should be very consistent on how you name functions, prefixing functions that return results with get and those that set info in the class with set is a very common practice, and one that will be familiar to most programmers, as it alludes to the concept of getters and setters I've described above.

Always return something

I don't really like it when functions don't return something:

public function studentInfo($id) {
    global $db;

    $u = mysql_fetch_array(mysql_query("SELECT * FROM $db[students] WHERE id='$id'"));
    if($u) {
         return $u;
    }
} 

Here, of course you return $u;, but only if($u). For that function, I would probably return false as well:

public function studentInfo($id) {
    global $db;

    $u = mysql_fetch_array(mysql_query("SELECT * FROM $db[students] WHERE id='$id'"));
    if($u) {
         return $u;
    }

    return false;
} 

Of course null is returned by default when you don't return something explicitly, but I find it a lot better when I know exactly what I should expect from a function return, and false will tell me that something failed long after I totally forget what goes on in the function. If you are in a situation where you can't think of something obvious to return, that might be a good place to throw an exception instead.

Adding a student

You could do a save() function:

class Student {

    ...

    public function save() {
        if(!this->validate()) {
            return false; 
        }

        return $this->store();            
    }

    private function validate() {
        // check all properties
        // if the required ones are filled and valid, return true,
        // else return false
    }

    private function store() {
        if($this->userExists()) {
            return $this->insert();
        }

        return $this->update();
    }

    private function userExists() {
        // determine if user already exists in the database
    }

    private function insert() {
        // insert user in the database
    }

    private function update() {
        // update existing user
    }    

    ...        
}

Of course that's more pseudo code, than code. But more or less, that's the common workflow. I'm assuming each function returns either true or false. Notice how the return value propagates from insert() or update() to save()?

In conclusion

You need to do a lot of research into the fundamental object orient concepts. The wikipedia article is as a good place to start as any, and most articles on the specific concepts have PHP examples. Your code is clean, and you do quite a few things correctly, but you are starting out in a new programming paradigm, one that is extremely useful and extremely vague. More often than not "best practices" are not obvious, and you'll need quite solid conceptual foundations to write object oriented code.

And please, consider writing phpDoc comments.

Happy new year!

share|improve this answer
    
-1 spoon-feeding [/proactive comment] –  Yannis Dec 31 '11 at 15:29

From an OOP standpoint, I dont think your properties should be public.

I think studentInfo should be renamed getStudentInfo

For database access its good to look at a ORM framework. I suggest Doctrine. If you want to use pure PHP, I think use PDO or MySQLi at least. Why use PDO on Nettuts

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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