Manipulating survey questions

Question

I've been a developer for many years, but have always put together files of lists of functions to do everything that needed to be done (instead of using classes/objects like I should be). I've recently started forcing myself to try to get into more object-oriented programming.

I wanted to get some feedback on whether or not I'm implementing classes/objects 'properly' and 'effectively'.

I'm manipulating "Questions" within "Surveys". "Questions" also have sub-parts such as "Choices", but all-in-all my classes are set up in the same format.

I have a Questions.class.php (which contains all items related to getting/setting/updating/deleting questions) and a QuestionList.class.php (which grabs lists of questions from the database).

<?php
//Question.class.php
class Question {

    protected $_table;
    public $_question;

    public $lang;

    function __construct($question_id) {    
        $this->_table = 'TABLE NAME OF QUESTIONS THEMSELVES';
        $this->_surveys_table = 'TABLE NAME OF SURVEYS (A SURVEY CONTAINS QUESTIONS)';

        //When I create an object in external code, I pass it either an ID of a question that exists in my database, or NULL to set up a blank question that doesn't yet exist in the database.
        if(isset($question_id) && is_int($question_id)) {
            $this->_question = $this->get($question_id);
        } else {
            $this->_question = array();
        }

        //This is just a file of error messages--stole the concept from a large Authentication class
        require "language.php";
        $this->lang = $lang;
    }

    //Literally just gets the question from the database and returns an error if it doesn't exist.
    public function get($question_id) {
        $return['error'] = true;

        global $DBH;
        $query = "SELECT * FROM {$this->_table} WHERE question_id = :question_id";
        $q = $DBH->prepare($query);
        $q->bindValue(':question_id', $question_id, \PDO::PARAM_INT);
        $q->execute();

        if(!$row = $q->fetch(\PDO::FETCH_ASSOC)) {
            $return['message'] = $this->lang['questions_nomatch'];
            return $return;
        }

        return $row;        
    }

    //"Sets" the object into the object/fields array so that I can add it or use the object without actually saving to the database.
    public function set($question, $question_multi, $survey_id) {
        $return['error'] = true;

        $this->_question = array(
                'question' => $question,
                'question_multi' => $question_multi,
                'survey_id' => $survey_id
            );

        $return['message'] = $this->lang['questions_set'];
        $return['error'] = false;
        return $return;

    }

    //Adds a question to the database
    public function add() {
        $return['error'] = true;

        $this->_question['question_order'] = $this->getOrder();

        global $DBH;
        //my database insert query is here.

        if(!$q->execute()) {
          $return['message'] = $this->lang['database_error'];
          return $return;
        }

        $this->_question['question_id'] = $DBH->lastInsertId();
        $this->incrementOrder($this->_question['question_order']);

        $return['message'] = $this->lang['questions_added'];
        $return['error'] = false;
        return $return;

    }

    //I can use this to update a previously 'get' object (one that already exists in the database) in the database
    public function update() {
        $return['error'] = true;

        global $DBH;
        //my database update query is here.

        if(!$q->execute()) {
          $return['message'] = $this->lang['database_error'];
          return $return;
        }

        $return['message'] = $this->lang['questions_updated'];
        $return['error'] = false;
        return $return;
    }

    //Deletes a question from the database.
    public function delete() {
        $return['error'] = true;

        global $DBH;
        $query = "DELETE FROM {$this->_table} WHERE question_id = :question_id";
        $q = $DBH->prepare($query);
        $q->bindValue(':question_id', $this->_question['question_id'], \PDO::PARAM_INT);

        if(!$q->execute()) {
          $return['message'] = $this->lang['database_error'];
          return $return;
        }

        $return['message'] = $this->lang['questions_deleted'];
        $return['error'] = false;
        $return['delid'] = $this->_question['question_id'];
        return $return;
    }

}


?>

So, if I want to add a new question:

