HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

C# OOP Pong game in WinForms

Submitted by: @import:stackexchange-codereview··
0
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

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

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 to

Code 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.