2
\$\begingroup\$

I've been learning C# for about a month and have a finished game. I would like some constructive feedback on my progress so far.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Threading;
using System.Globalization;

namespace Snake_Eyes
{
public partial class Form1 : Form
{
    Panel[] playerPanel = new Panel[5]; //Array to hold player panels
    Label[] playerName = new Label[5]; //Array to hold player name labels
    Label[] playerScore = new Label[5]; // Array to hold player score lbels
    PictureBox[] playerPic = new PictureBox[5]; // Array to hold players picture
    int[] playerTotalScore = new int[5]; // Array to hold players Total score

    private Random RandomClass = new Random(); // Generates numbers for dice

    public Form1()
    {
        InitializeComponent();

        playerPanel[0] = panel1;
        playerPanel[1] = panel2;
        playerPanel[2] = panel3; // Fill playerPanel array with Players Panels
        playerPanel[3] = panel4;
        playerPanel[4] = panel5;

        playerName[0] = lblPlay1;
        playerName[1] = lblPlay2;
        playerName[2] = lblPlay3; // Fill playerName array with Players name Labels
        playerName[3] = lblPlay4;
        playerName[4] = lblPlay5;

        playerPic[0] = picPlay1;
        playerPic[1] = picPlay2;
        playerPic[2] = picPlay3; // Fill playerPic array with Players Pictureboxes
        playerPic[3] = picPlay4;
        playerPic[4] = picPlay5;

        playerScore[0] = lblScore1;
        playerScore[1] = lblScore2;
        playerScore[2] = lblScore3; // Fill playerScore with players Score Labels
        playerScore[3] = lblScore4;
        playerScore[4] = lblScore5;

        PictureBox picBox;
        btnYes.Location = new Point(155, 240);//======================================
        btnNo.Location = new Point(350, 240);
        txbEnterName.Location = new Point(210, 250);// on form
        pbDice1.Location = new Point(185, 290);
        pbDice2.Location = new Point(375, 290);//=======================================


        for (int i = 0; i < 14; i++) // There are 14 pictures for players to choose from in resources
        {
            picBox = new PictureBox();
            picBox.Size = new Size(75, 75);
            picBox.BorderStyle = BorderStyle.Fixed3D;
            tlpSelectPic.Controls.Add(picBox); // Add picture boxes to picture panel
            picBox.Image = (Bitmap)Properties.Resources.ResourceManager.GetObject("pic" + i); // Place image in picturebox
            picBox.Click += new EventHandler(picBox_Click); // Enable picture to be clicked 
        }

    }

    int diceNumber1; // var to show value of first dice
    int diceNumber2; // var to show value of second dice
    int choice; // var to see how many players have been selacted
    int playerNum = 0; // var to keep track of how many players have entered their info
    int endGame = 0; // var to set end of game, 

    //================================================
    // Method to get number of players
    //================================================
    private void playerNumbers() // Check number of players chosen, if confirmed delete remainder using method delPlayers()
    {
        DialogResult totalPlayers = MessageBox.Show(choice + " Player Game", "Number of Players", MessageBoxButtons.YesNo, MessageBoxIcon.Question, MessageBoxDefaultButton.Button1);
        if (totalPlayers == DialogResult.Yes) 

        {
            delPlayers(); // Call method
        }
        else
        {
            switch (choice) // If selection mistakenly made, clear selected radio button
            {
                case 1:
                    optOne.Checked = false;
                    break;
                case 2:
                    optTwo.Checked = false;
                    break;
                case 3:
                    optThree.Checked = false;
                    break;
                case 4:
                    optFour.Checked = false;
                    break;
                case 5:
                    optFive.Checked = false;
                    break;
            }
        }
    }

