3
\$\begingroup\$

This is from a flex app that is for quizzing. It calls the database for the question and then checks to see if it has been tagged (bookmarked) by a user already. "Question" is a value object and "Tag" is also a value object.

I just wanted to post it to see how it could be better. It works fine, but I feel that since I am new to this, it probably isn't best.

public function getQuestionByNumber($itemNum, $userID) {
    $stmt = mysqli_prepare($this->connection,
          "SELECT
                        questions.id, questions.question_number, questions.chapter_id, questions.section_id, 
                        questions.question_txt, questions.answers, questions.feedbacks, questions.correct_answer, 
                        questions.question_type, questions.module, questions.shuffle_answer,
                        questions.tags
           FROM questions where questions.question_number=?");
      $this->throwExceptionOnError();

      mysqli_stmt_bind_param($stmt, 'i', $itemNum); $this->throwExceptionOnError();

      mysqli_stmt_execute($stmt); $this->throwExceptionOnError();

      $row = new Question();

      mysqli_stmt_bind_result($stmt, $row->id, $row->question_number, $row->chapter_id, $row->section_id, 
                                        $row->question_txt, $row->answers, $row->feedbacks, $row->correct_answer, 
                                        $row->question_type, $row->module, $row->shuffle_answer, $row->tags);


          if (mysqli_stmt_fetch($stmt)) {

              } else {
                          return null;
                  }
            mysqli_stmt_free_result($stmt);     
            mysqli_stmt_close($stmt);

    $stmt2 = mysqli_prepare($this->connection,
          "SELECT
                        id
           FROM tagged where user_id =? && question_number=?");
      $this->throwExceptionOnError();

      mysqli_stmt_bind_param($stmt2, 'ii', $userID, $itemNum);
      $this->throwExceptionOnError();

      mysqli_stmt_execute($stmt2);
      $this->throwExceptionOnError();

      $tag = new Tag();

      mysqli_stmt_bind_result($stmt2, $tag->id);

      if (mysqli_stmt_fetch($stmt2)) {
                  $row->tagged = $tag->id;
                  return $row;
      } else {
                  $row->tagged = 0;
                  return $row;
          }


      mysqli_stmt_free_result($stmt2);
      mysqli_close($this->connection);

}
\$\endgroup\$
3
\$\begingroup\$

You could combine your two SQL queries so you don't have to hit the database twice.

The new SQL statement would be something like this:

SELECT
    q.id, q.question_number, q.chapter_id, q.section_id, 
    q.question_txt, q.answers, q.feedbacks, q.correct_answer, 
    q.question_type, q.module, q.shuffle_answer,
    q.tags, COALESCE(t.id, 0) As tagged
FROM questions q
     LEFT JOIN tagged t ON q.question_number = t.question_number AND t.user_id = ?
WHERE q.question_number=?

The COALESCE function would return the id from the tagged table if one exists, otherwise it returns 0. This eliminates the need for your final if/else statement and gives you a direct mapping for $row->tagged.

You should also release all result sets and close all connections before issuing a return statement. Any code after a return statement will not be executed.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the information. I will look into the COALESCE function and learn more about it. Also I didn't know that code after a return statement would not be executed. I really appreciate your time. \$\endgroup\$ Aug 23 '11 at 21:46
  • \$\begingroup\$ The re-write worked great! I learned a lot. On your second point about closing connections, I stuck the free result and close statements in the if() statement before the return. Is this the best way to do it? \$\endgroup\$ Aug 23 '11 at 23:56
  • \$\begingroup\$ It's better, but it still wouldn't close the connection in the case where your query didn't return anything. What I'd do is rewrite the if/else altogether. Make the condition if (!mysqli_stmt_fetch($stmt)) { $row = null }. This would only happen if you got no results from the SQL. After that, free the result, close the connection, then return $row. \$\endgroup\$ Aug 24 '11 at 1:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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