-1
\$\begingroup\$

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));
        }

    }

}
\$\endgroup\$
  • \$\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 Aug 8 '15 at 0:39
10
\$\begingroup\$

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:

enter image description here

| improve this answer | |
\$\endgroup\$

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.