    //==================================================
    // Method to remove players
    // =================================================
    private void delPlayers() // Method to delete redundant players
    {
        if (choice == 5) // No player panels need deleting so let players start entering names 
        {
            pnlPlayerNumbers.Visible = false; // Hide player numbers panel
            lblMessage.Text = (playerName[playerNum].Text + " Enter your Name and press Enter");// Ask next player to enter their name
            txbEnterName.Visible = true;
            txbEnterName.Focus(); // Give focus to textbox
        }
        else  //Remove the player panels not required
        {
            for (int i = choice; i < 5; i++)
            {
                playerPanel[i].Visible = false; // Remove player panels not needed
                pnlPlayerNumbers.Visible = false; //==============================
                lblMessage.Text = (playerName[playerNum].Text + " Enter your Name and press Enter");// Ask next player to enter their name
                txbEnterName.Visible = true;
                txbEnterName.Focus();                 

            }
        }
     }

    //=============================================
    // Method to let players choose a picture
    // ============================================
    private void picBox_Click(object sender, EventArgs e) // Method to allow players to pic a picture
    {
        var pb = (PictureBox)sender;
        playerPic[playerNum].Image = pb.Image; // Place the image chosen into players pictureBox
        tlpSelectPic.Visible = false; // Hide picture panel

        if (playerPic[choice - 1].Image != null) // If last player has picked their picture let game begin
        {
            txbEnterName.Visible = false;
            btnYes.Visible = true;
            btnNo.Visible = true;
            pbDice1.Visible = true;
            pbDice2.Visible = true;
            playerNum = 0;
            lblMessage.Text = (playerName[playerNum].Text + " Roll Dice"); 
        }
        else
        {
            txbEnterName.Visible = true;
            txbEnterName.Text = ""; //Clear txtbox so next player can enter their name
            txbEnterName.Focus(); // Give focus back to textbox
            playerNum++; //Add one ready for next button click
            lblMessage.Text = (playerName[playerNum].Text + " Enter your Name and press Enter");// Ask next player to enter their name
        }
    } 

    private void pictureBox1_Click(object sender, EventArgs e)
    {

    }

    //===========================================================
    // Method to pick how many players will play
    // ==========================================================
    private void optOne_CheckedChanged(object sender, EventArgs e)
    {
        choice = 1;
        playerNumbers();
    }
    private void optTwo_CheckedChanged(object sender, EventArgs e)
    {
        choice = 2;
        playerNumbers();
    }
    private void optThree_CheckedChanged(object sender, EventArgs e)
    {
        choice = 3;
        playerNumbers();
    }
    private void optFour_CheckedChanged(object sender, EventArgs e)
    {
        choice = 4;
        playerNumbers();
    }
    private void optFive_CheckedChanged(object sender, EventArgs e)
    {
        choice = 5;
        playerNumbers ();
    }
    //==================================================
    // Method to check if all players have played
    // =================================================
    private void GameOver()
    {
        //===================================================
        //Using DataGridView
        //====================================================
        pbDice1.Visible = false;
        pbDice2.Visible = false;
            dgvHighScore.Visible = true;

            //Set up Datagrid Headings
            dgvHighScore.ColumnCount = 2;
            DataGridViewImageColumn img = new DataGridViewImageColumn();
            img.Name = "img";
            //img.HeaderText = "MugShot";
            dgvHighScore.Columns.Add(img);

            //dgvHighScore.Columns[0].Name = "Name";
            //dgvHighScore.Columns[1].Name = "Score";
            dgvHighScore.Columns[1].SortMode = DataGridViewColumnSortMode.NotSortable; // Remove sort glyph
             //Fill Rows with information

             for (int i = 0; i < choice; i++)
            {
                dgvHighScore.Rows.Add(playerName[i].Text, playerTotalScore[i]); // Show players Total score
                dgvHighScore.Rows[i].Cells["img"].Value = playerPic[i].Image; // Show players picture
            }

        //Sort by highest score
            dgvHighScore.Sort(dgvHighScore.Columns[1], ListSortDirection.Descending);
            dgvHighScore.Size = new Size(280,375);
            dgvHighScore.Location = new Point(185, 60);
            btnYes.Visible = false;//======================
            btnNo.Visible = false;
            pbDice1.Visible = false; //Hide All
            pbDice2.Visible = false;
            pnlPlayerInfo.Visible = false;//====================
            // Change position, size and text of label
            lblMessage.Size = new Size(350, 50);
            lblMessage.Location = new Point(150, 5);
            lblMessage  .Text = ("GAME OVER!  " + dgvHighScore[0,0].FormattedValue.ToString() + "  WINS");
    }