//this is how I create a question that doesn't exist in the database.
$_question = new Question(NULL);
$_question->set($question, $question_multi, $survey_id);
$add_question = $_question->add();

if(!$add_question['error']) {
    //if there's no error adding, it now exists in the database and I do whatever
}

..or if I want to edit one that already exists:

//this is how I get a question that already exists from the database and update it to the database with new values
$question = new Question($question_id);

//I could also use $question->set($question, $question_multi, $survey_id) here but for some reason I refer the readability of this.
$question->_question['survey_id'] = $survey_id;
$question->_question['question'] = $question_question;
$question->_question['question_multi'] = $question_multi;

$question_updated = $question->update();

if(!$question_updated['error']) {
    //the update was successful and I do whatever
}

And as for my QuestionList.class.php, it's much simpler but I'd also like to have feedback on if I'm using classes properly at all:

<?php

class QuestionList {

    protected $_table;
    public $_questionlist;

    public $lang;

    function __construct($survey_id) {    
        $this->_table = 'TABLE NAME OF QUESTIONS THEMSELVES';
        require "language.php";
        $this->lang = $lang;

        //when created, I force the getAll function which either gets all values from the table or gets those with a specific survey_id
        $this->_questionlist = $this->getAll($survey_id);
    }

    public function getAll($survey_id = NULL) {
        $return['error'] = true;

        global $DBH;
        if($survey_id == NULL) {
            $query = "SELECT * FROM {$this->_table} ORDER BY question_id";
            $q = $DBH->prepare($query);
        } else {
            $query = "SELECT * FROM {$this->_table} WHERE survey_id = :survey_id ORDER BY question_order";
            $q = $DBH->prepare($query);
            $q->bindValue(':survey_id', $survey_id, \PDO::PARAM_INT);           
        }
        $q->execute();

        if(!$rows = $q->fetchAll(\PDO::FETCH_ASSOC)) {
            $return['message'] = $this->lang['questions_notfound'];
            return $return;
        }

        return $rows;
    }
}


?>

To use it, I simply pass it NULL to get an entire list of EVERY question ever, or a survey_id to get specific questions from a Survey:

$question_list = new QuestionList($survey_id);
$questions = $question_list->_questionlist;

I know this is probably long and annoying, but I appreciate any feedback/suggestions!

Answer 1

1. DO NOT USE GLOBALS
2. Message handling is not models responsibility.
3. Functions should do as few things as possible, preferably one.

4. Try not to reinvent the wheel. What you are trying to accomplish here is easily done using ORM (ex: Doctrine) or Active Record.

5. Caller should provide function with valid arguments (expect either array or int/string). Null is rarely a valid argument. Definitely not in your constructor.

Below code can be improved but I didn't want to refactor it further as scale of project is very low and it might have added unnecessary complexity. I have used PDO wrapper from here.

<?php

class Question {

    // attributes
    public $question;
    public $question_id; // used snake case as names should be the same as column names in db.
    public $question_multi;
    public $survey_id;
    public $question_order;

    public function table() {
        return 'TABLE NAME OF QUESTIONS THEMSELVES';
    }

    public static function findById($id) {
        /** @var PDOStatement $stmt */
        $stmt = DB::prepare("SELECT * FROM ".self::table()." WHERE question_id = :question_id");
        $stmt->bindValue(':question_id', $id, \PDO::PARAM_INT);
        if(!$stmt->fetch(\PDO::FETCH_ASSOC)) {
            return null;
        }
        $model = new self;
        $model->setAttributes($stmt->fetch(\PDO::FETCH_ASSOC));
        return $model;
    }

    public static function findAllBySurveyId($surveyId) {
        /** @var PDOStatement $stmt */
        $stmt = DB::prepare("SELECT * FROM ".self::table()." WHERE survey_id = :survey_id ORDER BY question_order");
        $stmt->bindValue(':survey_id', $surveyId, \PDO::PARAM_INT);
        return self::getAllModels($stmt);
    }

