Rock, Paper, Scissors in C#

Question

First of all, I am very new to programming and also this site.

This is a very simple "Rock, Paper, Scissors" game I made in C#. At first, it asks if I want to play the game. If my answer is yes, then I enter my name and the game starts. The program asks me to choose rock, paper or scissors, then the computer randomly picks one ,the program compares them and gives the outcome. If I win 5 rounds I win the game but if the computer wins 5 round it wins the game, and then the program asks if I want to play again.

My code is working but I think it could have been better, cleaner and shorter.

using System;
using System.ComponentModel.Design;

namespace rockpaperscissors
{
    class Player
    {
        public string playername;
        private int playerscore;

        public int Score
        {
            get { return playerscore; }
            set { playerscore = value; }
        }
    }
    class RockPaperScissors
    {
        public static string p_rps;
        private static int c_rps;
        private static int computerscore;

        public static void Initialize(Player player)
        {
            player.Score = 0;
            computerscore = 0;          
        }

        public static void Board(Player player)
        {
            Console.WriteLine("\n\t\t{0}: {1}\n\n\t\tComputer: {2}\n", player.playername, player.Score, computerscore);
        }

        public static int ComputerRPS()
        {
            Random c = new Random();
            c_rps = c.Next(1, 4);

            return c_rps;
        }

        public static void Check(int c, Player player)
        {
            c = ComputerRPS();

            switch(c)
            {
                case 1:

                    if (p_rps == "R")
                    {
                        Console.WriteLine("Tie");
                    }
                    else if (p_rps == "P")
                    {
                        Console.WriteLine("Computer chose rock.\nPaper beats rock. {0} wins this round.", player.playername);
                        player.Score++;
                    }
                    else if (p_rps == "S")
                    {
                        Console.WriteLine("Computer chose rock.\nRock beats scissors. Computer wins this round.");
                        computerscore++;
                    }

                    break;

                case 2:

                    if (p_rps == "R")
                    {
                        Console.WriteLine("Computer chose paper.\nPaper beats rock. Computer wins this round.");
                        computerscore++;
                    }
                    else if (p_rps == "P")
                    {
                        Console.WriteLine("Tie");
                    }
                    else if (p_rps == "S")
                    {
                        Console.WriteLine("Computer chose paper.\nScissors beats rock. {0} wins this round.", player.playername);
                        player.Score++;
                    }

                    break;

                case 3:

                    if (p_rps == "R")
                    {
                        Console.WriteLine("Computer chose scissors.\nRock beats scissors. {0} wins this round.", player.playername);
                        player.Score++;
                    }
                    else if (p_rps == "P")
                    {
                        Console.WriteLine("Computer chose scissors.\nScissors beats paper. Computer wins this round.");
                        computerscore++;
                    }
                    else if (p_rps == "S")
                    {
                        Console.WriteLine("Tie");
                    }

                    break;
            }
        }

        public static bool WhoWins(Player player)
        {
            if (player.Score == 5)
            {
                Console.WriteLine("\n{0} wins the game.\n",player.playername);
                return true;
            }
            if (computerscore == 5)
            {
                Console.WriteLine("\nComputer wins the game.\n");
                return true;
            }

            return false;
        }
    }
    class Program
    {
        public static bool play;
        public static string startgame;