    //============================================
    // Method to check if play carries on or ends
    // ============================================
    private void nextPlayer()
    {
        endGame++;
        playerNum++;
        runTotal = 0;// Reset for next player              
        Delayed(3000);
    }

    private void Form1_Load(object sender, EventArgs e)
    {
        lblMessage.Text = ("Choose how many Players will play.");
    }

    //================================================
    // Method to let players roll dice
    // ================================================
    private void btnYes_Click(object sender, EventArgs e)
    {
        // each integer now becomes a random number between 1 and 7 (but will include 1)
        // thanks to our previously declared random class
        diceNumber1 = RandomClass.Next(1, 7);
        diceNumber2 = RandomClass.Next(1, 7);

        int total = diceNumber1 + diceNumber2;

        // Display the values rolled.
        pbDice1.Image = (Bitmap)Properties.Resources.ResourceManager.GetObject("dice" + diceNumber1);
        pbDice2.Image = (Bitmap)Properties.Resources.ResourceManager.GetObject("dice" + diceNumber2);
        runTotal = runTotal + total;
        lblMessage.Text = (playerName[playerNum].Text + " rolled " + diceNumber1.ToString() + " and " + diceNumber2.ToString() + "   Total score this throw = " + total);

       if (diceNumber1 == 1 && diceNumber2 == 1)
        {
            playerScore[playerNum] = playerScore[playerNum];
            lblMessage.Text = ("SNAKE EYES   " + playerName[playerNum].Text + "'s score = 0 and turn ends");
            runTotal = 0; 
            playerTotalScore[playerNum] = runTotal; // Save totalscore as 0
            playerScore[playerNum].Text = runTotal.ToString();// Show total as 0
            nextPlayer();
        }
        else if(diceNumber1 == 1 || diceNumber2 == 1)
        {
            lblMessage.Text = (playerName[playerNum].Text + " rolled a 1          " + playerName[playerNum].Text + "'s turn ends");
            runTotal = runTotal - total; // Remove total of last throw
            nextPlayer();
        }
        else
        {
            playerTotalScore[playerNum] = runTotal; // Keep a running total of players score to show in high score
            playerScore[playerNum].Text = runTotal.ToString();// Update players score label
        }
    }
    public static int counter;
    //================================================
    // Method to give players option to end turn
    // ===============================================
    private void btnNo_Click(object sender, EventArgs e)
    {
        playerScore[playerNum] = playerScore[playerNum];
        lblMessage.Text = (playerName[playerNum].Text + " gives up turn");
        nextPlayer();
     }

    //======================================
    // Method to pause game for set amount of time
    //======================================
    public void Delayed(int delay)
    {
        System.Windows.Forms.Timer timer = new System.Windows.Forms.Timer();
        timer.Interval = delay;
        timer.Tick += (s, e) =>
        {
            if (endGame < choice)
            {
                timer.Stop();
                btnYes.Visible = true;
                btnNo.Visible = true;
                lblMessage.Text = (playerName[playerNum].Text + " Roll Dice");
            }
            else
            {
                timer.Stop();
                GameOver();
            }

        };
                timer.Start();
                btnYes.Visible = false;
                btnNo.Visible = false;
            }

    private void tmrPause_Tick(object sender, EventArgs e)
    {

    }//Not used

    private void pnlEnterName_Paint(object sender, PaintEventArgs e)
    {

    }//Not used