    public static function findAll() {
        /** @var PDOStatement $stmt */
        $stmt = DB::prepare("SELECT * FROM ".self::table());
        return self::getAllModels($stmt);
    }

    public function add() {
        /** @var PDOStatement $stmt */
        $this->question_order = $this->getOrder();
        $stmt = DB::prepare(/*my database insert query is here.*/);
        if(!$stmt->execute()) {
            return false;
        }
        $this->setAttributes(['question_id' => DB::lastInsertId()]);
        $this->incrementOrder();
        return true;
    }

    public function update() {
        /** @var PDOStatement $stmt */
        $stmt = DB::prepare(/*my database update query is here.*/);
        return $stmt->execute();
    }

    public function delete() {
        $stmt = DB::prepare("DELETE FROM {$this->table()} WHERE question_id = :question_id");
        return $stmt->execute([':question_id' => $this->question_id]);
    }

    public function setAttributes(array $attributes) {
        foreach($attributes as $attribute => $value) {
            $this->$attribute = $value;
        }
    }

    private function getOrder() {
        return 1;
    }

    private function incrementOrder() {
        $this->question_order = $this->getOrder() + 1;
    }

    private function getAllModels(PDOStatement $stmt) {
        $models = [];
        while($row = $stmt->fetch(\PDO::FETCH_ASSOC)) {
            $model = new self;
            $model->setAttributes($row);
            $models[] = $model;
        }
        return $models;
    }
}

$question1 = new Question();
$question1->setAttributes([
    'question' => $question,
    'question_multi' => $question_multi,
    'survey_id' => $survey_id,
]);

if($question1->add()) {
    // successful
}

$question2 = (new Question())->findById(1);
$question2->survey_id = $survey_id;
$question2->question = $question;
$question2->question_multi = $question_multi;

if($question2->update()) {
    // successful
}

Answer 2

Put shared values in shared space

    function __construct($question_id) {    
        $this->_table = 'TABLE NAME OF QUESTIONS THEMSELVES';
        $this->_surveys_table = 'TABLE NAME OF SURVEYS (A SURVEY CONTAINS QUESTIONS)';

Since these are the same for every instance of this class, you can set them as static variables of the class.

    protected static _table = 'TABLE NAME OF QUESTIONS THEMSELVES';
    protected static _surveys_table = 'TABLE NAME OF SURVEYS (A SURVEY CONTAINS QUESTIONS)';

    function __construct($question_id) {

As static variables they are set once for the entire class.

    protected const _table = 'TABLE NAME OF QUESTIONS THEMSELVES';
    protected const _surveys_table = 'TABLE NAME OF SURVEYS (A SURVEY CONTAINS QUESTIONS)';

    function __construct($question_id) {

As constants they'd be unchangeable as well.

Public class data is rare

    public $_question;

    public $lang;

By making this public, you are essentially making this into a structure. Anything can modify these. Which is odd, since the class also gets $_question from the database and $lang from the language.php file. So on the one side, you have the class defining how to set these values and on the other side, you leave them open to modification anywhere in the program. Usually you want to pick between one or the other--open modification or class defined behavior. As stands, you don't know if you're getting the behavior defined by the class or the user.

I also find it odd that you set $lang in each object that you instantiate. It would be more common to have a language class that would load this information and make it accessible through a static method. Then there's only be one copy, not one copy for every object in the program.

Database queries in a constructor is rare

            $this->_question = $this->get($question_id);

This is an anti-pattern. By linking object construction to the database, you make it difficult to construct objects without the database.

It would be more common to create a factory-style method for this. It would call the constructor and fill the object with the results of the database call. Or simply do those two things separately and directly and skip the combo method.

QuestionList should return questions

Your QuestionList class returns database results directly. It would be more common to convert those into Question objects. Then Question would take the responsibility of parsing each row from the database. As is the caller has to know how database results are organized. That's what the objects are supposed to encapsulate.