Code Review Asked by LTCD2CV on December 22, 2021
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>
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.
Answered by mickmackusa on December 22, 2021
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP