12
\$\begingroup\$

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
            Draw.DrawString(p1p.ToString(), Drawfont, sb, 180, 10);
            Draw.DrawString(p2p.ToString(), Drawfont, sb, 380, 10);
        }

        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 == true)
            { int z = paddle1.Y <= 0 ? paddle1.Y = 0 : paddle1.Y -= 3; }
            if (p1movesDown == true)
            { int z = paddle1.Y >= 381 ? paddle1.Y = 381 : paddle1.Y += 3; }
            if (p2movesUp == true)
            { int z = paddle2.Y <= 0 ? paddle2.Y = 0 : paddle2.Y -= 3; }
            if (p2movesDown == true)
            { int z = paddle2.Y >= 381 ? paddle2.Y = 381 : paddle2.Y += 3; }
        }

        public static void Restart()                            //Method called upon player scoring, to reset speed values
        {                                                       //and ball position
            ball.X = 290;   Yspeed = 1;
            ball.Y = 115;   RandoMin = 1;
            Xspeed = -1;    RandoMax = 3;
        }

        public static void CheckScore()                         //Check if any player has scored, and increase p1p accordingly
        {
            if (ball.X < 1)
            { p2p += 1; Restart(); }
            else if (ball.X > 579)
            { p1p += 1; Restart(); }
        }

        public static void IncreaseSpeed()                      //Increase both the normal speed and the results of
        {                                                       //any possible randomization in the Randomize() method
            RandoMin += 1;
            RandoMax += 1;
            Xspeed = Xspeed < 0 ? Xspeed -= 1 : Xspeed += 1;
        }

        public static void MoveBall(Timer t1)
        {
            ball.X += (int)Xspeed;                                     //Changes ball coordinates based on speed in both x & y axis
            ball.Y += (int)Yspeed;
            if (ball.Y > 465 || ball.Y < 0) { Yspeed = -Yspeed; }     //If ball touch one of the Y bounds, it's y speed gets a change in sign, and ball rebounce
            if (ball.X > 579 || ball.X < 1) { Xspeed = -Xspeed; }     //Same for X bounds, with x speed
            if (ball.IntersectsWith(paddle1) || ball.IntersectsWith(paddle2))
            {
                int dst = paddle1.Y + 100;
                int Distance = dst - ball.Y;
                if (Distance > 75 || Distance < 25) { Randomize(); }  //If the ball intersects the paddle "away" from the centre, the ball movement get randomized
                else { Xspeed = -Xspeed; }                             //else, it's speed on the X axis gets simply reverted
            }
        }

        static void Randomize()
        {
            Random r = new Random();

            double s = r.Next(RandoMin, RandoMax);                     //Uses RandoMin & RandoMax values to randomize the X speed of the ball
            Xspeed = ball.IntersectsWith(paddle1) ? Xspeed = s : Xspeed = -s;

            if (Yspeed < 0)                                            //If ball is moving upward, (so y speed is negative) the random value assigned
            {                                                          //will be changed in sign, so the ball can still go upward
                double t = r.Next(RandoMin, RandoMax);
                Yspeed = -t;
            }
            else                                                       //Else, directly change the Y speed to a positive value
            { Yspeed = r.Next(RandoMin, RandoMax); }
        }                                                              //End of PongGame Class
    }

    public partial class Form1 : Form
    {
        Graphics Draw;
        SolidBrush sb = new SolidBrush(Color.White);
        public Form1()
        {
            InitializeComponent();
            pictureBox1.BackColor = Color.Black;
            Draw = pictureBox1.CreateGraphics();

            timer1.Interval = 10;
            timer1.Start();
            timer2.Start();
        }


        private void timer1_Tick(object sender, EventArgs e)
        {  
            PongGame.DrawIt(Draw);                      //Draws paddles & ball
            PongGame.MoveBall(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
        }

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

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

        private void timer2_Tick(object sender, EventArgs e)
        { PongGame.IncreaseSpeed(); }     //Every 3 seconds, this timer will increase Overall speed
    }
}
\$\endgroup\$
11
\$\begingroup\$

Here are a few observations:

  1. 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;
        }
    }
    
  2. 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 <= 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;
            }
        }
    }
    
  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:

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

    Add the following:

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

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

  5. 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; }
    
  6. 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 dependencies, but let's keep it simple for now.

  7. I'm a little confused as to the use of doubles in the PongGame class. They only contain whole numbers. Do you expect a score to go over two billion? If not, make them all int. Better performance and intent.

Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ Hi, and thanks for the very detailed answer : ) I got only a question regarding the using statement to call dispose() at the end of it, this way, the program needs to recreate 100 times a seconds the graphics instance... should this be avoided ? And how, possibly ? \$\endgroup\$ – Rphysx Mar 22 '14 at 2:40
  • \$\begingroup\$ @Rphysx I would suggest running it in both configurations under a profiler and noting the differences in time, memory and GC pressure. \$\endgroup\$ – Jesse C. Slicer Mar 24 '14 at 18:19

protected by Jamal Jul 28 '17 at 0:34

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.