1
\$\begingroup\$

I'm a beginner in PHP and I would like to know how to make a good quest system.

What is the code for : This code is to accept quest. It show the quest and where the user is in the quest (Ongoing, Available, Finished..).

What I'm looking for : As a beginner, this code is quite bad, and i'm looking for an improved version. For example, all the code is only working for 1 quest, and I would like an useful way to automatically put the quest when created instead of having to copy/paste the whole thing and change the id of the quest. And of course if i could do this in 1 query instead of querya and queryb (1 For the Fetch and 1 For the FetchColumn) it would be good.

The code is actually working if you wonder

Important informations : Most of the text is in frech, so while I'm translating it, I may forgot to change 1 or 2 things. If that's the case, don't assume the code is not working : just tell me what is wrong and it may be a mistake.

The Code

<?php
  $querya = $db->prepare('SELECT * FROM quetejoueur WHERE id_joueur=:id AND id_queteorij=1');
  $querya->bindValue(':id', $donnees["id"], PDO::PARAM_STR);
  $querya->execute();

  $queryb = $db->prepare('SELECT * FROM quetejoueur WHERE id_joueur=:id AND id_queteorij=1');
  $queryb->bindValue(':id', $donnees["id"], PDO::PARAM_STR);
  $queryb->execute();

while ( $findquetes = $querya->fetch() ) {
    if ($queryb->fetchColumn() > 0) {
      if ($findquetes["comp_quetej"] < 1) {
          ?><button disabled class="pomegranate-flat-button" style=" border-radius:6px; font-size:15px; padding-left:45px; padding-right:45px;"> Quest - Quest 1 (Ongoing) </button><?php
      }


    if ($findquetes["comp_quetej"] > 0) {
      if ($findquetes["recomp_quetej"] < 1) {
          echo '<form action="" method="POST"><input type="submit" class="peter-river-flat-button" style=" border-radius:6px; font-size:15px; padding-left:45px; padding-right:45px;" value="Quest - Quest 1 (Confirm the quest)" name="confirmerquete"></form>';
      }

      if ($findquetes["recomp_quetej"] > 0) {
        echo'<button class="emerald-flat-button" style=" border-radius:6px; font-size:15px; padding-left:45px; padding-right:45px;">Quest- Quest1 (Finished) </button>';
      }
    }

    }
    else {
      ?><button class="peter-river-flat-button" style="border-radius:6px; padding-left:45px; padding-right:45px;" onclick="window.location.href='../quete/vigier.php'">Quête - Le Vigier (Disponible) </button><?php
    }

}

$querya->CloseCursor();
$queryb->CloseCursor();

  if ((isset($_POST["confirmerquete"])) AND ($findquetes["recomp_quetej"] < 1)) {
    $querya=$db->prepare('UPDATE users SET exp= (exp + :queteexp) WHERE username=:username');
    $querya->bindValue(':queteexp', $findquetes["xp_quetej"], PDO::PARAM_INT);
    $querya->bindValue(':username', $username, PDO::PARAM_STR);
    $querya->execute();
    $querya->CloseCursor();

    $queryb=$db->prepare('UPDATE users SET money=(money + :moneyquest) WHERE username=:username');
    $queryb->bindValue(':moneyquest', $findquetes["or_quetej"], PDO::PARAM_INT);
    $queryb->bindValue(':username', $username, PDO::PARAM_STR);
    $queryb->execute();
    $queryb->CloseCursor();

    $query=$db->prepare('UPDATE quetejoueur SET recomp_quetej=1 WHERE id_quetej = :idqj');
    $query->bindParam(':idqj', $findquetes['id_quetej'], PDO::PARAM_INT);
    $query->execute();
    $query->CloseCursor();
  }


?>

If you need further explainations, just tell me. I'm sorry if this code is bad written, as I said this is only a beginner's work.

\$\endgroup\$
1
  • \$\begingroup\$ For future reference, please don't translate code just for the sake of posting a Code Review question. For one, we can no longer give you good advice about naming, when we can't tell what you really wrote. For another, bugs can be introduced, especially if you don't test the code adequately after translation. I'd much rather have you add English explanations to your real original code. \$\endgroup\$ Commented Aug 25, 2017 at 19:14

