4
\$\begingroup\$

This is my first ever created game. I been slowly but continuously learning PHP for the past 2 months and just last night I thought that I might be able to create a simple and very basic game.

Due to my lack of coding experience I am aware that my code could be pretty bad to the point of making anyone to cringe.

But because I am totally aware that my code could be a shame when it comes to write simple and easy to understand code, that's why I Google "code review" and I found this site.

I want to improve the code and game in 3 ways.

1- Change the code from procedural to object oriented. (But I need to learn object oriented PHP first.)

2- Create a scoreboard that shows player and computer score. (I assume I can get this done with $_SESSION.)

3- Create a highest player scoreboard. (I already know how to create prepared PDO statements to insert data into MariaDB. I also learned procedural MySQLi but I already decided to focus on learning PDO just because of scalability. Then later I can continue learning MySQLi.)

So, my problem is that I just want to know what experienced PHP programmers can tell me to improve my code. Maybe a way to simplify what I've done into an easier to read code or less lines of code etc. Any comment is very appreciated, positive or negative. I don't take anything personal.

The code consist of just a single index.php file with everything in there. Nothing more, nothing less. Anyone can literally copy paste the code and get it running with XAMPP etc.

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Document</title>
</head>

<style>
    .game_form {
        text-align: center;
        margin: 100px;
        font-size: 150%;
    }
</style>

<body>

    <?php
//        First shuffle/randomizer algorithm.
//        There are multiple duplicated values to alter the options because otherwise
//        the computer will almost always choose the same values
//        and it is easy to win by learning/guessing the pattern.
        $array = ['rock', 'rock', 'rock', 'rock', 'paper', 'paper', 'paper', 'paper', 'scissor', 'scissor', 'scissor', 'scissor'];
        shuffle($array);
        $computerSelection = $array[3];
    
//        Second shuffle/randomizer algorithm.
//        $input = ["rock"=>"rock", "paper"=>"paper", "scissor"=>"scissor", 
//                  "rock"=>"rock", "paper"=>"paper", "scissor"=>"scissor", 
//                  "rock"=>"rock", "paper"=>"paper", "scissor"=>"scissor", 
//                  "rock"=>"rock", "paper"=>"paper", "scissor"=>"scissor", 
//                  "rock"=>"rock", "paper"=>"paper", "scissor"=>"scissor"];
//        $computerRandomSelection = array_rand($input, 1);
    ?>
    
    <form class="game_form" action="index.php" method="POST">
      
        <input type="radio" name="playerSelection" value="rock">Rock<br>
        <input type="radio" name="playerSelection" value="paper">Paper<br>
        <input type="radio" name="playerSelection" value="scissor">Scissor<br>
       
        <button name="submit">SUBMIT</button><br><br>
        
    <?php
        if (isset($_POST['playerSelection']) == NULL) {
            echo "Select an option.";
        }
        
        if (isset($_POST['playerSelection'])) {
            $playerSelection = $_POST['playerSelection'];
            echo "You selected: " . $playerSelection . "<br>";
            echo "Computer selected: " . $computerSelection . "<br>";
            
        if ($playerSelection == 'rock') {
            if ($computerSelection == 'rock')
                echo "<h1>Draw...</h1>";
            if ($computerSelection == 'paper')
                echo "<h1>You lose.</h1>";
            if ($computerSelection == 'scissor')
                echo "<h1>Winner!</h1>";
        }
            
        if ($playerSelection == 'paper') {
            if ($computerSelection == 'paper')
                echo "<h1>Draw...</h1>";
            if ($computerSelection == 'rock')
                echo "<h1>Winner!</h1>";
            if ($computerSelection == 'scissor')
                echo "<h1>You lose.</h1>";
        }
            
        if ($playerSelection == 'scissor') {
            if ($computerSelection == 'scissor')
                echo "<h1>Draw...</h1>";
            if ($computerSelection == 'rock')
                echo "<h1>You lose.</h1>";
            if ($computerSelection == 'paper')
                echo "<h1>Winner!</h1>";
        }
        }
    ?>
    </form>
    
<!--
Just a reference to build the winner vs the looser.
Rock & Rock ===== Draw
Rock & Paper ==== Paper wins
Rock & Scissor == Rock wins

Paper & Paper === Draw
Paper & Rock ==== Paper wins
Paper & Scissor = Scissor wins

