I just began my C# debut and was just wondering if this Rock-Paper-Scissor console app is good and flexible.
abstract class Participant //Abstract - no need for instances, Many common functionality
{
public int wins;
float winRate;
public void DisplayWinRate()
{
winRate = ((float)wins / Game_Info.gamesPlayed) * 100; //Win rate percentage
string _winRate = String.Format("win rate: {0}%", winRate.ToString());
Console.WriteLine(_winRate.PadLeft(0));
}
public abstract R_P_S Choice(); //Every participant needs to have a choice. No choice about that TROLLOLOLOL
}
enum R_P_S
{
invalid,
rock,
paper,
scissor
}
class Computer : Participant // THE TROLL KING ENTERS
{
//1-3 2
//1-4 3
Random rand = new Random();
public override R_P_S Choice() //Override the abstract method Choice inside the Participant class
{
R_P_S element;
element = (R_P_S)rand.Next(1,Enum.GetNames(typeof(R_P_S)).Length);
return element;
}
}
class Player : Participant // DEEEEEEEZ NUUUTS
{
public override R_P_S Choice()
{
R_P_S element;
string playerChoice = Console.ReadLine().Trim();
bool validEntry = Enum.TryParse(playerChoice, out element);
if (!validEntry)
{
return R_P_S.invalid;
}
return element;
}
}
struct Game_Info //The game's current state
{
public static int gamesPlayed; //Made it static - So it does not change on a object reference basis.
}
static class Settings
{
public static int space = 28;
}
class Game_Loop
{
static void Main()
{
Console.BackgroundColor = ConsoleColor.White;
Console.ForegroundColor = ConsoleColor.Black;
Console.WindowWidth = 50;
Computer comp = new Computer();
Player player = new Player();
R_P_S computerChoice;
R_P_S playerChoice;
ConsoleKeyInfo input;
bool playAgain;
do //Runs at least once
{
Console.Clear(); //Clears the console window after and before the game is played
computerChoice = comp.Choice();
playerChoice = player.Choice();
Console.Clear();
while (playerChoice == computerChoice) // 1/3 chance to 1/2 chance
{
//Interesting to see how many cycles it takes
computerChoice = comp.Choice();
}
Console.WriteLine("Player: " + playerChoice);
Console.WriteLine("\n" + "Computer: " + computerChoice);
determineWinner(player,comp,playerChoice,computerChoice);
//Placing a invalid value does not count as a game played.
Game_Info.gamesPlayed = (playerChoice != R_P_S.invalid) ? Game_Info.gamesPlayed + 1 : Game_Info.gamesPlayed;
Console.WriteLine("\n" + "Play again? <y/n>".PadLeft(32));
Console.WriteLine("\n");
int resetPosY = Console.CursorTop;
int resetPosX = Console.CursorLeft;
Console.SetCursorPosition(30, 0); //Displays the winrate UP-LEFT
player.DisplayWinRate();
Console.SetCursorPosition(30, 2);
comp.DisplayWinRate();
Console.SetCursorPosition(resetPosX, resetPosY); //Where the cursor should be
input = Console.ReadKey(true);
playAgain = input.KeyChar == 'y';
} while (playAgain);
}
public static void determineWinner(Player player ,Computer comp,R_P_S playerChoice,R_P_S computerChoice)
{
R_P_S rock = R_P_S.rock;
R_P_S scissor = R_P_S.scissor;
R_P_S paper = R_P_S.paper;
int space = Settings.space;
if (playerChoice == rock && computerChoice == scissor || playerChoice == paper && computerChoice == rock)
{
player.wins++;
Console.WriteLine("\n" + "You won!".PadLeft(space));
}
else if (playerChoice == scissor && computerChoice == rock || playerChoice == rock && computerChoice == paper)
{
comp.wins++;
Console.WriteLine("\n" + "Computer won!".PadLeft(space + 2));
}
else if (playerChoice == scissor && computerChoice == paper)
{
player.wins++;
Console.WriteLine("\n" + "You won!".PadLeft(space));
}
else if (playerChoice == paper && computerChoice == scissor)
{
comp.wins++;
Console.WriteLine("\n" + "Computer won!".PadLeft(space + 2));
}
else
{
Console.WriteLine("\n" + "invalid value".PadLeft(space + 2));
}
}
}
-
\$\begingroup\$ I don't want to put this as an answer as I don't believe it to be fleshed out enough and it doesn't answer your question of if your code is well structured & safe, but perhaps consider RockPaperScissors instead of R_P_S - while R_P_S does a good job of portraying meaning, you couldn't tell what it is from a glance. \$\endgroup\$Ashley Davies– Ashley Davies2015年08月08日 00:39:23 +00:00Commented Aug 8, 2015 at 0:39
1 Answer 1
I'd start with your enum:
enum R_P_S { invalid, rock, paper, scissor }
I'm not going to mention R_P_S
is a really bad name for it and that types and their members should be PascalCase
(or did I just... yeah I did), but the biggest problem here is this invalid
value. If you have 3 valid values, make the enum have, well, 3 valid values:
enum Selection
{
Rock,
Paper,
Scissors
}
You're using this invalid
value to represent the lack of a selection - when there's no selection yet...
public abstract R_P_S Choice(); //Every participant needs to have a choice. No choice about that TROLLOLOLOL
...it makes more sense to convey the lack of a value by using a nullable type - a Nullable<R_P_S>
or, as I'd put it, a Selection?
.
public abstract Selection? Choice();
That way, when the input is invalid...
if (!validEntry) { return R_P_S.invalid; }
You could just return null
:
if (!validEntry)
{
return null;
}
That said, Choice()
is a bad name for a method. Try to name your methods starting with a verb: methods do something - keep nouns for types (classes).
These variables aren't needed:
R_P_S rock = R_P_S.rock;
R_P_S scissor = R_P_S.scissor;
R_P_S paper = R_P_S.paper;
And the Settings
class and its Space
member is really only used in the determineWinner
static method, to format the output. Wait. "Determine winner", "Format the output" ...how do these two things end up in the same sentence?
I bet this comment will make you think "WTF?" in a few months from now:
//1-3 2 //1-4 3
Do yourself a favor, remove the fluff:
// THE TROLL KING ENTERS
// DEEEEEEEZ NUUUTS
//Every participant needs to have a choice. No choice about that TROLLOLOLOL
Keep comments that explain why, and remove anything that says what (and especially those that say wut?!)
This comment made me wonder:
//Made it static - So it does not change on a object reference basis.
The member is static indeed, and making it static will effectively make it belong to the type, as opposed to the instance. But the type in question is a struct
- a value type. Value types don't have "object references". And value types should be immutable. In fact, Game_Info
is outright wrong. Make it a reference type (a class), and don't expose public fields - expose properties instead.
As an exercise for pushing flexibility, I'd recommend you consider how your code could be refactored to support Rock-Paper-Scissors-Lizard-Spock, or even to push it to... Rock-Paper-Scissors-Lizard-Spock-Spiderman-Batman-Wizard-Glock:
-
\$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$Wasiim Ouro-sama– Wasiim Ouro-sama2015年08月08日 04:14:21 +00:00Commented Aug 8, 2015 at 4:14
Explore related questions
See similar questions with these tags.