Questionnaire classes and interfaces

Question

I Need your help, please. I'm trying to develop a simple Questionnaire.

Here is what I did already. First, I have interfaces for Question and Answer entities.

interface IAnswerProvider{

    public function getAnswers();

}

interface IQuestionProvider{
    public function getQuestions();
}

Then I have classes which implement these interfaces. Both Answers and Questions have translations in different languages - that's why each of them have a $current_language property.

class Answer implements IAnswerProvider
{

    private $current_language = '';

    private $current_question_id = 0;


    public function __construct($current_language, $question_id = 0)
    {
        if (!empty($question_id)) {
            $this->setCurrentQuestionId($question_id);
        }

        $this->setCurrentLanguage($current_language);
    }

    public function getAnswers()
    {

        $question_id = $this->getCurrentQuestionId();

        if (empty($question_id)) {
            throw new Error('Question ID is not defined');
        }

        $query = 'select * from  answers a   where a.question_id=' . escape($question_id);

        return DB::getInstance()->fetchAllRows($query);
    }


    /**
     * @return string
     */
    public function getCurrentLanguage()
    {
        return $this->current_language;
    }

    /**
     * @param string $current_language
     */
    public function setCurrentLanguage($current_language)
    {
        $this->current_language = $current_language;
    }

    /**
     * @return int
     */
    public function getCurrentQuestionId()
    {
        return $this->current_question_id;
    }

    /**
     * @param int $current_question_id
     */
    public function setCurrentQuestionId($current_question_id)
    {
        $this->current_question_id = $current_question_id;
    }

}

class Question implements IQuestionProvider
{

    private $current_language = '';

    public function __construct($current_language)
    {
        $this->setCurrentLanguage($current_language);
    }

    /**
     * @return string
     */
    public function getCurrentLanguage()
    {
        return $this->current_language;
    }

    /**
     * @param string $current_languge
     */
    public function setCurrentLanguage($current_language)
    {
        $this->current_language = $current_language;
    }




    public function getQuestions()
    {


        $query = 'select * from questions q where language ='.escape($this->getCurrentLanguage());


        return DB::getInstance()->fetchAllRows($query);
    }


}

And here is a class which does all jobs related with questions and answers.

class Questionnaire{

    private $oQuestionProvider = '';

    private $oAnswerProvider = '';

    public function __construct(IQuestionProvider $oQueston, IAnswerProvider $oAnswer)
    {
        $this->oQuestionProvider = $oQueston;
        $this->oAnswerProvider = $oAnswer;
    }

    /**
    * Returns an array of questions , each question has answers property 
    * with array of answers
    */
    public function getQuestionsWithAnswers(){

        $aData = $this->oQuestionProvider->getQuestions();

        //assign answers to each question
        foreach ($aData as $v){
            $this->oAnswerProvider->setCurrentQuestionId($v->question_id);
            $v->answers = $this->oAnswerProvider->getAnswers();
        }

        return $aData;
    }

}

What I 'm concerned about is that I have code duplication in the Question and Answer classes - I mean setCurrentLanguage and getCurrentLanguage methods. Maybe it's better to move them into some abstract class and then inherit both Answer and Question classes from it ? Are SOLID principles being violated here?

Please let me know what do you think about this code. Thanks

Answer 1

This is not a real answer, but it is too long to be a comment.

I would be very lazy, in this case, and define the language at a global level. You could even make it a constant, because it probably won't change during the running of the script. If you want to keep language setters and getters in every object then I would indeed use a parent class for that.

I also wonder if you reconnect to the database for every query? That's very inefficient. I hope you reuse the connection. Also, even though the variables are not under user control, I would, as a habit, use parameter binding in queries, for security reasons. Better use it too often than too little.

Also try to use sensible names for your variable and field names. The use of 'current' in some, and not in others, seems weird. Remember, they are always 'current' when used. Names have to make sense. Just remove the word 'current' everywhere and it's fixed.