Scissor & Scissor = Draw
Scissor & Rock == Rock wins
Scissor & Paper = Scissor wins
-->
</body>
</html>
Improve this question
\$\endgroup\$
  • \$\begingroup\$ We can definitely give you advice about how to refine your code, however asking for help creating new functionality is a no-no here. As for refactoring the script to convert it from procedural to oop, I have a feeling that you are expected to DO that work yourself, then if it works, then we can help you polish it up. This question _may_be closed due to these concerns. \$\endgroup\$ – mickmackusa Jul 24 '20 at 4:01
  • \$\begingroup\$ Understand, I have to do my homework to learn what I need to learn. I am here just for the code review. I wanted to know how bad is my code. Maybe I am underestimating myself and my code might be fine just the way it is. But definitely I don’t want to be spoon feed what I want to add to the code. I’m here just for a code review. I would like to know how an experienced PHP developer would’ve tackle a game like that. \$\endgroup\$ – LTCD2CV Jul 24 '20 at 5:36
  • 1
    \$\begingroup\$ For UX I'd just use buttons for direct submission, rather than selecting something, and then submitting. \$\endgroup\$ – Progrock Jul 29 '20 at 0:10
  • \$\begingroup\$ I didn’t thought about that. Will definitely implement it. Thanks for the idea. \$\endgroup\$ – LTCD2CV Jul 30 '20 at 1:38
3
\$\begingroup\$

I personally find the fun part of programming to be the challenge of trying to find the leanest, most direct way to execute a task. One measure of how careful a developer is is to pay attention to occurrences where a script repeats something. You should always strive to not repeat yourself (D.R.Y. means Don't Repeat Yourself).

You don't need to bloat your array of options with redundant elements. Just name the three options and move on.

$options = ['paper', 'rock', 'scissors'];

...and don't repeat the <h1> elements in your condition block. Find the outcome, save the string as a variable and echo that variable inside your html markup just one time.

When you want to create your html form, don't manually write the options, loop over the $options array. printf() is a personal choice to avoid needing to escape double quotes or use concatenation syntax. In other words, the extra function call is for code cleanliness.

foreach ($options as $option) {
    printf (
        '<input type="radio" name="playerSelection" value="%s">%s<br>',
        $option,
        ucfirst($option)
    );
}

Another important point is to never generate data that you will not use. On the page load before the human makes a selection, there will be no comparison. This means that you should definitely not be making a random selection for the "computer".

I don't like the loose comparison to NULL, just use if (!isset($_POST['playerSelection'])) {. Instead of writing the inverted if condition immediately after the first, just use } else {.

As for determining the outcomes, there will be several different techniques to choose from. Some developers will prefer to have a literal lookup array of all combinations of selections pointing to a literal output value. Others will aim for a mathematical technique that will spare memory at a cost of readability. This part will come down to how adventurous you'd care to be. Your battery of conditions is very easy to read but it is also one of the longest ways to code your logic. Compare to this alternative:

After further consideration, I think it is better to only declare the flipped options array since this perfectly enables the random selection for the computer and the lookups to determine the $difference.

Code: (Demo)

$options = array_flip(['paper', 'rock', 'scissors']);
$outcomes = ['draw', 'win', 'lose'];

$cpuSelection = array_rand($options);
$playerSelection = 'rock';
$difference = $options[$cpuSelection] - $options[$playerSelection];

var_export([
    'cpu' => $cpuSelection,
    'human' => $playerSelection,
    'outcome' => $outcomes[($difference + 3) % 3]
]);

Outputs (potentially):

array (
  'cpu' => 'rock',
  'human' => 'rock',
  'outcome' => 'draw',
)

array (
  'cpu' => 'scissors',
  'human' => 'rock',
  'outcome' => 'win',
)

array (
  'cpu' => 'paper',
  'human' => 'rock',
  'outcome' => 'lose',
)

It isn't instantly comprehensible. Why do I have a magic 3 in there? If you sharpen your pencil and write a matrix of inputs and outputs, you will find that if you subtract the computer's option's indexvalue from the player's option's indexvalue, then a pattern forms...

If the difference is 0, then it is a draw. If the difference is 1 or -2, then it is a win. If the difference is 2 or -1, then it is a loss. By adding 3 then using the modulus operator to find the remainder upon dividing by 3, you have 3 reliable integers by which you can translate into words via the $outcomes lookup array.

As I said, there will be several ways to attack the outcome calculation. At the end of the day, this is the type of logical algorithm that you will write once and put behind you so that you can focus on more important things like improving the UX, or converting the script structure to oop, etc.

In terms of the UI, yeah, it's pretty raw, but because I don't really care to delve into the html/js/css aspects of this little project, I'll end my review here.

Improve this answer
\$\endgroup\$
  • \$\begingroup\$ Wow! Thank you very much for this answer. It is priceless. This way the code will be more than 50% shorter and cleaner. Probably wouldn't thought about making an array of the potential answers. Once again, thank you very much, really appreciated! \$\endgroup\$ – LTCD2CV Jul 24 '20 at 16:50

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.