I am a beginner in C# and trying to learn how to program with OOP in mind. I am 100% that the code is not OOP-friendly at all, but the game is running and working, so if anyone could look throw the code and give me feedback on how to improve it, that would be helpful.
The game is a simple snake game with three players option. I have done two players part, but the code seems to be ugly and messy, so I didn't want to add anything more to the garbage, and I am completely ok if I have to redo everything I am doing this to learn anyways
Form1.cs
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Diagnostics;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
namespace snakeGame3players
{
public partial class Form1 : Form
{
private readonly List<Square> SnakeBoddy = new List<Square>();
private readonly List<Square> SnakeBoddy2 = new List<Square>();
private readonly List<Square> SnakeFoods1 = new List<Square>();
private readonly List<Square> SnakeFoods2 = new List<Square>();
private readonly List<Square> SnakeFoods3 = new List<Square>();
private readonly List<Square> SnakeFoods4 = new List<Square>();
System.Windows.Forms.Timer eventTimer = new System.Windows.Forms.Timer();
System.Windows.Forms.Timer foodTimer = new System.Windows.Forms.Timer();
System.Windows.Forms.Timer foodTimer2 = new System.Windows.Forms.Timer();
System.Windows.Forms.Timer foodTimer3 = new System.Windows.Forms.Timer();
System.Windows.Forms.Timer foodTimer4 = new System.Windows.Forms.Timer();
Random random = new Random();
bool exitFlag = false;
public Form1()
{
InitializeComponent();
choice1.Appearance = System.Windows.Forms.Appearance.Button;
choice2.Appearance = System.Windows.Forms.Appearance.Button;
choice3.Appearance = System.Windows.Forms.Appearance.Button;
choice1.Size = new Size(154, 56);
choice2.Size = new Size(154, 56);
choice3.Size = new Size(154, 56);
new Player1setting();
new Player2setting();
new FoodSetting();
GameTimer.Interval = 100;
this.KeyPreview = true;
GameTimer.Tick += UpdateScreen;
GameTimer.Start();
label3.Visible = false;
label2.Visible = false;
}
private void UpdateScreen(object sender, EventArgs e)
{
if (label1.Visible == true)
{
if (KeyInput.KeyInputs(Keys.Q))
{
Application.Restart();
Environment.Exit(0);
}
}
if (choice2.Checked && Player1setting.Gameover ==false)
{
if (KeyInput.KeyInputs(Keys.Right) && Player1setting.Movment != SnakeMovment.Left)
{
Player1setting.Movment = SnakeMovment.Right;
}
else if (KeyInput.KeyInputs(Keys.Left) && Player1setting.Movment != SnakeMovment.Right)
{
Player1setting.Movment = SnakeMovment.Left;
}
else if (KeyInput.KeyInputs(Keys.Up) && Player1setting.Movment != SnakeMovment.Down)
{
Player1setting.Movment = SnakeMovment.Up;
}
else if (KeyInput.KeyInputs(Keys.Down) && Player1setting.Movment != SnakeMovment.Up)
{
Player1setting.Movment = SnakeMovment.Down;
}
PlayerMovment();
}
pictureBox1.Invalidate();
if (choice1.Checked && Player2setting.Gameover2 == false)
{
if (KeyInput.KeyInputs(Keys.D) && Player2setting.Movment2 != SnakeMovment2.A)
{
Player2setting.Movment2 = SnakeMovment2.D;
}
else if (KeyInput.KeyInputs(Keys.A) && Player2setting.Movment2 != SnakeMovment2.D)
{
Player2setting.Movment2 = SnakeMovment2.A;
}
else if (KeyInput.KeyInputs(Keys.W) && Player2setting.Movment2 != SnakeMovment2.S)
{
Player2setting.Movment2 = SnakeMovment2.W;
}
else if (KeyInput.KeyInputs(Keys.S) && Player2setting.Movment2 != SnakeMovment2.W)
{
Player2setting.Movment2 = SnakeMovment2.S;
}
PlayerMovment2();
}
pictureBox1.Invalidate();
}
private void PlayerMovment()
{
if (choice2.Checked) {
for (int i = SnakeBoddy.Count - 1; i >= 0; i--)
{
if (i == 0)
{
switch (Player1setting.Movment)
{
case SnakeMovment.Right:
SnakeBoddy[i].X++;
break;
case SnakeMovment.Left:
SnakeBoddy[i].X--;
break;
case SnakeMovment.Up:
SnakeBoddy[i].Y--;
break;
case SnakeMovment.Down:
SnakeBoddy[i].Y++;
break;
}
int maxXPosition = pictureBox1.Size.Width / Player1setting.Width;
int maxYPosition = pictureBox1.Size.Height / Player1setting.Height;
if (
SnakeBoddy[i].X < 0 || SnakeBoddy[i].Y < 0 ||
SnakeBoddy[i].X > maxXPosition || SnakeBoddy[i].Y >= maxYPosition
)
{
Die();
}
for (int J = 1; J < SnakeBoddy.Count; J++)
{
if (SnakeBoddy[i].X == SnakeBoddy[J].X && SnakeBoddy[i].Y == SnakeBoddy[J].Y)
{
Die();
}
}
for (int g = 0; g < SnakeBoddy2.Count; g++)
{
if (SnakeBoddy[0].X == SnakeBoddy2[g].X && SnakeBoddy[0].Y == SnakeBoddy2[g].Y)
{
Player1setting.GameScore += FoodSetting.food2;
label3.Text = Player1setting.GameScore.ToString();
Die();
}
}
if (SnakeFoods1.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
EatFood();
}
if (SnakeFoods2.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
EatFood2();
}
if (SnakeFoods3.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
EatFood3();
}
if (SnakeFoods4.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
EatFood4();
}
}
else
{
SnakeBoddy[i].X = SnakeBoddy[i - 1].X;
SnakeBoddy[i].Y = SnakeBoddy[i - 1].Y;
}
}
}
}
private void PlayerMovment2()
{
if (choice1.Checked)
{
for (int x = SnakeBoddy2.Count - 1; x >= 0; x--)
{
if (x == 0)
{
switch (Player2setting.Movment2)
{
case SnakeMovment2.D:
SnakeBoddy2[x].X++;
break;
case SnakeMovment2.A:
SnakeBoddy2[x].X--;
break;
case SnakeMovment2.W:
SnakeBoddy2[x].Y--;
break;
case SnakeMovment2.S:
SnakeBoddy2[x].Y++;
break;
}
int maxXPosition2 = pictureBox1.Size.Width / Player2setting.Width2;
int maxYPosition2 = pictureBox1.Size.Height / Player2setting.Height2;
if (
SnakeBoddy2[x].X < 0 || SnakeBoddy2[x].Y < 0 ||
SnakeBoddy2[x].X > maxXPosition2 || SnakeBoddy2[x].Y >= maxYPosition2
)
{
Die2();
}
for (int h = 1; h < SnakeBoddy2.Count; h++)
{
if (SnakeBoddy2[x].X == SnakeBoddy2[h].X && SnakeBoddy2[x].Y == SnakeBoddy2[h].Y)
{
Die2();
}
}
for (int g = 0; g < SnakeBoddy.Count; g++)
{
if (SnakeBoddy2[0].X == SnakeBoddy[g].X && SnakeBoddy2[0].Y == SnakeBoddy[g].Y)
{
Player1setting.GameScore += FoodSetting.food2;
label2.Text = Player1setting.GameScore.ToString();
Die2();
}
}
if (SnakeFoods1.Any(s => s.X == SnakeBoddy2[0].X && s.Y == SnakeBoddy2[0].Y))
{
EatFood();
}
if (SnakeFoods2.Any(s => s.X == SnakeBoddy2[0].X && s.Y == SnakeBoddy2[0].Y))
{
EatFood2();
}
if (SnakeFoods3.Any(s => s.X == SnakeBoddy2[0].X && s.Y == SnakeBoddy2[0].Y))
{
EatFood3();
}
if (SnakeFoods4.Any(s => s.X == SnakeBoddy2[0].X && s.Y == SnakeBoddy2[0].Y))
{
EatFood4();
}
}
else
{
SnakeBoddy2[x].X = SnakeBoddy2[x - 1].X;
SnakeBoddy2[x].Y = SnakeBoddy2[x - 1].Y;
}
}
}
}
private void Form1_Load(object sender, EventArgs e)
{
}
private void Keyisup(object sender, KeyEventArgs e)
{
KeyInput.SnakeDirections(e.KeyCode, false);
}
private void Keyisdown(object sender, KeyEventArgs e)
{
KeyInput.SnakeDirections(e.KeyCode, true);
}
private void UpdateGame(object sender, PaintEventArgs e)
{
Graphics canvas = e.Graphics;
if (label1.Visible==false && Information.Visible==false)
{
foreach (Square SnakeFood in SnakeFoods1) {
canvas.FillRectangle(Brushes.Red,
new Rectangle(
SnakeFood.X * Player2setting.Width2,
SnakeFood.Y * Player2setting.Width2,
Player2setting.Width2, Player2setting.Height2
));
}
foreach (Square SnakeFoods2 in SnakeFoods2) {
canvas.FillRectangle(Brushes.Orange,
new Rectangle(
SnakeFoods2.X * Player1setting.Width,
SnakeFoods2.Y * Player1setting.Width,
Player1setting.Width, Player1setting.Height
));
}
foreach (Square SnakeFoods3 in SnakeFoods3) {
canvas.FillRectangle(Brushes.Purple,
new Rectangle(
SnakeFoods3.X * Player1setting.Width,
SnakeFoods3.Y * Player1setting.Width,
Player1setting.Width, Player1setting.Height
));
}
foreach (Square SnakeFoods4 in SnakeFoods4) {
canvas.FillRectangle(Brushes.Green,
new Rectangle(
SnakeFoods4.X * Player1setting.Width,
SnakeFoods4.Y * Player1setting.Width,
Player1setting.Width, Player1setting.Height
));
}
}
if (choice2.Checked)
{
if (Player1setting.Gameover == false)
{
Brush SnakeBoddyColor;
for (int i = 0; i < SnakeBoddy.Count; i++)
{
if (i == 0)
{
SnakeBoddyColor = Brushes.Black;
canvas.FillRectangle(SnakeBoddyColor,
new Rectangle(
SnakeBoddy[i].X * Player1setting.Width,
SnakeBoddy[i].Y * Player1setting.Width,
Player1setting.Width, Player1setting.Height
));
}
else
{
SnakeBoddyColor = Brushes.Red;
canvas.FillRectangle(SnakeBoddyColor,
new Rectangle(
SnakeBoddy[i].X * Player1setting.Width,
SnakeBoddy[i].Y * Player1setting.Width,
Player1setting.Width, Player1setting.Height
));
}
}
}
}
if (choice1.Checked)
{
if (Player2setting.Gameover2 == false)
{
Brush SnakeBoddyColor2;
for (int x = 0; x < SnakeBoddy2.Count; x++)
{
if (x == 0)
{
SnakeBoddyColor2 = Brushes.Black;
canvas.FillRectangle(SnakeBoddyColor2,
new Rectangle(
SnakeBoddy2[x].X * Player2setting.Width2,
SnakeBoddy2[x].Y * Player2setting.Width2,
Player2setting.Width2, Player2setting.Height2
));
}
else
{
SnakeBoddyColor2 = Brushes.Blue;
canvas.FillRectangle(SnakeBoddyColor2,
new Rectangle(
SnakeBoddy2[x].X * Player2setting.Width2,
SnakeBoddy2[x].Y * Player2setting.Width2,
Player2setting.Width2, Player2setting.Height2
));
}
}
}
}
if (Player2setting.GameScore2 > Player1setting.GameScore && Player2setting.Gameover2 == true)
{
string gameEnd = "Player 1 \n" + "With the score of " + Player2setting.GameScore2 + "\nPress Q to play again \n";
label1.Text = gameEnd;
label3.Visible = false;
label2.Visible = false;
label1.Visible = true;
}
if (Player2setting.GameScore2 < Player1setting.GameScore && Player1setting.Gameover == true)
{
string gameEnd = "Player 2 \n" + "With the score of " + Player1setting.GameScore + "\nPress Q to play again\n";
label1.Text = gameEnd;
label3.Visible = false;
label2.Visible = false;
label1.Visible = true;
}
if (Player2setting.GameScore2 == Player1setting.GameScore && Player1setting.Gameover == true && Player2setting.Gameover2 == true)
{
string gameEnd = "Draw \nPress Q to play again\n";
label1.Text = gameEnd;
label3.Visible = false;
label2.Visible = false;
label1.Visible = true;
}
}
private void StartGame()
{
pictureBox1.Visible = false;
Information.Visible = false;
Player1Information.Visible = false;
Player2Information.Visible = false;
Player3Information.Visible = false;
choice1.Visible = false;
choice2.Visible = false;
choice3.Visible = false;
GameStart.Visible = false;
pictureBox3.Visible = false;
pictureBox1.Visible = true;
label1.Visible = false;
new FoodSetting();
new Player1setting();
new Player2setting();
if (choice2.Checked)
{
label2.Visible = true;
SnakeBoddy.Clear();
Square SneakHead = new Square { X = 35, Y = 5 };
SnakeBoddy.Add(SneakHead);
if (choice2.Checked && !choice1.Checked)
{
Square SneakHead2 = new Square { X = 35, Y = 5 };
Player2setting.GameScore2 = -1;
SnakeBoddy2.Add(SneakHead2);
}
}
if (choice1.Checked)
{
label3.Visible = true;
SnakeBoddy2.Clear();
Square SneakHead2 = new Square { X = 10, Y = 5 };
SnakeBoddy2.Add(SneakHead2);
if (choice1.Checked && !choice2.Checked)
{
Square SneakHead = new Square { X = 10, Y = 5 };
Player1setting.GameScore = -1;
SnakeBoddy.Add(SneakHead);
}
}
label2.Text = Player1setting.GameScore.ToString();
label3.Text = Player2setting.GameScore2.ToString();
FoodEvent();
}
private void GenerateSnakeFood()
{
Square newFood = GenerateRandomFood();
while (SnakeFoods1.Any(snakeFood => snakeFood.X == newFood.X && snakeFood.Y == newFood.Y))
{
newFood = GenerateRandomFood();
}
SnakeFoods1.Add(newFood);
}
private void GenerateSnakeFood2()
{
Square newFood = GenerateRandomFood();
while (SnakeFoods2.Any(snakeFood => snakeFood.X == newFood.X && snakeFood.Y == newFood.Y))
{
newFood = GenerateRandomFood();
}
SnakeFoods2.Add(newFood);
}
private void GenerateSnakeFood3()
{
Square newFood = GenerateRandomFood();
while (SnakeFoods3.Any(snakeFood => snakeFood.X == newFood.X && snakeFood.Y == newFood.Y))
{
newFood = GenerateRandomFood();
}
SnakeFoods3.Add(newFood);
}
private void GenerateSnakeFood4()
{
Square newFood = GenerateRandomFood();
while (SnakeFoods4.Any(snakeFood => snakeFood.X == newFood.X && snakeFood.Y == newFood.Y))
{
newFood = GenerateRandomFood();
}
SnakeFoods4.Add(newFood);
}
private Square GenerateRandomFood()
{
int maxXPosition = pictureBox1.Size.Width / Player1setting.Width;
int maxYPosition = pictureBox1.Size.Height / Player1setting.Height;
return new Square { X = random.Next(0, maxXPosition), Y = random.Next(0, maxYPosition) };
}
private void EatFood()
{
if (SnakeFoods1.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
Square foodBody = new Square
{
X = SnakeBoddy[SnakeBoddy.Count - 1].X,
Y = SnakeBoddy[SnakeBoddy.Count - 1].Y
};
SnakeBoddy.Add(foodBody);
Player1setting.GameScore += FoodSetting.food1;
label2.Text = Player1setting.GameScore.ToString();
var snakePosition = SnakeBoddy[0];
SnakeFoods1.RemoveAll(s => s.X == snakePosition.X && s.Y == snakePosition.Y);
}
if (SnakeFoods1.Any(s => s.X == SnakeBoddy2[0].X && s.Y == SnakeBoddy2[0].Y))
{
Square foodBody2 = new Square
{
X = SnakeBoddy2[SnakeBoddy2.Count - 1].X,
Y = SnakeBoddy2[SnakeBoddy2.Count - 1].Y
};
SnakeBoddy2.Add(foodBody2);
Player2setting.GameScore2 += FoodSetting.food1;
label3.Text = Player2setting.GameScore2.ToString();
var snakePosition = SnakeBoddy2[0];
SnakeFoods1.RemoveAll(s => s.X == snakePosition.X && s.Y == snakePosition.Y);
}
}
private void EatFood2()
{
if (SnakeFoods2.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
Square foodBody3 = new Square
{
X = SnakeBoddy[SnakeBoddy.Count - 1].X,
Y = SnakeBoddy[SnakeBoddy.Count - 1].Y
};
SnakeBoddy.Add(foodBody3);
Square foodBody35 = new Square
{
X = SnakeBoddy[SnakeBoddy.Count - 1].X,
Y = SnakeBoddy[SnakeBoddy.Count - 1].Y
};
SnakeBoddy.Add(foodBody35);
Player1setting.GameScore += FoodSetting.food2;
label2.Text = Player1setting.GameScore.ToString();
var snakePosition = SnakeBoddy[0];
SnakeFoods2.RemoveAll(s => s.X == snakePosition.X && s.Y == snakePosition.Y);
}
if (SnakeFoods2.Any(s => s.X == SnakeBoddy2[0].X && s.Y == SnakeBoddy2[0].Y))
{
Square foodBody4 = new Square
{
X = SnakeBoddy2[SnakeBoddy2.Count - 1].X,
Y = SnakeBoddy2[SnakeBoddy2.Count - 1].Y
};
SnakeBoddy2.Add(foodBody4);
Square foodBody45 = new Square
{
X = SnakeBoddy2[SnakeBoddy2.Count - 1].X,
Y = SnakeBoddy2[SnakeBoddy2.Count - 1].Y
};
SnakeBoddy2.Add(foodBody45);
Player2setting.GameScore2 += FoodSetting.food2;
label3.Text = Player2setting.GameScore2.ToString();
var snakePosition = SnakeBoddy2[0];
SnakeFoods2.RemoveAll(s => s.X == snakePosition.X && s.Y == snakePosition.Y);
}
}
private void EatFood3()
{
if (SnakeFoods3.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
if (SnakeBoddy.Count > 1)
{
SnakeBoddy.RemoveAt(SnakeBoddy.Count - 1);
}
Player1setting.GameScore += FoodSetting.food3;
label2.Text = Player1setting.GameScore.ToString();
var snakePosition = SnakeBoddy[0];
SnakeFoods3.RemoveAll(s => s.X == snakePosition.X && s.Y == snakePosition.Y);
}
if (SnakeFoods3.Any(s => s.X == SnakeBoddy2[0].X && s.Y == SnakeBoddy2[0].Y))
{
if (SnakeBoddy2.Count > 1)
{
SnakeBoddy2.RemoveAt(SnakeBoddy2.Count - 1);
}
Player2setting.GameScore2 += FoodSetting.food3;
label3.Text = Player2setting.GameScore2.ToString();
var snakePosition = SnakeBoddy2[0];
SnakeFoods3.RemoveAll(s => s.X == snakePosition.X && s.Y == snakePosition.Y);
}
}
private void EatFood4()
{
if (SnakeFoods4.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
Player1setting.GameScore += FoodSetting.food4;
label2.Text = Player1setting.GameScore.ToString();
var snakePosition = SnakeBoddy[0];
SnakeFoods4.RemoveAll(s => s.X == snakePosition.X && s.Y == snakePosition.Y);
RandomEevent();
}
if (SnakeFoods4.Any(s => s.X == SnakeBoddy2[0].X && s.Y == SnakeBoddy2[0].Y))
{
Player2setting.GameScore2 += FoodSetting.food4;
label3.Text = Player2setting.GameScore2.ToString();
var snakePosition = SnakeBoddy2[0];
SnakeFoods4.RemoveAll(s => s.X == snakePosition.X && s.Y == snakePosition.Y);
RandomEevent();
}
}
private void RandomEevent()
{
var time = DateTime.Now;
eventTimer.Tick += new EventHandler(TimerEventProcessor);
eventTimer.Interval = 3000;
eventTimer.Start();
while (exitFlag == false)
{
//Application.DoEvents();
if ((DateTime.Now - time).TotalSeconds > 30)
{
eventTimer.Stop();
exitFlag = true;
}
}
}
private void TimerEventProcessor(Object sender, EventArgs e)
{
eventTimer.Enabled = true;
Player2setting.Movment2 = (SnakeMovment2)random.Next(4);
}
private void FoodEvent()
{
foodTimer.Tick += new EventHandler(FoodGenerator);
foodTimer.Interval = random.Next(1000,4000);
foodTimer.Start();
foodTimer2.Tick += new EventHandler(FoodGenerator2);
foodTimer2.Interval = random.Next(4000, 7000);
foodTimer2.Start();
foodTimer3.Tick += new EventHandler(FoodGenerator3);
foodTimer3.Interval = random.Next(5000, 10000);
foodTimer3.Start();
foodTimer4.Tick += new EventHandler(FoodGenerator4);
foodTimer4.Interval = random.Next(3000, 6000);
foodTimer4.Start();
}
private void FoodGenerator(object sender, EventArgs e)
{
foodTimer.Enabled = true;
GenerateSnakeFood();
}
private void FoodGenerator2(object sender, EventArgs e)
{
foodTimer2.Enabled = true;
GenerateSnakeFood2();
}
private void FoodGenerator3(object sender, EventArgs e)
{
foodTimer3.Enabled = true;
GenerateSnakeFood3();
}
private void FoodGenerator4(object sender, EventArgs e)
{
foodTimer4.Enabled = true;
GenerateSnakeFood4();
}
private void Die()
{
Player1setting.Gameover = true;
}
private void Die2()
{
Player2setting.Gameover2 = true;
}
private void GameStart_Click(object sender, EventArgs e)
{
if (choice1.Checked || choice2.Checked || choice3.Checked) {
StartGame();
}
}
}
}
KeyInput.cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Collections;
using System.Windows.Forms;
namespace snakeGame3players
{
class KeyInput
{
private static Hashtable keyTable = new Hashtable();
public static bool KeyInputs(Keys key)
{
if (keyTable[key] == null)
{
return false;
}
return (bool)keyTable[key];
}
public static void SnakeDirections(Keys key, bool direction)
{
keyTable[key] = direction;
}
}
}
Player1setting.cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace snakeGame3players
{
public enum SnakeMovment
{
Right,
Left,
Down,
Up
};
public enum SnakeMovment2
{
W,
S,
D,
A
};
class Player1setting
{
public static int Width { get; set; }
public static int Height { get; set; }
public static int GameScore { get; set; }
public static bool Gameover { get; set; }
public static SnakeMovment Movment { get; set; }
public Player1setting()
{
Height = 16;
Width = 16;
Gameover = false;
Movment = SnakeMovment.Down;
GameScore = 0;
}
}
class Player2setting
{
public static int Width2 { get; set; }
public static int Height2 { get; set; }
public static int GameScore2 { get; set; }
public static bool Gameover2 { get; set; }
public static SnakeMovment2 Movment2 { get; set; }
public Player2setting()
{
Height2 = 16;
Width2 = 16;
Gameover2 = false;
Movment2 = SnakeMovment2.S;
GameScore2 = 0;
}
}
class FoodSetting
{
public static int food1 { get; set; }
public static int food2 { get; set; }
public static int food3 { get; set; }
public static int food4 { get; set; }
public FoodSetting()
{
food1 = 1;
food2 = 5;
food3 = 1;
food4 = 1;
}
}
}
Square.cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace snakeGame3players
{
class Square
{
public int X { get; set; }
public int Y { get; set; }
public Square()
{
X = 0;
Y = 0;
}
}
}
Program.cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using System.Windows.Forms;
namespace snakeGame3players
{
static class Program
{
/// <summary>
/// The main entry point for the application.
/// </summary>
[STAThread]
static void Main()
{
Application.EnableVisualStyles();
Application.SetCompatibleTextRenderingDefault(false);
Application.Run(new Form1());
}
}
}
2 Answers 2
OP has stated that (s)he is interested mainly about the UpdateScreen
.
So, in this review I try to focus only on that part of the application.
First, let me show you the end result and then let me iterate through the changes one-by-one:
private void UpdateScreen(object sender, EventArgs e)
{
if (label1.Visible)
{
if (KeyInput.IsKeyDown(Keys.Q))
{
Application.Restart();
Environment.Exit(0);
}
}
if (choice2.Checked)
{
playerOne.UpdateMovement();
UpdateSnakeBoddy();
}
pictureBox1.Invalidate();
if (choice1.Checked)
{
playerTwo.UpdateMovement();
UpdateSnakeBoddy2();
}
pictureBox1.Invalidate();
}
Use instances
- In your original solution you have a
Player1setting
class withstatic
properties. - In case of OOP you should work on a particular instance not on a global one.
So, lets get rid of the static
keywords:
class Player1setting
{
public int Width { get; set; }
public int Height { get; set; }
public int GameScore { get; set; }
public bool Gameover { get; set; }
public SnakeMovment Movment { get; set; }
public Player1setting()
{
Height = 16;
Width = 16;
Gameover = false;
Movment = SnakeMovment.Down;
GameScore = 0;
}
}
Then create instances:
//Class-wide private members
private readonly Player1setting playerOne;
private readonly Player2setting playerTwo;
public Form1()
{
InitializeComponent();
playerOne = new Player1setting();
playerTwo = new Player2setting();
...
}
And use them. So, we have to change all Player1setting.
and Player2setting.
to playerOne.
and playerTwo.
. For example:
if (KeyInput.KeyInputs(Keys.Right) && playerOne.Movment != SnakeMovment.Left)
{
playerOne.Movment = SnakeMovment.Right;
}
Use better naming
In the comments you can find the original
class PlayerOneSettings //Orig: Player1setting
{
public int Width { get; set; }
public int Height { get; set; }
public int GameScore { get; set; }
public bool IsGameOver { get; set; } //Orig: Gameover
public SnakeMovement Movement { get; set; } //Orig: SnakeMovment Movment
...
}
Please also rename the following methods:
PlayerMovment
toUpdateSnakeBoddy
PlayerMovment2
toUpdateSnakeBoddy2
In my opinion these names are more expressive because in those methods you are changing mainly those collections.
Encapsulate logic
The change of the Movement
should reside inside the Player{XYZ}Settings
classes.
So, let's introduce an UpdateMovement
method:
class PlayerOneSettings
{
...
public void UpdateMovement()
{
if (Player1setting.Gameover == false)
{
if (KeyInput.KeyInputs(Keys.Right) && Movement != SnakeMovement.Left)
{
Movement = SnakeMovement.Right;
}
else if (KeyInput.KeyInputs(Keys.Left) && Movement != SnakeMovement.Right)
{
Movement = SnakeMovement.Left;
}
else if (KeyInput.KeyInputs(Keys.Up) && Movement != SnakeMovement.Down)
{
Movement = SnakeMovement.Up;
}
else if (KeyInput.KeyInputs(Keys.Down) && Movement != SnakeMovement.Up)
{
Movement = SnakeMovement.Down;
}
}
}
}
Here I can access the class members (like Movement
) so I don't need to use player1
prefix.
We can also restrict the access of the Movement
's setter:
public SnakeMovement Movement { get; private set; }
Now the UpdateScreen
should look like as it is was stated above.
Reuse available components
The SnakeMovement
contains a subset of the Keys
enumeration.
Do not reinvent the wheel, try to re-use existing components.
class PlayerOneSettings
{
...
private readonly Keys[] ValidKeys = new[] { Keys.Right, Keys.Left, Keys.Up, Keys.Down };
}
Also we can define counterparts for each key:
class PlayerOneSettings
{
...
private readonly Keys[] ValidKeys = new[] { Keys.Right, Keys.Left, Keys.Up, Keys.Down };
private readonly Dictionary<Keys, Keys> KeyCounterpartMapping = new Dictionary<Keys, Keys>
{
{ Keys.Up, Keys.Down },
{ Keys.Down, Keys.Up },
{ Keys.Right, Keys.Left },
{ Keys.Left, Keys.Right },
};
}
With these in our hand we can greatly simplify the UpdateMovement
:
class PlayerOneSettings
{
...
public void UpdateMovement()
{
if (IsGameOver) return;
foreach (var key in ValidKeys)
{
var counterpart = KeyCounterpartMapping[key];
if (KeyInput.KeyInputs(key) && Movement != counterpart)
{
Movement = key;
break;
}
}
}
}
With this modification the data and the operation is separated cleanly.
Use inheritance
The PlayerOneSettings
and PlayerTwoSettings
have a huge resembles, which is not a coincidence.
So, it would be wise to define a base class and inherit from it:
abstract class PlayerSettings
{
public int Width { get; set; }
public int Height { get; set; }
public int GameScore { get; set; }
public bool IsGameOver { get; set; }
public Keys Movement { get; protected set; } //Orig: private
protected abstract Keys[] ValidKeys { get; }
protected abstract Dictionary<Keys, Keys> KeyCounterpartMapping { get; }
public void UpdateMovement()
{
if (IsGameOver) return;
foreach (var key in ValidKeys)
{
var counterpart = KeyCounterpartMapping[key];
if (KeyInput.IsKeyDown(key) && Movement != counterpart)
{
Movement = key;
break;
}
}
}
}
- I've changed the
Movement
's setter's accessibility fromprivate
toprotected
to be able to set it in the constructors of the derived classes. - Because fields can't be
virtual
orabstract
that's why I used properties forValidKeys
andKeyCounterpartMapping
. - I used
abstract
to enforce override in the derived classes:
class PlayerOneSettings: PlayerSettings
{
public PlayerOneSettings()
{
Height = 16;
Width = 16;
IsGameOver = false;
Movement = Keys.Down;
GameScore = 0;
}
protected override Keys[] ValidKeys { get; } = new[] { Keys.Right, Keys.Left, Keys.Up, Keys.Down };
protected override Dictionary<Keys, Keys> KeyCounterpartMapping { get; } = new Dictionary<Keys, Keys>
{
{ Keys.Up, Keys.Down },
{ Keys.Down, Keys.Up },
{ Keys.Right, Keys.Left },
{ Keys.Left, Keys.Right },
};
}
class PlayerTwoSettings: PlayerSettings
{
public PlayerTwoSettings()
{
Height = 16;
Width = 16;
IsGameOver = false;
Movement = Keys.S;
GameScore = 0;
}
protected override Keys[] ValidKeys { get; } = new[] { Keys.W, Keys.S, Keys.D, Keys.A };
protected override Dictionary<Keys, Keys> KeyCounterpartMapping { get; } = new Dictionary<Keys, Keys>
{
{ Keys.D, Keys.A },
{ Keys.A, Keys.D },
{ Keys.W, Keys.S },
{ Keys.S, Keys.W },
};
}
With these the derived classes only contains the deviations.
All the common stuff are inherited.
-
\$\begingroup\$ Sorry it was a copy-paste issue. Thanks I fix it. \$\endgroup\$Peter Csala– Peter Csala2021年03月17日 19:06:31 +00:00Commented Mar 17, 2021 at 19:06
-
\$\begingroup\$ @zellez Health should be your number one priority. Everything else could wait. After you are better, feel free to contact me and I'll try to help here. \$\endgroup\$Peter Csala– Peter Csala2021年03月29日 07:52:51 +00:00Commented Mar 29, 2021 at 7:52
-
\$\begingroup\$ @zellez I'm glad that you are better. As I stated in the review I've focused on the
UpdateScreen
method, not on thePlayerMovment
. In my opinion thePlayerMovment
has too much responsibility. If you want to move some part of this into a separate class then you have to remember data and behaviour should stick together. So,SnakeBoddy
and related part of thePlayerMovment
method should reside inside the same class. Food related updates should belong to another separate class. Does it make sense to you? \$\endgroup\$Peter Csala– Peter Csala2021年04月07日 09:18:42 +00:00Commented Apr 7, 2021 at 9:18 -
\$\begingroup\$ The collision detection could belong to the same class as the movement handling, because it operates on the same data. The reaction part could be separated into its on class if it operates on other data as well. \$\endgroup\$Peter Csala– Peter Csala2021年04月07日 09:45:50 +00:00Commented Apr 7, 2021 at 9:45
-
\$\begingroup\$ I know it is shameless, but I can't seem to get it to work. I know it has been a while since you have viewed the code, but if could you show me the proper way it doesn't need to be explained, in detail, or anything like that. I could sit and study the code and try to figure out why on my own. I just lack the basic logic to do it, and if you don't want to help me further, it is understandable, and I wish you a nice day. \$\endgroup\$zellez11– zellez112021年04月12日 10:02:14 +00:00Commented Apr 12, 2021 at 10:02
In this post I'll focus on the PlayerMovment
. Before I try to guide you through the OOP related transformations, let me show you a couple of refactoring techniques which will help you to dramatically reduce the complexity of this method.
Guard expression vs Early exit
Before
private void PlayerMovment()
{
if (choice2.Checked)
{
for (...)
{
...
}
}
}
After
private void PlayerMovment()
{
if (!choice2.Checked) return;
for (...)
{
...
}
}
- Instead of guarding the whole operation, do some preliminary checks and if they fail then early exit
- This will help you streamline your code and reduce the indentation
Splitting logic
Before
for (int i = SnakeBoddy.Count - 1; i >= 0; i--)
{
if (i == 0)
{
//DO a lots of stuff
}
else
{
//DO a simple stuff
}
}
After
//Move each part of the snake body forward (without head)
for (int i = SnakeBoddy.Count - 1; i > 0; i--)
{
//DO a simple stuff
}
//DO a lots of stuff
- In your reverse iteration I changed the exit condition from
i >= 0
toi > 0
- With this modification I could eliminate the branching logic
- So, the
for
loop now has that code which was inside theelse
block- In other words we move each part of the snake body forward (without head)
Don't repeat yourself #1
Before
switch (Player1setting.Movment)
{
case SnakeMovment.Right:
SnakeBoddy[i].X++;
break;
case SnakeMovment.Left:
SnakeBoddy[i].X--;
break;
case SnakeMovment.Up:
SnakeBoddy[i].Y--;
break;
case SnakeMovment.Down:
SnakeBoddy[i].Y++;
break;
}
After
//Adjust head
var head = SnakeBoddy[0];
head.X += Player1setting.Movment == SnakeMovment.Right ? 1
: Player1setting.Movment == SnakeMovment.Left ? -1 : 0;
head.Y += Player1setting.Movment == SnakeMovment.Down ? 1
: Player1setting.Movment == SnakeMovment.Up ? -1 : 0;
- One can argue with this modification that it does not improve legibility
- I can agree with them because using two nested conditional operators (
?:
) might not be easily readable - But logic is quite simple
- Increase
X
with1
if user pressedRight
- Increase
X
with-1
if user pressedLeft
- Increase
X
with0
otherwise
- Increase
- I can agree with them because using two nested conditional operators (
- I've also introduced a variable
head
because you have referred to the same element asSnakeBoddy[0]
and sometimes asSnakeBoddy[i]
wherei
is guaranteed to be 0
Don't repeat yourself #2
Before
for (int J = 1; J < SnakeBoddy.Count; J++)
{
if (SnakeBoddy[i].X == SnakeBoddy[J].X && SnakeBoddy[i].Y == SnakeBoddy[J].Y)
{
Die();
}
}
for (int g = 0; g < SnakeBoddy2.Count; g++)
{
if (SnakeBoddy[0].X == SnakeBoddy2[g].X && SnakeBoddy[0].Y == SnakeBoddy2[g].Y)
{
Player1setting.GameScore += FoodSetting.food2;
label3.Text = Player1setting.GameScore.ToString();
Die();
}
}
After
//Check whether the snake has collided with itself or with the other snake
foreach (var body in SnakeBoddy.Union(SnakeBoddy2))
{
if (head.X == body.X && head.Y == body.Y)
{
Die();
break;
}
}
- Here you can make use of the
foreach
, you don't have to usefor
- You have done the same operation against
SnakeBoddy
andSnakeBoddy2
- With LINQ's
Union
you can easily merge these two iterations
- With LINQ's
- Once you have detected collision then you do not need to iterate through the rest of the items that's why you can
break
out from the loop- Or even
return
from thePlayerMovment
- Or even
- I do believe that
GameScore
andlabel3
updates should not belong here that's why I removed from my code- This should be handled on the caller side
Separate data and operation
Before
if (SnakeFoods1.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
EatFood();
}
if (SnakeFoods2.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
EatFood2();
}
if (SnakeFoods3.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
EatFood3();
}
if (SnakeFoods4.Any(s => s.X == SnakeBoddy[0].X && s.Y == SnakeBoddy[0].Y))
{
EatFood4();
}
After
//Eat all food which was in its way
var allFood = new Dictionary<List<Square>, Action> {
{ SnakeFoods1, EatFood },
{ SnakeFoods2, EatFood2 },
{ SnakeFoods3, EatFood3 },
{ SnakeFoods4, EatFood4 }
};
foreach(var foodsAndEatMapping in allFood)
{
if (foodsAndEatMapping.Key.Any(s => s.X == head.X && s.Y == head.Y))
{
foodsAndEatMapping.Value();
}
}
- Yet again you have performed the same operation against different datasets
- By separating operation from data you can avoid repetition
- Here I've created a mapping (called
allFood
) where I have mapped the food source collection to the relatedEatFood
function- I did not want to spent time to generalize the
EatFood{XYZ}
methods but that should your homework
- I did not want to spent time to generalize the
The refactored PlayerMovment
private void PlayerMovment()
{
if (!choice2.Checked) return;
//Move each part of the snake body forward (without head)
for (int i = SnakeBoddy.Count - 1; i > 0; i--)
{
SnakeBoddy[i].X = SnakeBoddy[i - 1].X;
SnakeBoddy[i].Y = SnakeBoddy[i - 1].Y;
}
//Adjust head
var head = SnakeBoddy[0];
head.X += Player1setting.Movment == SnakeMovment.Right ? 1
: Player1setting.Movment == SnakeMovment.Left ? -1 : 0;
head.Y += Player1setting.Movment == SnakeMovment.Down ? 1
: Player1setting.Movment == SnakeMovment.Up ? -1 : 0;
//Check whether the snake has moved out of the arena
int maxXPosition = pictureBox1.Size.Width / Player1setting.Width;
int maxYPosition = pictureBox1.Size.Height / Player1setting.Height;
if (head.X < 0 || head.X > maxXPosition ||
head.Y < 0 || head.Y >= maxYPosition)
Die();
//Check whether the snake has collided with itself or with the other snake
foreach (var body in SnakeBoddy.Union(SnakeBoddy2))
{
if (head.X == body.X && head.Y == body.Y)
{
Die();
break;
}
}
//Eat all food which was in its way
var allFood = new Dictionary<List<Square>, Action> {
{ SnakeFoods1, EatFood },
{ SnakeFoods2, EatFood2 },
{ SnakeFoods3, EatFood3 },
{ SnakeFoods4, EatFood4}
};
foreach(var foodsAndEatMapping in allFood)
{
if (foodsAndEatMapping.Key.Any(s => s.X == head.X && s.Y == head.Y))
{
foodsAndEatMapping.Value();
}
}
}
- The same transformation should be applied to
PlayerMovment2
Now let's start to focus on the OOP stuff.
Basics
First we have to identify what sort of components do we have.
Then we have to identify what can we do with them.
Now, let's just list the comments here (which I have placed in the refactored version):
//Move each part of the snake body forward (without head)
//Adjust head
//Check whether the snake has moved out of the arena
//Check whether the snake has collided with itself or with the other snake
//Eat all food which was in its way
Nouns
To identify object let's focus on the nouns:
- Snake
- Body
- Head
- Arena
- Food
As you can see our primary object could be the Snake
, which encapsulates Body
and Head
. Depending on the operations Body
and Head
may or may not be public
- If we want to perform operations on
Body
andHead
at same time then most probably we don't need to expose them. They can become implementation details from the consumer point of view. - If we want to allow separate query operations against
Body
andHead
then we should expose only readonly access. In other words the modification methods remain private to the object.
The Arena
is related to pictureBox1
and Player1setting
. If they are not changing over time then the Arena
should not depend on them, just initialize it with the related values. Yet again exposing its members depends on the exposed behaviours. We will talk about this in the verbs section.
And finally the Food
, which is more precisely a set of food. So, here the object could be Foods
or FoodManager
or what so ever.
Verbs
To identify behaviours let's focus on the verbs:
- Snake
- Body
- Move
- Head
- Adjust
- Check collision
- Body
- Arena
- Check boundary
- Food
- Eat
As you can the Move
and Adjust
operates on the snake's body and head. If we want to perform them together then we should not separate them.
public class Snake
{
private List<Square> WholeBody {get; set;}
private Square Head => WholeBody[0];
private IEnumerable<Square> Body => WholeBody.Skip(1);
public Snake(...)
{
//TODO: Initialize snake's internals
}
public void MoveForward(int verticalMove, int horizontalMove)
{
AdjustHead(verticalMove, horizontalMove);
MoveBody();
}
private void MoveBody()
{
//TODO: perform operation against Body
}
private AdjustHead(int verticalMove, int horizontalMove)
{
//TODO: perform operation against Head
}
}
The CheckCollision
can be performed against the Snake
instance itself or against any other instance. So we can create two separate methods to cover both cases:
public Snake
{
...
public bool HaveIBittenMyself()
{
//TODO: Check whether head is collided with any part of the body
}
public bool HasOtherBittenMe(Square headOfOtherSnake)
{
//TODO: Check whether the other's head is collided with any part of the wholeBody
}
}
- As you can see the Snake itself does not know about the rest of the world. It can perform operations on its own data. This is crucial in case of OOP.
I think now you have the flavour so I'll leave Arena
and Foods
as an exercise for yourself. I hope this lengthy post helped you a bit.
Form1
class says you don't. \$\endgroup\$private void UpdateScreen(object sender, EventArgs e)
for an example to how I would do it if I was following OOP standards \$\endgroup\$