5
\$\begingroup\$

I have made my first little project using C#/WPF - a simple rock, paper, scissors game. I am happy with the functionality and look but I would love to know how a more experienced programmer would refactor the code I have to bring the size down and make it more readable. Many thanks.

MainWindow.xaml

<Window x:Class="Rock_Paper_Scissors.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:Rock_Paper_Scissors"
        mc:Ignorable="d"
        Title="Rck/Ppr/Scsr" Height="805.5" Width="612">
    <Grid>
        <Grid Margin="10">
            <Grid.ColumnDefinitions>
                <ColumnDefinition Width="*" />
                <ColumnDefinition Width="*" />
                <ColumnDefinition Width="*" />
            </Grid.ColumnDefinitions>
            <Button x:Name="btnRock" FontSize="35" Content="ROCK" HorizontalAlignment="Left" Margin="10,570,0,0" VerticalAlignment="Top" Width="175" Height="175" Click="Player_Choice"/>
            <Button x:Name="btnPaper" FontSize="35" Content="PAPER" HorizontalAlignment="Left" Margin="9,570,0,0" VerticalAlignment="Top" Width="175" Height="175" Grid.Column="1" Click="Player_Choice"/>
            <Button x:Name="btnScissors" FontSize="35" Content="SCISSORS" HorizontalAlignment="Left" Margin="10,570,0,0" VerticalAlignment="Top" Width="175" Height="175" Grid.Column="2" Click="Player_Choice"/>

            <TextBox Name="txbxPlayersChoice" BorderBrush="Black" TextAlignment="Center" Padding="0, 35" FontSize="35" Grid.Column="1" HorizontalAlignment="Left" Height="126" Margin="10,394,0,0" 
                     TextWrapping="Wrap" Text="" VerticalAlignment="Top" Width="174"/>

            <TextBox x:Name="txbxPlayerScore" FontSize="23" FontWeight="Bold" TextAlignment="Center" Padding="0 -6 0 0" BorderBrush="Black" Grid.Column="1" HorizontalAlignment="Left" Height="24" Margin="10,520,0,0" 
                     TextWrapping="Wrap" VerticalAlignment="Top" Width="174"/>

            <TextBox IsEnabled="False" BorderBrush="Black"  TextAlignment="Center" FontSize="15" Grid.Column="1" HorizontalAlignment="Left" Height="24" Margin="10,370,0,0" TextWrapping="Wrap" 
                     Text="Player" VerticalAlignment="Top" Width="174"/>

            <TextBox Name="txbxCompsChoice" BorderBrush="Black" TextAlignment="Center" Padding="0, 35" FontSize="35" Grid.Column="1" HorizontalAlignment="Left" Height="126" Margin="10,46,0,0" 
                TextWrapping="Wrap" Text="?" VerticalAlignment="Top" Width="174"/>

            <TextBox IsEnabled="False" BorderBrush="Black"  TextAlignment="Center" FontSize="15" Grid.Column="1" HorizontalAlignment="Left" Height="24" Margin="10,22,0,0" TextWrapping="Wrap" 
                     Text="Computer" VerticalAlignment="Top" Width="174"/>

            <TextBox x:Name="txbxCompScore" FontSize="23" FontWeight="Bold" TextAlignment="Center" Padding="0 -6 0 0" BorderBrush="Black" Grid.Column="1" HorizontalAlignment="Left" Height="24" Margin="10,172,0,0" 
                     TextWrapping="Wrap" VerticalAlignment="Top" Width="174"/>

            <Button x:Name="btnCountdown" Content="PLAY" Margin="10,201,10,390" Grid.Column="1" Click="BtnCountdown_Click" FontSize="55">
                <Button.Template>
                    <ControlTemplate TargetType="Button">
                        <Grid>
                            <Ellipse Fill="MediumTurquoise"/>
                            <ContentPresenter HorizontalAlignment="Center" VerticalAlignment="Center"/>
                        </Grid>
                    </ControlTemplate>


                </Button.Template>
            </Button>

            <TextBox IsEnabled="False" Name="txbxLeft" TextAlignment="Center" Padding="0, 260, 0, 0" HorizontalAlignment="Left" Height="534" Margin="10,10,0,0" TextWrapping="Wrap" 
                     Text="Pick Rock, Paper or Scissors" VerticalAlignment="Top" Width="175"/>

            <TextBox IsEnabled="False" Name="txbxRight" TextAlignment="Center" Padding="0, 260, 0, 0" HorizontalAlignment="Left" Height="534" Margin="10,10,0,0" TextWrapping="Wrap"
                     Text="Press play to start.
                     First to 3 wins. " VerticalAlignment="Top" Width="175" Grid.Column="2"/>
        </Grid>
    </Grid>