        static void StartGameOrNot()
        {
            do
            {                
                startgame = Console.ReadLine().ToUpper();
                startgame.ToUpper();

                if (startgame == "Y")
                {
                    play = true;
                }
                else if (startgame == "N")
                {
                    Console.WriteLine("\nOkay then, goodbye");
                    Environment.Exit(0);
                }
                else
                {
                    Console.Write("\nInvalid. Do you want to start the game? [Y/N] --> ");
                }
            } while (startgame != "Y" && startgame != "N");
        }
        static void Main(string[] args)
        {
            Console.Write("Do you want to start the game? [Y/N] --> ");
            StartGameOrNot();

            Player player1 = new Player();

            Console.Clear();
            Console.Write("\n\n\tWhat is your name? --> ");
            player1.playername = Console.ReadLine();

            Console.Clear();
            RockPaperScissors.Initialize(player1);

            while (play)
            {                              
                RockPaperScissors.Board(player1);

                do
                {
                    Console.Write("Rock, paper, scissors? [R/P/S] --> ");
                    RockPaperScissors.p_rps = Console.ReadLine().ToUpper();                   
                } while (RockPaperScissors.p_rps == "R" && RockPaperScissors.p_rps == "P" && RockPaperScissors.p_rps == "S");

                int c = RockPaperScissors.ComputerRPS();

                Console.Clear();

                RockPaperScissors.Check(c, player1);

                if(RockPaperScissors.WhoWins(player1))
                {
                    Console.Write("Replay? --> ");
                    StartGameOrNot();
                    RockPaperScissors.Initialize(player1);
                    Console.Clear();
                }
            }
        }
    }
}
```

Answer 1

The program compiles and runs on my machine, and I didn't notice any unexpected behavior or bug, so this is a good start.

Issues

Naming conventions

In C# CamelCase is preferred for naming class properties. As such, names like PlayerName, PlayerScore, ComputerScore would be better.

Same goes for namespaces, so RockPaperScissors would be better.

Also, some names choices aren't quite meaningful. I feel like ComputerMove and PlayerMove would make more sense than c_rps and p_rps.

Unused using

You don't use anything from System.ComponentModel.Design, so the line using System.ComponentModel.Design; should be removed.

Don't use global variables

While they technically are properties of the Program class, the play and startgame variables are scoped over the whole program, and there is no reason for this. Pass the variables as arguments if they should be used across multiple methods.

Use auto-properties

Considering this snippet:

private int playerscore;

public int Score
{
    get { return playerscore; }
    set { playerscore = value; }
}

C# has a shorthand for this kind of pattern, which would simply be:

public int PlayerScore { get; set; }

Inconsistent types

A player move is stored as a string, while a computer move is store as an int. This is confusing, and makes it harder than necessary to compare the values. See your Check() method: there's got to be a way to make it simpler than listing all possible cases, right?

Using ints for both would already makes it much easier.

Another option is to create a class implementing the IComparable interface. It may be considered overkill in a simple case like this one, but has the advantage of being more generic and applicable to other, possibly more complex, cases.

Magic number

The game stops when a player's score reaches 5. That's fine, but it would better for this value to be a named constant.

If you change your mind and decide that 3 is better, you'd have to change the value in 2 places in your code. That would be easy to miss. Also, if you were to use this value elsewhere (e.g. the start screen, so the player knows for how long he'll be playing), you'd have to remember what that value is, and remember to modify it if you decide to change the value. It also makes the code more readable.

Separation of concerns

I can tell you tried to separate concerns, with the core of the game being in its own class, and console display being (somewhat) in the Program class. However, you only achieve this goal partially. A lot of methods directly print stuff on the console, instead of returning the value, so display is mixed with logic.

This is bad practice, the main reason being that it makes the code harder to reuse. Say you'd like to make a fancy-pants GUI to play the game, you'd have to rewrite most of the existing code because all your Console.WriteLines would fail if you don't use a console.

A good approach would be to have a class holding the game state, and a class in charge of displaying the game, fetching the required data from the game state object.

Possible game improvements

When you'll have a clean code for your basic game of rock, paper, scissors, implementing one of these improvements should be possible with minimum changes.

Set the length of the game

Let the player decide for how long he wants to play before starting the game.

PvP

Playing vs the computer is nice, but an option to play vs a human opponent would be even better. I would have a ComputerPlayer and a HumanPlayer class, which both inherit from a Player class. That way

Rock, paper, scissors, lizard, Spock

How would you go at changing the rules to include more moves? Any odd number of moves can make a balanced game. (see Wikipedia if you're not familiar with that variant).

This is where Move class would probably prove more readable than working with ints for moves.

Answer 2

Seperation of concerns, the rule engine should be separated from the "AI". Now you need a large rewrite if you want to support two players.

Properly written there would be no difference if a AI player or a real player was playing and even two AI could play against each other.

Also the rules can be defined with a 2 dimensional array instead of switch case / if-block

Here is a working but not perfected example, I suspect that this is a school homework so do not want to give you everything ;)

class Program
{
    private enum Type
    {
        Rock = 0, 
        Paper = 1,
        Scissor = 2
    }

    private interface IPLayerInput
    {
        Type GetInput();
    }

    private class KeyboardPlayerInput : IPLayerInput
    {
        public Type GetInput()
        {
            return (Type)Enum.Parse(typeof(Type), Console.ReadLine() ?? string.Empty);
        }
    }

    private class AiPLayerInput : IPLayerInput
    {
        private readonly Type[] _values;
        private readonly Random _rand;

        public AiPLayerInput()
        {
            _values = Enum.GetValues(typeof(Type)).Cast<Type>().ToArray();
            _rand = new Random();
        }

        public Type GetInput()
        {
            return _values[_rand.Next(0, _values.Length)];
        }
    }

    private class Player
    {
        private readonly IPLayerInput _input;

        public Player(string name, IPLayerInput input)
        {
            _input = input;
            Name = name;
        }

        public int Score { get; set; }
        public string Name { get; }

        public void RequestNewHand()
        {
            CurrentHand = _input.GetInput();
        }

        public Type CurrentHand { get; private set; } 
    }

    static void Main()
    {
        var rules = new Type?[,] { 
            { null, Type.Paper, Type.Rock }, 
            { Type.Paper, null, Type.Scissor }, 
            { Type.Rock, Type.Scissor, null } };


        var players = new List<Player> {new Player("AI", new AiPLayerInput()), new Player("Hooman", new KeyboardPlayerInput())};

        Player winner = null;
        while (winner == null)
        {
            players.ForEach(p => p.RequestNewHand());
            foreach (var player in players)
            {
                var score = players.Count(p => p != player && rules[(int)player.CurrentHand, (int)p.CurrentHand] == player.CurrentHand);
                player.Score += score;
            }

            winner = players.SingleOrDefault(p => p.Score >= 3);
        }

        Console.WriteLine($"Winner is {winner.Name}");

    }
}

Answer 3

In case 2 you have "Computer chose paper.\nScissors beats rock."

I don't think that is right. Should be "Computer chose paper.\nScissors beats paper."

You have a static variable, c_rps, but it's not needed.

What is "using System.ComponentModel.Design;" for? I doubt it is needed.

Answer 4

You have "Computer chose paper.\nScissors beats rock" in case 2.
You have a static variable, c_rps, you don't need it
(1,4) is not right, should be Next(1,3), there are three choices in Rock, Paper and Scissors.
What is System. ComponentModel. Design use;?
I don't think you need that as well

Hope that helps!