How to properly use OOP in PHP with forms

Question

I am trying to wrap my head around how to properly use OOP in PHP when dealing with form submissions.

The example below is a form for editing course information. I have a class called Course which has methods for loading the class with information from the database and saving information into the database.

Everything works but I feel like I am doing this all wrong - any critique/suggestion would be nice.

edit_course.php

<?php
require_once 'header_includes.php'; 

$course = new Course();
$errors = array();

// get course ID
$cid = filter_input(INPUT_GET, 'cid', FILTER_VALIDATE_INT);
// check course ID
if ($cid && $course->load_data($cid)) {

    $filter = array(
        'code' => FILTER_SANITIZE_STRING,
        'name' => FILTER_SANITIZE_STRING,
        'hid-submit' => FILTER_VALIDATE_BOOLEAN
    );

    $inputs = filter_input_array(INPUT_POST, $filter);

    // check if form submitted
    if (!empty($inputs['hid-submit'])) {
        // show error messages if any
        $course->code = $inputs['code'];
        if (empty($inputs['code'])) {
            $errors['code'] = 'This is not a valid code.';
        }
        $course->name = $inputs['name'];
        if (empty($inputs['name'])) {
            $errors['name'] = 'This is not a valid name.';
        }
        $course->description = $inputs['description'];
        // save course information
        if (!count($errors)) {
            $saved = $course->save_data();
        }
    }
}

require_once 'header.php'; // header template
?>


<?php if ($course->error !== false) { ?>
    <div id="error"><?php echo $course->error ?></div>
<?php } ?>

<form name="form1" action="" method="post">
    <div>
        <label>Code:</label>
        <input type="text" name="code" value="<?php echo $course->code ?>" maxlength="10" size="8" />
        <?php if (array_key_exists('code', $errors)) { ?>
           <div class="error"><?php echo $errors['code'] ?></div>
        <?php } ?>
    </div>
    <div>
        <label>Name:</label>
        <input type="text" name="name" value="<?php echo $course->name ?>" maxlength="45" size="25" />
        <?php if (array_key_exists('name', $errors)) { ?>
            <div class="error"><?php echo $errors['name'] ?></div>
        <?php } ?>
    </div>
    <div>
        <input type="submit" value="Save" />
        <input type="hidden" name="hid-submit" value="1" />
    </div>
</form>

<?php
    require_once 'footer.php'; // footer template
?>

Course.class.php

<?php
if(!class_exists('Course')) {
    class Course {
        private $id = NULL;
        private $code = '';
        private $name = '';
        private $error = FALSE;

        public function __get($property) {
            if (property_exists($this, $property)) {
              return $this->$property;
            }
        }

        public function __set($property, $value) {
            if (property_exists($this, $property)) {
              $this->$property = $value;
            }
            return $this;
        }

        /**
         * Load course data from database
         *
         * @param   int
         * @return  bool 
         */ 
        public function load_data($id) {
            // SQL statement
            $sql = 'SELECT id, code, name FROM courses WHERE id = :id';

            try {
                $core = Core::getInstance();
                // 
                $sth = $core->dbh->prepare($sql);
                $sth->bindParam(':id', $id, PDO::PARAM_INT);

                if ($sth->execute()) {
                    // if course is found       
                    if ($row = $sth->fetch(PDO::FETCH_OBJ)) {
                        $this->id = intval($row->id);
                        $this->code = $row->code;
                        $this->name = $row->name;

                        return TRUE;
                    }
                } else {
                    throw new Exception('A problem has occurred while retrieving information from database.');
                }
                $sth->closeCursor();
            } catch (Exception $e) {
                $this->error = $e->getMessage();
            }

            return FALSE;
        }

        /**
         * Save course information 
                     *
         * @return  bool
         */     
        public function save_data() {
            // SQL statements
            if ($this->id) {
                $sql = 'UPDATE courses 
                        SET code = :code, name = :name
                        WHERE id = :id';
            } else {
                $sql = 'INSERT INTO courses(id, code, name) 
                        VALUES(:id, :code, :name)';
            }

            try {
                $core = Core::getInstance();
                // update/insert into database
                $sth = $this->core->dbh->prepare($sql);
                $sth->bindParam(':id', $this->id, PDO::PARAM_INT);
                $sth->bindParam(':code', $this->code, PDO::PARAM_STR);
                $sth->bindParam(':name', $this->title, PDO::PARAM_STR);

                if (!$sth->execute()) throw new Exception('A problem has occurred while updating database.');

                return TRUE;
            } catch (Exception $e) {
                $this->error = $e->getMessage();
            }

            return FALSE;
        }
    }
}
?>

Answer 1

The *_once() versions of require() and include() are relatively slower and should be avoided. When just including a few files it wont be noticeable, but the more you add, the more it will stack up. There are some exceptions, but headers and footers shouldn't be among them.

When using filter_input_array(), you don't have to worry about a returned value being empty. Empty is usually reserved for empty arrays. NULL and FALSE are the negative returns. NULL for elements that aren't set, and FALSE for any failures. As long as you are sure you checked for a certain element and the key exists in the array, then all you have to do is treat the return as a boolean.

