Beginning C# dice game

Question

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

• Caridorc: Does not work on Ubuntu, is this Windows specific?
• Whoisit: Jamal, thanks for looking yes it is windows specific.
• radarbob: A link to another Dice Game question

Answer 1

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".
 }
}

• Whoisit: Eurotrash, thanks for looking what say is clear and understandable, I'll try to remember all advice you have given for next project.
• pacmaninbw: 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.

Answer 2

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

• Whoisit: 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.
• pacmaninbw: @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.
• Whoisit: I've clearly a lot to learn to learn and a long way to go! But I guess we all have to start somewhere.