0
\$\begingroup\$

I was just wondering if someone could check my small "random dice game", which I programmed due to a "do it yourself" from my PHP book. I am happy that it is working and finished the DIY, but however, I want to know if the solution is okay or if there is a way better/easier way to do it.

Also please let me know if the way of adding comments is fine, if it is too much or not enough.

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8">
    <title>Random Dice Game</title>
    <link rel="stylesheet" href="css/bootstrap.min.css">
</head>
<body>
<table class="table">
    <thead><h3>Lets go!</h3></thead>
    <tbody>
    <?php
        // Defining variables
        $sum1 = 0;
        $sum2 = 0;

        // while loop running until $sum1 or $sum2 >= 21
        while($sum1 < 21 && $sum2 < 21) {
            // random dice, safing result into variables
            $dice1 = random_int(1, 6);
            $dice2 = random_int(1, 6);
            // $sum1 is increased by $dice1
            $sum1 += $dice1;
            // checking if game is over (winner player 1)
            if($sum1 >= 21) {
                echo "<tr><td>Spieler 1 hat $dice1 gewürfelt. Summe: <b>$sum1</b><br>Das Spiel ist beendet.</td></tr>";
                break;
            } 
            // $sum2 is increased by $dice2
            $sum2 += $dice2;
            // checking if game is over (winner player 2)
            if($sum2 >= 21) {
                echo "<tr><td>Spieler 2 hat $dice2 gewürfelt. Summe: <b>$sum2</b><br>Das Spiel ist beendet.</td></tr>";
                break;
            }
            // printing results of both players, if no one won yet
            echo "<tr><td>Spieler 1 hat $dice1 gewürfelt. Summe: <b>$sum1</b><br>Spieler 2 hat $dice2 gewürfelt. Summe: <b>$sum2</b><br></td></tr>";
        }
        // if game is over, checking who won, printing win-message
        if($sum1 >= 21 || $sum2 >= 21) {
            if($sum1 >= 21) {
                echo "<tr><td>Wir haben einen Sieger! Glückwunsch, <b>Spieler 1</b>!<br></td></tr></tbody></table>";
            } else {
                echo "<tr><td>Wir haben einen Sieger! Glückwunsch, <b>Spieler 2</b>!<br></td></tr></tbody></table>";
            }
        }
    ?>
</body>
</html>
\$\endgroup\$

migrated from stackoverflow.com Dec 29 '18 at 13:20

This question came from our site for professional and enthusiast programmers.

1
\$\begingroup\$

Some comments about the generated HTML:

  • <h3>: this element cannot be a child element of thead. Only <tr> elements can be children of <thead>. As a result the browser will in fact place the <h3> element outside of the table.
  • <br>: this element makes not a lot of sense just before a </td>. It it was to create extra vertical white-space, then it is better to use CSS styling on your <td> elements
  • <table>: as your table contains one column only, and the contents are in fact phrases, it is it a bit odd to use a <table> for that. It would make more sense to use <p> or <div> elements.
  • You include bootstrap CSS, which is OK, but for the little content you currently have it is probably overkill. It currently takes care of putting a horizontal border between <td> elements, but this you can manage with your own styles (if needed) on the <p> or <div> tags.

Comments about the PHP code:

  • if($sum1 >= 21 || $sum2 >= 21): this condition will always be true since the while would have continued if this were not the case. You should just omit the if.
  • As the logic is the same for the two players it would be better not to have code repetition, but "toggle" the player between player 1 and 2.
  • It would be even better to put the die-rolling logic in a Player class: one instance per player
  • Separate the HTML output generation from the logic. It is better to keep a log of the game in variables and produce the output from that right at the end of your code.

Here is how it could look:

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8">
    <title>Random Dice Game</title>
    <link rel="stylesheet" href="css/bootstrap.min.css">
<style>
    div.player0 {
        padding-top: 1rem;
        padding-left: 1rem;
        border-top: 1px solid #dee2e6;
    }
    div.player1 {
        padding-bottom: 1rem;
        padding-left: 1rem;
    }
    div.end {
        padding-bottom: 1rem;
        padding-left: 1rem;
    }
</style>
</head>