if( $inputs[ 'hid-submit' ] ) {

Your code is rather repetitive here. This violates one of the core OOP principles: "Don't Repeat Yourself" (DRY). As the name implies, your code should not repeat. Typically this can be fixed by creating a function or loop. There are a couple of different methods here. The first is to use array_filter() to keep only those elements with FALSE values. Then, since your errors seem so similar (only the last part changes), you can use a foreach loop later to provide the proper contents. The second method is to just use a foreach loop from the beginning and check each element individually. The first method is cleaner, but less reusable. If the $errors array expands and the rest of the errors aren't similar enough, then you will have to revert to using the second.

//array_filter method
$errors = array_filter( $inputs, 'empty' );
//later
foreach( array_keys( $errors ) AS $error ) {
    echo "this is not a valid $error";//or throw error, log it, whatever...
}

//loop method
$errors = array();
foreach( $inputs AS $error => $value ) {
    if( ! $value ) {
        $errors[ $error ] = "This is not a valid $error";
    }
}

PHP has two versions of syntax. The first you are already familiar with. The second is typically only used in include files and is preferred because of its more non-PHP friendly legibility. To go along with this is PHP's short echo tags <?=, not to be confused with short open tags <?. If your version of PHP is >= 5.4 then they are enabled by default and entirely separate from the dreaded short open tags. Below I am using both the secondary syntax and the short echo tag. If your version is lower than required, even though this feature is still available, I would suggest you keep using the echo and keep the short tags off.

<?php if( $course->error !== FALSE ) : ?>

    <div id="error"><?= $course->error; ?></div>

<?php endif; ?>

What is the purpose of checking if the class exists already before creating it? If you are vigilant in the way you load your classes then there shouldn't be any issues with loading multiples of the same class. This is one of those few times where using the *_once() versions of require() or include() are ok. I'm assuming you are using an autoloader of some sort, just make sure that your class isn't already loaded there. You can do that with that same if statement you are currently using, or you can use that *_once() include.

Leaving your class properties empty when first declaring them will give them all NULL values by default. Its only necessary to give it an explicit first value if you are planning on using it immediately.

private
    $id,
    $code,
    $name,
    $error
;

Your getter and setter completely defies the purpose of having private properties. If you are just going to allow any application to modify your properties this way, you might as well make them public. It works the same and there's less code. Typically, a getter/setter is used to get/set only properties in a whitelist or array elements. The first is used for limited private property interaction, the second is primarily used in Views (MVC).

if( in_array( $property, $this->whitelist ) ) {
  return $this->$property;
  //or
  $this->$property = $value;
}

//or

if( array_key_exists( $property, $this->data ) ) {
    return $this->data[ $property ];
    //or
    $this->data[ $property ] = $value;
}

Returning $this is used to allow you to chain methods together ($class->m1()->m2()). Therefore, returning $this from your setter serves no purpose. You can't chain anything after setting a variable or property, so this makes no sense. Your setter doesn't need a return, you can just remove it.

Your internal comments are unnecessary. Actually, all internal comments are unnecessary. They just add clutter. If you have something you need to explain, you should limit it to the doccomments.

You should avoid defining variables in statements. Because of how easy it is to neglect the rest of the comparison (==, !=), it is sometimes quite difficult to determine if an assignment was meant or if this was just an accident. To avoid the confusion and potential backlash of improperly working code, you should just not do this. There are exceptions, such as loops, but generally this is considered bad practice. PHP is one of the few languages that still hasn't realized that. This feature actually lead to the development of the Yoda style to prevent accidental assignments in statements.

//bad
if( $row = $sth->fetch( PDO::FETCH_OHJ ) ) {

//good
$row = $sth->fetch( PDO::FETCH_OHJ );
if( $row ) {

for( $i = 0; //also fine

There is another principle being violated here: the Arrow Anti-Pattern. This pattern illustrates code that is heavily or unnecessarily indented so that it comes to points like an arrow. The arrow shape isn't necessary, its just one of the signs of violation. The problem with such code is legibility. The more indentation you have the less room per line you have for code (you should only go up to 80 characters, including indentation). It isn't bad here, but it should still be avoided. The easiest way to avoid violating this principle is to return early. And, if you are returning early, you can avoid unnecessary things like else statements. So, if we reverse our if logic so that the larger block of code would be in the else, then we can remove that entire level of indentation.

if( ! $sth->execute() ) {
    throw new Exception('A problem has occurred while retrieving information from database.');
} //else {//else implied, not necessary

//rest of code here, no more indentation

So, going back to that first principle I mentioned, DRY; If you look at your last two methods, you should notice that they do some pretty similar things. Both eventually connect to the database, prepare a statement, and bind some parameters. So, if we make this as reusable as possible, we can create a new private method. The easiest way to do this is to get our bindings into an array and loop over it. I'll let you create the actual method, but the below should help you get started.

$params = array(
    ':id'   => array( $this->id, PDO::PARAM_INT ),
    ':code' => array( $this->code, PDO::PARAM_STR ),
    ':name' => array( $this->title, PDO::PARAM_STR )
);

$core = Core::getInstance();
$sth  = $this->core->dbh->prepare( $sql );
foreach( $params AS $param => $data ) {
    list( $value, $type ) = $data;
    $sth->bindParam( $param, $value, $type );
}

The biggest key, at least for me, to understanding OOP was to understand the underlying principles first. I mentioned one already and partially demonstrated it, DRY, but there are many more (Single Responsibility, Law of Demeter, etc...). My biggest suggestion to you would be to take a look at the wikipedia entry for SOLID and follow all of the links. Its a lot to read, but understanding all the principles and how they work together will make understanding OOP easier.