    private void lblResult_Click(object sender, EventArgs e)
    {

    }//Not used
    //======================================
    //Method to let users enter their names
    //======================================
    private void txbEnterName_KeyPress(object sender, KeyPressEventArgs e)
    {
        if (e.KeyChar == (char)Keys.Enter)
        {
            if (string.IsNullOrEmpty(txbEnterName.Text)) // Check to make sure player has entered text
            {
                MessageBox.Show("You must enter a name", "Nothing Entered", MessageBoxButtons.OK, MessageBoxIcon.Exclamation);
            }
            else
            {

                playerName[playerNum].Text = CultureInfo.CurrentCulture.TextInfo.ToTitleCase(txbEnterName.Text); //Place the players name in their name label
                txbEnterName.Visible = false;
                tlpSelectPic.Visible = true; // Show pictures           
                lblMessage.Text = (playerName[playerNum].Text + " Choose your picture"); // Player information
            }
        }
    }
    int runTotal = 0;// Reset for next player
}
}
\$\endgroup\$
  • \$\begingroup\$ Does not work on Ubuntu, is this Windows specific? \$\endgroup\$ – Caridorc Mar 12 '16 at 11:12
  • \$\begingroup\$ Jamal, thanks for looking yes it is windows specific. \$\endgroup\$ – Whoisit Mar 13 '16 at 12:21
  • \$\begingroup\$ A link to another Dice Game question \$\endgroup\$ – radarbob Mar 14 '16 at 2:59
1
\$\begingroup\$

I'm going to focus on your comments. Your code is littered with comments saying what the code is doing.

Some are completely redundant as they state the obvious:

int[] playerTotalScore = new int[5]; // Array to hold players Total score

delPlayers(); // Call method

txbEnterName.Focus(); // Give focus to textbox

Some actually do add clarity to the code:

int choice; // var to see how many players have been selected

runTotal = 0;// Reset for next player

if (playerPic[choice - 1].Image != null) // If last player has picked their picture let game begin

In either case my advice is the same: A comment should never be required to say what your code is doing. If you feel like adding a comment to say what's going on, then it's time to move that code into an appropriately named method (or name your variables better so the logic is clearer).

For example:

//==================================================
// Method to remove players
// =================================================
private void delPlayers() // Method to delete redundant players
{
  if (choice == 5) // No player panels need deleting so let players start entering names 
  {
      pnlPlayerNumbers.Visible = false; // Hide player numbers panel
      lblMessage.Text = (playerName[playerNum].Text + " Enter your Name and press Enter");// Ask next player to enter their name
      txbEnterName.Visible = true;
      txbEnterName.Focus(); // Give focus to textbox
  }
  else  //Remove the player panels not required
  {
      for (int i = choice; i < 5; i++)
      {
          playerPanel[i].Visible = false; // Remove player panels not needed
          pnlPlayerNumbers.Visible = false; //==============================
          lblMessage.Text = (playerName[playerNum].Text + " Enter your Name and press Enter");// Ask next player to enter their name
          txbEnterName.Visible = true;
          txbEnterName.Focus();                 

      }
  }
}

If you name your method better, you won't need a comment explaining what it does. private void RemoveRedundantPlayers() (Also you should follow standard naming conventions and use PascalCase for method names.)

if (choice == 5) // No player panels need deleting so let players start entering names

Could be

if (!RedundantPlayersExist())

or

var redundantPlayersExist = choice == 5; // TODO: rename the "choice" variable because it makes no sense in this context
if (!redundantPlayersExist)

