patterncsharpModerate
C# OOP Pong game in WinForms
Viewed 0 times
ponggameoopwinforms
Problem
I want to learn C# and have started off with making simple Pong game. I would like to ask you whether the approach I took is correct, as in it is readable and fits the conventions/standards. Maybe some of the implementations I have made (more like for sure) are far from optimal or not complete, probably some things are made in a bit too chaotic or simplistic way, probably there are some more complex (but still simple enough) implementations of certain things that are way more appealing yet do not cost much more work. I really would like to know how to make clean code.
Program.cs
Game.cs
```
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;
namespace Pong
{
class Game
{
public Timer gameTime;
// int gameTimeInterval=1;
public Player player;
public Player player2;
// public AI ai;
public Ball ball;
public Form form;
public GameController controller;
public KeyEventHandler KeyDown { get; private set; }
public Game(Form form)
{
this.form = form;
controller = new GameController(form);
gameTime = new Timer();
gameTime.Enabled = true;
gameTime.Interval = 1;
gameTime.Tick += new EventHandler(OnGameTi
Program.cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Drawing;
namespace Pong
{
static class Program
{
///
/// The main entry point for the application.
///
[STAThread]
static void Main()
{
Application.EnableVisualStyles();
Application.SetCompatibleTextRenderingDefault(false);
GameArea game = new GameArea();
Application.Run(game);
}
}
}Game.cs
```
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;
namespace Pong
{
class Game
{
public Timer gameTime;
// int gameTimeInterval=1;
public Player player;
public Player player2;
// public AI ai;
public Ball ball;
public Form form;
public GameController controller;
public KeyEventHandler KeyDown { get; private set; }
public Game(Form form)
{
this.form = form;
controller = new GameController(form);
gameTime = new Timer();
gameTime.Enabled = true;
gameTime.Interval = 1;
gameTime.Tick += new EventHandler(OnGameTi
Solution
First thing's first:
Don't use public fields. (What is a field? It's a member of a class/struct that looks like a regular variable.)
These are all fields. In C# we don't make fields public, fields don't support events on
Etc.
Next, we don't use
Next, we should consider whether or not the
Always, always, always use braces. Not using braces can definitely throw unexpected behaviour into our programme, and it doesn't really cost anything to add them:
Don't name parameters, variables or members
In this case,
Generally, it's best practice to explicitly state access modifiers. I.e.:
Those are both
First: in C#,
Second: we need to add our explicit access modifier.
One note I will make on
A little design note: if you find yourself giving the same variables, properties, parameters, fields, etc the same name prefix (as below), it's probably time for a new class:
We should probably make an object (in this case, it makes more sense to be a
And use that class for our speed:
The difference between
You asked specifically about
This has a couple possible updates that would make life easier.
First, let's use
Next, we're doing the same thing for both
This makes the code a bit more readable, and now if you modify where the
Personally, I would add a
Don't use public fields. (What is a field? It's a member of a class/struct that looks like a regular variable.)
public Timer gameTime;
// int gameTimeInterval=1;
public Player player;
public Player player2;
// public AI ai;
public Ball ball;
public Form form;
public GameController controller;These are all fields. In C# we don't make fields public, fields don't support events on
set, get, etc. They don't support any sort of access control, and just allow anyone to read or write that value. This is almost always very bad. Instead, make them properties. It's really simple and easy to do:public Timer gameTime { get; set; }
public Player player { get; set; }
public Player player2 { get; set; }Etc.
Next, we don't use
camelCase on any public members in C#, instead we use PascalCase for the public and protected members.public Timer GameTime { get; set; }Next, we should consider whether or not the
GameTime property should be set from outside the class, in our case, it doesn't have such a need, so we're going to go to the next step of making the set method on the property private:public Timer GameTime { get; private set; }Always, always, always use braces. Not using braces can definitely throw unexpected behaviour into our programme, and it doesn't really cost anything to add them:
public void PaddleCollision(Player player, Player player2, Ball ball)
{
if (ball.Bounds.IntersectsWith(player.Bounds))
{
ball.ballSpeedX = -ball.ballSpeedX;
}
else if (ball.Bounds.IntersectsWith(player2.Bounds))
{
ball.ballSpeedX = -ball.ballSpeedX;
}
}Don't name parameters, variables or members
obj unless they represent a generic object.public void CollisionGameArea(Ball obj)In this case,
obj is always a Ball object, so it should, at the very least, be named ball.public void CollisionGameArea(Ball ball)Generally, it's best practice to explicitly state access modifiers. I.e.:
const int GameAreaWidth = 1248;
const int GameAreaHeight = 720;Those are both
private by default, so we need to change two things here:First: in C#,
private members are camelCase;Second: we need to add our explicit access modifier.
private const int gameAreaWidth = 1248;
private const int gameAreaHeight = 720;One note I will make on
private member naming: some developers (myself included) use the underscore _camelCase notation instead of regular camelCase on private members as this helps distinguish them from parameters in methods (which are also camelCase style).A little design note: if you find yourself giving the same variables, properties, parameters, fields, etc the same name prefix (as below), it's probably time for a new class:
public int ballSpeedX{set;get;}
public int ballSpeedY{set;get;}We should probably make an object (in this case, it makes more sense to be a
struct) for it:public struct BallSpeed
{
public int X { get; set; }
public int Y { get; set; }
}And use that class for our speed:
public BallSpeed BallSpeed { get; set; }The difference between
class and struct is beyond the scope of this answer, but it changes how C# will likely treat the object and any implementations of it.You asked specifically about
OnKeyDown in the form class:public void OnKeyDown(object sender, KeyEventArgs e)
{
int y = game.player2.Location.Y;
int playerSpeed = 25;
if (e.KeyCode == Keys.Up && y - 25 >= 0)
{
game.player2.Location = new Point(this.Width - (game.player2.Width*2), y - playerSpeed);
}
else if (e.KeyCode == Keys.Down && y + playerSpeed <= (this.Height - game.player2.Height*2))
{
game.player2.Location = new Point(this.Width - (game.player2.Width*2), y + playerSpeed);
}
}This has a couple possible updates that would make life easier.
First, let's use
playerSpeed for the first if like we did the second.if (e.KeyCode == Keys.Up && y - playerSpeed >= 0)
{
game.player2.Location = new Point(this.Width - (game.player2.Width*2), y - playerSpeed);
}Next, we're doing the same thing for both
if blocks, so we can abstract that pretty easily:public void OnKeyDown(object sender, KeyEventArgs e)
{
int y = game.player2.Location.Y;
int playerSpeed = 25;
int direction = 0;
if (e.KeyCode == Keys.Up && y - playerSpeed >= 0)
{
direction = -1;
}
else if (e.KeyCode == Keys.Down && y + playerSpeed <= (this.Height - game.player2.Height * 2))
{
direction = 1;
}
game.player2.Location = new Point(this.Width - (game.player2.Width * 2), y + (playerSpeed * direction));
}This makes the code a bit more readable, and now if you modify where the
Point location is (say you add 3px buffer) you don't have to modify both Point lines.Personally, I would add a
Move(float distance) method toCode Snippets
public Timer gameTime;
// int gameTimeInterval=1;
public Player player;
public Player player2;
// public AI ai;
public Ball ball;
public Form form;
public GameController controller;public Timer gameTime { get; set; }
public Player player { get; set; }
public Player player2 { get; set; }public Timer GameTime { get; set; }public Timer GameTime { get; private set; }public void PaddleCollision(Player player, Player player2, Ball ball)
{
if (ball.Bounds.IntersectsWith(player.Bounds))
{
ball.ballSpeedX = -ball.ballSpeedX;
}
else if (ball.Bounds.IntersectsWith(player2.Bounds))
{
ball.ballSpeedX = -ball.ballSpeedX;
}
}Context
StackExchange Code Review Q#145046, answer score: 11
Revisions (0)
No revisions yet.