What would be a better way of adding the functionality to "add" a student?

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?

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

StackExchange Code Review Q#7331, answer score: 13