(You could have either a method or a variable that you check in there, the point is that what you're checking in your if statement should be self-explanatory without the need for comment.

The code in this method also raises another issue, which is that this method is ostensibly about deleting/removing redundant players, but actually all the code executed if choice == 5 is true is doing something else; as per your comment:

No player panels need deleting so let players start entering names

You shouldn't be letting players start entering names in a method which is supposed to be about deleting redundant players.

So the outline of your method would be better as:

private void RemoveRedundantPlayers()
{
    if (redundantPlayersExist) 
    {
        // code removing redundant players goes here, or maybe this can loop through the players,
        // and for each redundant player found, it passes the player to another method that actually performs the "removal".
    }
}
\$\endgroup\$
  • \$\begingroup\$ Eurotrash, thanks for looking what say is clear and understandable, I'll try to remember all advice you have given for next project. \$\endgroup\$ – Whoisit Mar 13 '16 at 12:29
  • \$\begingroup\$ What eurotrash has implied but not stated clearly is that when software is maintained you have to maintain the comments as well, since comments don't actually do anything it's better to have self documenting code rather than a lot of comments. \$\endgroup\$ – pacmaninbw Mar 13 '16 at 12:56
1
\$\begingroup\$

First and foremost you are using what professionals call MAGIC NUMBERS, examples are 5 for the size of the arrays and 14 for the number of pictures. These should be named constants such as

public const int MaximumPlayerCount = 5;
public const int MaximumPlayerImageCount = 14;

The reason to create constants is that it is easier for someone else to modify the code and it is much easier to modify the code if you only need to change a number in one place, for example if you wanted to change the maximum number of players you would change the value of MaximumPlayerCount rather than having to change the following:

    Panel[] playerPanel = new Panel[5];
    Label[] playerName = new Label[5]; 
    Label[] playerScore = new Label[5];
    PictureBox[] playerPic = new PictureBox[5]; picture
    int[] playerTotalScore = new int[5];

    if (choice == 5) {}
    for (int i = choice; i < 5; i++) {}

versus

    public const int MaximumPlayerCount = 12; // was originally 5
    Panel[] playerPanel = new Panel[MaximumPlayerCount];
    Label[] playerName = new Label[MaximumPlayerCount]; 
    Label[] playerScore = new Label[MaximumPlayerCount];
    PictureBox[] playerPic = new PictureBox[MaximumPlayerCount];
    int[] playerTotalScore = new int[MaximumPlayerCount];
    if (choice == MaximumPlayerCount) {}
    or (int i = choice; i < MaximumPlayerCount; i++) {}

As someone else mentioned, creating a player class would be better than having multiple arrays,

    public partial class Player
    {
        protected Panel panel;
        protected Label name;
        protected Label score;
        protected PictureBox pic;
        protected int totalScore;
        Player(Panel panel, Label Name, Label Score, PictureBox Pic, int TotalScore) 
        public Panel GetPanel();
        public SetPanel(Panel panel);
        ...
    }

It would be better to only create the players you need rather than to remove redundant players.

If you can, remove all the unused functions, rather than commenting unused (you missed pictureBox1_Click).

The following functions are possibly unused as well: optOne_CheckedChanged() optTwo_CheckedChanged() optThree_CheckedChanged() optFour_CheckedChanged() optFive_CheckedChanged()

Your switch statement has no default case, this can lead to unknown states in your program.

You can use the array.add() method to add players (see https://msdn.microsoft.com/en-us/library/system.collections.arraylist.add%28v=vs.110%29.aspx).

public const int NoScoreYet = 0;
Players.add(Player(panel, name, score, pic, NoScoreYet); 
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks Pacman, the comments you have left will definitely help me to understand and code better, need to go away and look at litrature on creating classes. \$\endgroup\$ – Whoisit Mar 13 '16 at 12:25
  • \$\begingroup\$ @Whoisit When you start creating classes, remember 1 rule, Keep It Simple (KIS). You want a class for managing a player, you want another class for displaying a player, you want another class for displaying the game. \$\endgroup\$ – pacmaninbw Mar 13 '16 at 12:39
  • \$\begingroup\$ I've clearly a lot to learn to learn and a long way to go! But I guess we all have to start somewhere. \$\endgroup\$ – Whoisit Mar 13 '16 at 16:27

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.