1 Answer 1

2
\$\begingroup\$

I don't understand the need for two queries. They seem to be retrieving the same exact result set, so why query twice?


Your code is difficult to read because of the inconsistent indentation and the inconsistent way in which you render your HTML output (sometimes dropping out of PHP, sometimes echoing out of PHP, for more or less that same type of use case).
I would also encourage you to get better with overall style consistency. Variable names are not very meaningful (querya/queryb). Spacing around operators is inconsistent. Spacing around parenthesis is inconsistent. Use an IDE that allows you to enforce a styleguide and stick with it.


The nesting in your while loop is a bit out of control. You should think about inverting conditionals and/or using continue to clean up these nested conditionals. Consider how much easier it is to read code like this:

<?php 
while ( $findquetes = $querya->fetch() ) {
    if ($queryb->fetchColumn() <= 0) {
?>
<button
    class="peter-river-flat-button"
    style="border-radius:6px; padding-left:45px; padding-right:45px;" 
    onclick="window.location.href='../quete/vigier.php'"
>Quête - Le Vigier (Disponible)</button>
<?php
        continue;
    }
    if ($findquetes["comp_quetej"] <= 0) {
        continue;
    }
    if ($findquetes["comp_quetej"] < 1) {
?>
<button disabled class="pomegranate-flat-button"
    style=" border-radius:6px; font-size:15px; padding-left:45px; padding-right:45px;">Quest - Quest 1 (Ongoing)</button>
<?php
        continue;
    }
    if($findquetes["recomp_quetej"] <= 0) {
        continue;
    }
    if ($findquetes["recomp_quetej"] < 1) {
?>
<form action="" method="POST">
    <input type="submit" class="peter-river-flat-button"
        style=" border-radius:6px; font-size:15px; padding-left:45px; padding-right:45px;"
        value="Quest - Quest 1 (Confirm the quest)" name="confirmerquete"></form>
<?php
        continue;       
    }
?>
<button class="emerald-flat-button"
    style=" border-radius:6px; font-size:15px; padding-left:45px; padding-right:45px;">Quest- Quest1 (Finished)</button>';
<?php
}

Now, the code path is much more clear, and having done your conditionals this way, you can clearly see what might be problematic conditional logic in your current approach (what happens with certain values between 0 and 1?). What I have shown above obviously still needs to be cleaned up to the point where the design intent is made clear. The general philosophical approach should be to make quick exits from functions/loops, etc. to enable you to de-nest your happy path code. Also note that there are very few times where you really need an else condition. If you have written an else, you may want to revisit your condition to see if there is a better way.

Note that you should also de-nest your very last conditional. You have ~15 lines of code inside a conditional when it doesn't need to be.


Why so much inline styling? If you have class names being used, why are these styles not attached to the classes in external CSS? This certainly makes code harder to read the way you have it. This along with perhaps getting to a better approach for templating your display code would server you well in getting to a higher quality code base.


onclick="window.location.href='../quete/vigier.php'"

Do you really need this in javascript as opposed to setting form's action or using a simple <a> styled similarly to button?

If you really need to apply javascript here, consider moving away from in-line usage. Over time you will begin to realize that separation of application display and behavior code allows for a tremendous amount of code re-use. Consider putting this in document.ready handler and applying it to element based on class or id.


You should be able to combine you to update queries against users table into a single query.


I don't know what DB class you are using, but it is unclear how you handle unexpected database interactions. What happens is prepare fails? What happens if execution fails? Are there any possible exceptions you should be catching or error handling that should be in place? You code seem to really only consider "happy path".


I find it odd to have all you POST logic crammed way down at the bottom of the script after the point where you have already rendered output to the browser. Ideally, if you have a script which have different GET and POST logic, you would handle all the database interactions up front and make it much clearer earlier in the script whether the code is relevant to GET or POST.


Why would $findquetes be part of the conditional criteria for this POST logic? This variable may or may not even be populated with a record.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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