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

Small C# Pong game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewcsharpstackoverflowwinformsbeginnerc#game
gamesmallpong

Problem

I recently made a 140-lined Pong game in C# using forms.

Since I'm very new to programming, I'd like if somebody would please read it and give me advice on where to improve, what you believe I did wrong and most of all, what should have been done better and how.

Plus, would it be somewhat useful to implement delegates somewhere in the code above ? I'm still studying them but found no useful way to implement those yet.

Finally, I found no way to use the Dispose() method on the created graphics in the Pong Class without breaking the code. Is it really that useful ? Isn't the garbage collector supposed to do so anyway?

```
namespace WindowsFormsApplication2
{
public class PongGame
{
public static Rectangle paddle1 = new Rectangle(14, 180, 20, 100);
public static Rectangle paddle2 = new Rectangle(566, 180, 20, 100);
public static Rectangle ball = new Rectangle(290, 115, 16, 16);
static Font Drawfont = new Font("Arial", 40);
public static bool p1movesUp, p1movesDown, p2movesUp, p2movesDown;
public static SolidBrush sb = new SolidBrush(Color.White);
public static double p1p; //Double that will store player 1 score
public static double p2p; //Double that will store player 2 score
static int RandoMin = 1; //Those 2 random integers are used to randomize ball directions
static int RandoMax = 3; //in the Randomize() method to avoid repetition of ball movement
public static double Xspeed = -1; //Beginning Initial speed
public static double Yspeed = 1;

public static void DrawIt(Graphics Draw)
{ //Draws both paddles and ball
Draw.Clear(Color.Black);
Draw.FillRectangle(sb, paddle2);
Draw.FillRectangle(sb, paddle1);
Draw.FillRectangle(sb, ball);
//Draw Score

Solution

Here are a few observations:

-
In Form1_KeyDown and Form1_KeyUp, the logic would be cleaner organized into a switch statement:

private void Form1_KeyDown(object sender, KeyEventArgs e)
{
    switch (e.KeyData)
    {
        case Keys.S:
            PongGame.p1movesDown = true;
            return;
        case Keys.W:
            PongGame.p1movesUp = true;
            return;
        case Keys.L:
            PongGame.p2movesDown = true;
            return;
        case Keys.P:
            PongGame.p2movesUp = true;
            return;
    }
}

private void Form1_KeyUp(object sender, KeyEventArgs e)
{
    switch (e.KeyData)
    {
        case Keys.S:
            PongGame.p1movesDown = false;
            return;
        case Keys.W:
            PongGame.p1movesUp = false;
            return;
        case Keys.L:
            PongGame.p2movesDown = false;
            return;
        case Keys.P:
            PongGame.p2movesUp = false;
            return;
    }
}


-
In CheckIfMoving, a variable z is declared just to use a bizarre version of the ternary operator (?:). This should be rewritten to do what you actually want rather than trying to be clever:

public static void CheckIfMoving()
{
    // If player press the key to move the paddle, this method
    // changes the Y position of the paddle Accordingly
    if (p1movesUp)
    {
        if (paddle1.Y = 381)
        {
            paddle1.Y = 381;
        }
        else
        {
            paddle1.Y += 3;
        }
    }

    if (p2movesUp)
    {
        if (paddle2.Y = 381)
        {
            paddle2.Y = 381;
        }
        else
        {
            paddle2.Y += 3;
        }
    }
}


-
"I found no way to use the Dispose() method on the created graphics in the Pong Class without breaking the code. Is it really that useful? Isn't the garbage collector supposed to do so anyway?"

This is comparing apples and oranges, but don't feel bad! Many people mix up or conflate Dispose() with the garbage collector. Sometimes they're intertwined, many times they are not. The very simple rule of thumb is - if a class implements IDisposable, call Dispose() at some point. Period. Deep down in the bowels of that class (or one that it has as a member), a non-.NET unmanaged resource is being used. Dispose() ensures that resource is returned in a deterministic fashion. I highly suggest reading and understanding this answer.

So, how do you fix this? Simple. The other partial part of your Form1 class has a Dispose() method. It usually looks something like this:

/// 
/// Clean up any resources being used.
/// 
/// true if managed resources should be disposed; otherwise, false.
protected override void Dispose(bool disposing)
{
    if (disposing && (components != null))
    {
        components.Dispose();
    }
    base.Dispose(disposing);
}


Add the following:

/// 
/// Clean up any resources being used.
/// 
/// true if managed resources should be disposed; otherwise, false.
protected override void Dispose(bool disposing)
{
    this.Draw.Dispose();
    if (disposing && (components != null))
    {
        components.Dispose();
    }
    base.Dispose(disposing);
}


However, that's sorta cheating. You're holding the Graphics object for the lifetime of your application. A better way would be removing Draw as a class member of Form1 and using it just around the piece that draws (timer1_Tick). Note that using will call Dispose() at the end of the scope automatically for you:

private void timer1_Tick(object sender, EventArgs e)
{
    using (Graphics Draw = this.pictureBox1.CreateGraphics())
    {
        PongGame.DrawIt(Draw);      // Draws paddles & ball
    }

    PongGame.MoveBall(this.timer1); // Moves the ball
    PongGame.CheckScore();          // Check if one player scored
    PongGame.CheckIfMoving();       // Method that check if player is moving up or down the paddle
}


-
Don't continuously redeclare the random number generator in the Randomize method:

Random r = new Random();


instead, do it once as a class-level member:

private static readonly Random r = new Random();


This will prevent possible duplicate randoms in a short time span.

-
Do all those members of PongGame really need to be public? I'm thinking not. The only members accessed outside that class are p1movesUp, p1movesDown, p2movesUp, and p2movesDown. Even then, I'd make all of them private and instead make those four auto-implemented properties to aid with encapsulation:

public static bool p1movesUp { get; set; }
public static bool p1movesDown { get; set; }
public static bool p2movesUp { get; set; }
public static bool p2movesDown { get; set; }


-
Since everything in the PongGame class is static, declare the class statically as well: public static class PongGame. I have some future thoughts about making everything not-so-static, implementing an interface and injecting dep

Code Snippets

private void Form1_KeyDown(object sender, KeyEventArgs e)
{
    switch (e.KeyData)
    {
        case Keys.S:
            PongGame.p1movesDown = true;
            return;
        case Keys.W:
            PongGame.p1movesUp = true;
            return;
        case Keys.L:
            PongGame.p2movesDown = true;
            return;
        case Keys.P:
            PongGame.p2movesUp = true;
            return;
    }
}

private void Form1_KeyUp(object sender, KeyEventArgs e)
{
    switch (e.KeyData)
    {
        case Keys.S:
            PongGame.p1movesDown = false;
            return;
        case Keys.W:
            PongGame.p1movesUp = false;
            return;
        case Keys.L:
            PongGame.p2movesDown = false;
            return;
        case Keys.P:
            PongGame.p2movesUp = false;
            return;
    }
}
public static void CheckIfMoving()
{
    // If player press the key to move the paddle, this method
    // changes the Y position of the paddle Accordingly
    if (p1movesUp)
    {
        if (paddle1.Y <= 0)
        {
            paddle1.Y = 0;
        }
        else
        {
            paddle1.Y -= 3;
        }
    }

    if (p1movesDown)
    {
        if (paddle1.Y >= 381)
        {
            paddle1.Y = 381;
        }
        else
        {
            paddle1.Y += 3;
        }
    }

    if (p2movesUp)
    {
        if (paddle2.Y <= 0)
        {
            paddle2.Y = 0;
        }
        else
        {
            paddle2.Y -= 3;
        }
    }

    if (p2movesDown)
    {
        if (paddle2.Y >= 381)
        {
            paddle2.Y = 381;
        }
        else
        {
            paddle2.Y += 3;
        }
    }
}
/// <summary>
/// Clean up any resources being used.
/// </summary>
/// <param name="disposing">true if managed resources should be disposed; otherwise, false.</param>
protected override void Dispose(bool disposing)
{
    if (disposing && (components != null))
    {
        components.Dispose();
    }
    base.Dispose(disposing);
}
/// <summary>
/// Clean up any resources being used.
/// </summary>
/// <param name="disposing">true if managed resources should be disposed; otherwise, false.</param>
protected override void Dispose(bool disposing)
{
    this.Draw.Dispose();
    if (disposing && (components != null))
    {
        components.Dispose();
    }
    base.Dispose(disposing);
}
private void timer1_Tick(object sender, EventArgs e)
{
    using (Graphics Draw = this.pictureBox1.CreateGraphics())
    {
        PongGame.DrawIt(Draw);      // Draws paddles & ball
    }

    PongGame.MoveBall(this.timer1); // Moves the ball
    PongGame.CheckScore();          // Check if one player scored
    PongGame.CheckIfMoving();       // Method that check if player is moving up or down the paddle
}

Context

StackExchange Code Review Q#45011, answer score: 12

Revisions (0)

No revisions yet.