<body>
    <h3>Lets go!</h3>
    <?php
        class Player {
            public $sum = 0;
            public $name;
            public $iswinner = false;

            public function __construct($name) {
                $this->name = $name;
            }
            public function rolldie() {
                // Throw die randomly, saving result into variable
                $die = random_int(1, 6);
                // $sum is increased by $die
                $this->sum += $die;
                // Check whether game is over
                $this->iswinner = $this->sum >= 21;
            }
        }
        // Defining variables
        $players = [new Player("Spieler 1"), new Player("Spieler 2")];
        $log = [];
        $playerid = 1;

        // Loop until a player wins
        while (!$players[$playerid]->iswinner) {
            // Switch player (toggle between 0 and 1)
            $playerid = 1 - $playerid;
            $player = $players[$playerid];
            // Roll the die
            $die = $player->rolldie();
            // Log the result
            $log[] = [$playerid, $player->name, $die, $player->sum];
        }
        $winner = $player->name;

        // Produce the HTML output
        foreach($log as list($playerid, $name, $die, $sum)) {
    ?>
    <div class="player<?=$playerid?>">$name hat <?=$die?> gewürfelt. Summe: <b><?=$sum?></b></div>
    <?php
        }
    ?>
    <div class="end">Das Spiel ist beendet.</div>
    <div class="winner">Wir haben einen Sieger! Glückwunsch, <b><?=$winner?></b>!</div>    
</body>
</html>
\$\endgroup\$
0
\$\begingroup\$

Caption vs. thead

    <thead><h3>Lets go!</h3></thead>

There are several problems with this. The <thead> element should contain column headers, so something like

  <table>
    <thead>
      <tr>
        <th>Column name</th>
      </tr>
    </thead>

Replace "Column name" with appropriate content.

You may want

  <table>
    <caption>Let's go!</caption>

You can put an <h3> inside a caption, but you probably shouldn't, particularly not as the only content. Instead, use CSS to make the caption style match the h3 style.

Or semantically, what you may want is

  <table>
    <tr>
      <td>Let's go!</td>
    </tr>

That puts this statement at the beginning of the table content rather than making it a heading or caption. That may better reflect the purpose.

Semantic headings

Another problem is that you are using headings incorrectly. Each page should have one h1 element. In your case, it should probably be before the table and say

  <h1>Random dice game</h1>

to match the title. They don't have to be the same, but often are. The title appears in things like the tab's header or a bookmark. The h1 heading appears in the content and is intended to say what is in the content.

Below the h1, you can have one or more subheadings, which are h2 elements. An h3 element would appear below an h2, creating a semantic hierarchy.

Presumably you see the problem now. Not only do you not have any h2 elements, you don't have an h1. So you should not have any h3 elements.

As I alluded previously, "Let's go!" (incidentally, this is the proper spelling; "let's" is a contraction of "let us" in this context) is not really a proper semantic heading or caption. It does not describe the table or that section of content.

Consider using a <span> with appropriate CSS or a simple <strong> to surround "Let's go" instead of something with more semantic meaning that you don't want.

Table vs. list

I also notice that your table isn't very tabular. It's basically a bunch of rows. Rather than using a table, consider using a list.

                echo "<tr><td>Spieler 1 hat $dice1 gewürfelt. Summe: <b>$sum1</b><br>Das Spiel ist beendet.</td></tr>";

could be

                echo "<li>Spieler 1 hat $dice1 gewürfelt. Summe: <strong>$sum1</strong><br/>Das Spiel ist beendet.</li>";

I also replaced the non-semantic <b> with <strong> and changed the SGML <br> to an XML <br/>. Screen readers and other alternate browsers may find it easier to parse this way.

The list can be either ordered (<ol>) or unordered (<ul>), whichever works better for you.

Comments

Comments should be used to explain why you are coding things a certain way, not what you are doing. Code like

        $sum1 = 0;
        $sum2 = 0;

should be self-explanatory. You don't have to say that you are initializing variables. You might consider changing the names though. Consider

        $score1 = 0;
        $score2 = 0;

Or even (old way of declaring PHP arrays)

        $player_scores = array(0, 0);

Or as previously suggested, make an array of players where each player object tracks its score.

Back to comments.

        // while loop running until $sum1 or $sum2 >= 21
        while($sum1 < 21 && $sum2 < 21) {

Both the comment and the code say the same thing. So why have the comment? We can just read the code. Consider instead

        // first player to $WINNING_SCORE or more wins
        while ($scores[0] < $WINNING_SCORE && $scores[1] < $WINNING_SCORE) {

Now the comment tells my why we are comparing these two variables. Also, I replaced the magic number 21 with a constant. This makes the comment less likely to fall out of synch with the code. E.g.

        // first player to 21 or more wins
        while ($scores[0] < 21 && $scores[1] < 15) {

Here, I actually made the code fall out of synch as well. The idea was to make the winning condition lower for both, but the code edit only changed it for one. A constant would not have had that problem.

The only time to use verbose comments like you are is as part of a lecture to students who don't know what the code says, so you are explaining it to them. Unfortunately, this often gives students the wrong idea about how to use comments. So if you are using comments didactically this way, I would suggest on the next slide or figure, showing the code with the comments used more as they would in production code.

It's also worth noting that function names can be used to comment code as well. For example, if you put your random statement inside a roll_die function, then it is obvious that you are using it to simulate a die roll. Or

while (!has_any_won($players)) {
\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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