</Window>

MainWindow.xaml.cs

using System;
using System.Windows;
using System.Windows.Controls;

namespace Rock_Paper_Scissors
{
    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow : Window
    {
        public MainWindow()
        {
            InitializeComponent();  
        }

        private System.Windows.Forms.Timer timer1;
        private int counter = 7;
        string ROCK = "ROCK";
        string PAPER = "PAPER";
        string SCISSORS = "SCISSORS";

        private void The_Game(object sender, EventArgs e)
        {
            counter--;
            if (counter == 6)
            {
                btnCountdown.FontSize = 95; btnCountdown.Content = "3";
            }
            if (counter == 5)
            {
                btnCountdown.Content = "2";
            }
            if (counter == 4)
            {
                btnCountdown.Content = "1";
            }
            if (counter == 3)
            {
                btnCountdown.FontSize = 55; btnCountdown.Content = "SHOW";
                Comp_Choice();
                btnRock.IsEnabled = btnPaper.IsEnabled = btnScissors.IsEnabled = false;

                if ((txbxPlayersChoice.Text == ROCK && txbxCompsChoice.Text == PAPER)
                    || (txbxPlayersChoice.Text == PAPER && txbxCompsChoice.Text == SCISSORS)
                    || (txbxPlayersChoice.Text == SCISSORS && txbxCompsChoice.Text == ROCK))
                {
                    txbxLeft.Text = txbxRight.Text = "You Lose"; txbxCompScore.Text = txbxCompScore.Text + "X";
                }
                if ((txbxPlayersChoice.Text == PAPER && txbxCompsChoice.Text == ROCK)
                    || (txbxPlayersChoice.Text == ROCK && txbxCompsChoice.Text == SCISSORS)
                    || (txbxPlayersChoice.Text == SCISSORS && txbxCompsChoice.Text == PAPER))
                {
                    txbxLeft.Text = txbxRight.Text = "You Win"; txbxPlayerScore.Text = txbxPlayerScore.Text + "X";
                }
                if ((txbxPlayersChoice.Text == ROCK && txbxCompsChoice.Text == ROCK)
                    || (txbxPlayersChoice.Text == PAPER && txbxCompsChoice.Text == PAPER)
                    || (txbxPlayersChoice.Text == SCISSORS && txbxCompsChoice.Text == SCISSORS))
                {
                    txbxLeft.Text = txbxRight.Text = "DRAW";
                }
            }
            if (counter == 0)
            {
                timer1.Stop();
                btnCountdown.FontSize = 25; btnCountdown.Content = "PLAY AGAIN";
                txbxCompsChoice.Text = "?"; txbxPlayersChoice.Text = "";
                txbxLeft.Text = "Pick Rock, Paper or Scissors"; txbxRight.Text = "Click Play Again to start";
                btnCountdown.IsEnabled = true;
                counter = 7;

                if (txbxPlayerScore.Text == "XXX")
                {
                    txbxLeft.Text = "GAME OVER"; txbxRight.Text = "YOU WIN!";
                    btnCountdown.Content = "  YOU WIN\nPLAY AGAIN";
                }
                if(txbxCompScore.Text == "XXX")
                {
                    txbxLeft.Text = "GAME OVER"; txbxRight.Text = "YOU LOSE!";
                    btnCountdown.Content = " YOU LOSE\nPLAY AGAIN";
                }
                btnRock.IsEnabled = btnPaper.IsEnabled = btnScissors.IsEnabled = true;
            }
        }

        private void BtnCountdown_Click(object sender, RoutedEventArgs e)
        {
            if(btnCountdown.IsPressed == true)
            {
                btnCountdown.FontSize = 32;
            }
            timer1 = new System.Windows.Forms.Timer();
            timer1.Tick += new EventHandler(The_Game);
            timer1.Interval = 1000; timer1.Start();
            btnCountdown.IsEnabled = false;

            if (txbxLeft.Text == "GAME OVER")
            {
                txbxPlayerScore.Text = ""; txbxCompScore.Text = "";
            }
        }

        public void Comp_Choice()
        {
            string[] comp_choices = { ROCK, PAPER, SCISSORS };
            Random rand = new Random();
            int rand_index = rand.Next(comp_choices.Length);
            txbxCompsChoice.Text = comp_choices[rand_index];
        }

        private void Player_Choice(object sender, RoutedEventArgs e)
        {    
            Button clicked = (Button)sender;
            txbxPlayersChoice.Text = clicked.Content.ToString();
        }
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ Do you wanna go the MVVM way? \$\endgroup\$
    – Dbuggy
    Nov 22, 2019 at 10:45
  • \$\begingroup\$ Sure thing - I'd like to see how that would look at least \$\endgroup\$ Nov 22, 2019 at 14:10
  • 4
    \$\begingroup\$ Number one piece of advice here is do not put multiple short statements on one line. That is not idiomatic in C# and it is hard to read. \$\endgroup\$ Nov 22, 2019 at 20:44
  • 3
    \$\begingroup\$ Number two piece of advice is: keep the game logic and the UI logic separate. That's what Dbuggy is getting at; the model -- the state of the game -- and the user interface -- the view -- are logically separate but you've rolled them all up together. \$\endgroup\$ Nov 22, 2019 at 20:48
  • \$\begingroup\$ Thanks for your reply Eric. I had a feeling the multiple short statements on one line would be a no, but wanted to test the water. I understand what you're saying about breaking it up into more logical classes/methods but can't see how to approach that, can you briefly advise please? \$\endgroup\$ Nov 22, 2019 at 21:23

2 Answers 2

1
\$\begingroup\$

Just moving Eric's comments to an actual answer.

  1. Number one piece of advice here is do not put multiple short statements on one line. That is not idiomatic in C# and it is hard to read.

  2. Number two piece of advice is: keep the game logic and the UI logic separate. That's what Dbuggy is getting at; the model -- the state of the game -- and the user interface -- the view -- are logically separate but you've rolled them all up together.

\$\endgroup\$
1
\$\begingroup\$
string ROCK = "ROCK";
string PAPER = "PAPER";
string SCISSORS = "SCISSORS";  

These should be constants because the values won't change. You have some more magic strings which should be converted to constants as well.

Well, naming things is hard but one should stick to some guidelines. If you don't have guidelines I would suggest using the .NET Naming Guidelines. What really is important about naming is that one can grasp at first glance what the code is about. Think about if you or Sam the maintainer is looking at the code in a few months, do you still know what the abbreviations are meant for?

E.g here

public void Comp_Choice()
{
    string[] comp_choices = { ROCK, PAPER, SCISSORS };
    Random rand = new Random();
    int rand_index = rand.Next(comp_choices.Length);
    txbxCompsChoice.Text = comp_choices[rand_index];
}  

you have at least 4 times used abbreviations for naming things. But this method has some more issues. Each time this method is called you are creating string[] comp_choices which could be done at class-level and used each time.

Random isn't really random and if not using .NET Core one should make it a class-level variable which is intialized inside the constructor or at class intialization, because it is initialized using the current time as seed. Meaning if e.g this method is called very fast in a row it would produce the same values.

Based on the mentione naming guidelines methdos should be named using PascalCase casing and fields should be named using camelCase casing. Snake_case casing usually isn't used in .NET development.

I would prefer the method in question returning a string than beeing void.


Checking for a Draw could be made first because it is the easiest and can be the shortest piece of code. In addition if a Draw happens you only need to check if either the user or the computer has won. Hence I suggest to change the logic like so

if ((txbxPlayersChoice.Text == txbxCompsChoice.Text )
{
    txbxLeft.Text = txbxRight.Text = "DRAW";
}
else if ((txbxPlayersChoice.Text == ROCK && txbxCompsChoice.Text == PAPER)
    || (txbxPlayersChoice.Text == PAPER && txbxCompsChoice.Text == SCISSORS)
    || (txbxPlayersChoice.Text == SCISSORS && txbxCompsChoice.Text == ROCK))
{
    txbxLeft.Text = txbxRight.Text = "You Lose"; 
    txbxCompScore.Text = txbxCompScore.Text + "X";
}
else 
{
    txbxLeft.Text = txbxRight.Text = "You Win"; 
    txbxPlayerScore.Text = txbxPlayerScore.Text + "X";
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.