Advertisement Jump to content
Sign in to follow this  
Devlyn

First Game Code Review - Pong

This topic is 1761 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hi All

Read this article: Your First Step to Game Development Starts Here.  And I think is a good starting point for some who does not have any game development experience.

 

So I made my first C# WinForm Pong game.  As stated in the above article, I can post my code here for review.  So I asking for criticisms, what could I do better? What did I do right?  Are there alternatives?

 

Here is my full code:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Media;

/*
 * Source:
 *  http://csharptactics.blogspot.com/2011/02/basic-pong-game.html
 */

namespace Pong
{
    public partial class gameArea : Form
    {
        PictureBox picBoxPlayer, picBoxAI, picBoxBall;
        Timer gameTime; // also the game loop

        const int SCREEN_WIDTH = 800;
        const int SCREEN_HEIGHT = 600;

        Size sizePlayer = new Size(25, 100);
        Size sizeAI = new Size(25, 100);
        Size sizeBall = new Size(20, 20);

        const int gameTimeInterval = 1;

        const int ballStartSpeed = 2;
        const int ballIncreaseSpeedRate = 1;
        const int ballSpeedLimited = 15;

        const int aiOffSetLoops = 15;

        int ballSpeedX = ballStartSpeed;
        int ballSpeedY = ballStartSpeed;


        Random rad;
        int aiOffSet;
        int aiOffSetCounter;

        Dictionary<string, SoundPlayer> sounds;

        public gameArea()
        {
            InitializeComponent();

            this.DoubleBuffered = true;

            picBoxPlayer = new PictureBox();
            picBoxAI = new PictureBox();
            picBoxBall = new PictureBox();

            gameTime = new Timer();
            gameTime.Interval = gameTimeInterval;

            gameTime.Tick += new EventHandler(gameTime_Tick);

            this.Width = SCREEN_WIDTH;
            this.Height = SCREEN_HEIGHT;
            this.StartPosition = FormStartPosition.CenterScreen;
            this.BackColor = Color.Black;

            picBoxPlayer.Size = sizePlayer;
            picBoxPlayer.Location = new Point(picBoxPlayer.Width / 2, ClientSize.Height / 2 - picBoxPlayer.Height / 2);
            picBoxPlayer.BackColor = Color.Blue;
            this.Controls.Add(picBoxPlayer);

            picBoxAI.Size = sizeAI;
            picBoxAI.Location = new Point(ClientSize.Width - (picBoxAI.Width + picBoxAI.Width / 2), ClientSize.Height / 2 - picBoxPlayer.Height / 2); // TODO: why picBoxPlayer and not picBoxAI?
            picBoxAI.BackColor = Color.Red;
            this.Controls.Add(picBoxAI);

            rad = new Random();
            aiOffSet = 0;
            aiOffSetCounter = 1;

            picBoxBall.Size = sizeBall;
            picBoxBall.Location = new Point(ClientSize.Width / 2 - picBoxBall.Width / 2, ClientSize.Height / 2 - picBoxBall.Height / 2);
            picBoxBall.BackColor = Color.Green;
            this.Controls.Add(picBoxBall);

            // Load Sounds
            sounds = new Dictionary<string, SoundPlayer>();
            for (int k = 1; k <= 10; k++)
            {
                sounds.Add(String.Format(@"pong{0}", k), new SoundPlayer(String.Format(@"pong{0}.wav", k)));
            }

            // Start Game loop
            gameTime.Enabled = true;
        }

        void gameTime_Tick(object sender, EventArgs e)
        {
            picBoxBall.Location = new Point(picBoxBall.Location.X + ballSpeedX, picBoxBall.Location.Y + ballSpeedY);
            gameAreaCollosions();
            padlleCollision();
            playerMovement();
            aiMovement();
        }

        private void iaChangeOffSet()
        {
            if (aiOffSetCounter >= aiOffSetLoops)
            {
                aiOffSet = rad.Next(1, picBoxAI.Height + picBoxBall.Height);
                aiOffSetCounter = 1;
            }
            else
            {
                aiOffSetCounter++;
            }
        }

        private void gameAreaCollosions()
        {
            if (picBoxBall.Location.Y > ClientSize.Height - picBoxBall.Height || picBoxBall.Location.Y < 0)
            {
                iaChangeOffSet();
                ballSpeedY = -ballSpeedY;
                sideCollision();
            }
            else if (picBoxBall.Location.X > ClientSize.Width)
            {
                padlleSideCollision();
                resetBall();
            } 
            else if (picBoxBall.Location.X < 0)
            {
                padlleSideCollision();
                resetBall();
            }
        }
        private void resetBall() 
        {
            if (ballSpeedX > 0)
                ballSpeedX = -ballStartSpeed;
            else
                ballSpeedX = ballStartSpeed;
            if (ballSpeedY > 0)
                ballSpeedY = -ballStartSpeed;
            else
                ballSpeedY = ballStartSpeed;

            aiOffSet = 0;
            picBoxBall.Location = new Point(ClientSize.Width / 2 - picBoxBall.Width / 2, ClientSize.Height / 2 - picBoxBall.Height / 2);
        }
        private void playerMovement()
        {
            if (this.PointToClient(MousePosition).Y >= picBoxPlayer.Height / 2 && this.PointToClient(MousePosition).Y <= ClientSize.Height - picBoxPlayer.Height / 2)
            {
                int playerX = picBoxPlayer.Width / 2;
                int playerY = this.PointToClient(MousePosition).Y - picBoxPlayer.Height / 2;

                picBoxPlayer.Location = new Point(playerX, playerY);
            }
        }
        private void aiMovement()
        {
            int aiX = ClientSize.Width - (picBoxAI.Width + picBoxAI.Width / 2);
            int aiY = (picBoxBall.Location.Y - picBoxAI.Height / 2) + aiOffSet;

            if (aiY < 0)
                aiY = 0;
            if (aiY > ClientSize.Height - picBoxAI.Height)
                aiY = ClientSize.Height - picBoxAI.Height;

            picBoxAI.Location = new Point(aiX, aiY);
        }
        private void padlleCollision()
        {
            if (picBoxBall.Bounds.IntersectsWith(picBoxAI.Bounds))
            {
                picBoxBall.Location = new Point(picBoxAI.Location.X - picBoxBall.Width, picBoxBall.Location.Y);
                ballSpeedX = -ballSpeedX;
                aiCollision();
            }
            if (picBoxBall.Bounds.IntersectsWith(picBoxPlayer.Bounds))
            {
                picBoxBall.Location = new Point(picBoxPlayer.Location.X + picBoxPlayer.Width, picBoxBall.Location.Y);
                ballSpeedX = -ballSpeedX;
                playerCollision();
            }
        }
        private void playerCollision() 
        {
            sounds["pong1"].Play();
            SlowDownBall();
        }
        private void aiCollision()
        {
            sounds["pong2"].Play();
            SlowDownBall();
        }
        private void sideCollision()
        {
            sounds["pong3"].Play();

            SpeedUpBall();
        }
        private void padlleSideCollision()
        {
            sounds["pong9"].Play();
        }
        private void SpeedUpBall()
        {
            if (ballSpeedY > 0)
            {
                ballSpeedY += ballIncreaseSpeedRate;
                if (ballSpeedY >= ballSpeedLimited)
                    ballSpeedY = ballSpeedLimited;
            }
            else
            {
                ballSpeedY -= ballIncreaseSpeedRate;
                if (ballSpeedY <= -ballSpeedLimited)
                    ballSpeedY = -ballSpeedLimited;
            }

            if (ballSpeedX > 0)
            {
                ballSpeedX += ballIncreaseSpeedRate;
                if (ballSpeedX >= ballSpeedLimited)
                    ballSpeedX = ballSpeedLimited;
            }
            else
            {
                ballSpeedX -= ballIncreaseSpeedRate;
                if (ballSpeedX <= -ballSpeedLimited)
                    ballSpeedX = -ballSpeedLimited;
            }
        }
        private void SlowDownBall()
        {
            if (ballSpeedY > 0)
            {
                ballSpeedY -= ballIncreaseSpeedRate;
                if (ballSpeedY <= ballStartSpeed)
                    ballSpeedY = ballStartSpeed;
            }
            else
            {
                ballSpeedY += ballIncreaseSpeedRate;
                if (ballSpeedY >= -ballStartSpeed)
                    ballSpeedY = -ballStartSpeed;
            }

            if (ballSpeedX > 0)
            {
                ballSpeedX -= ballIncreaseSpeedRate;
                if (ballSpeedX <= ballStartSpeed)
                    ballSpeedX = ballStartSpeed;
            }
            else
            {
                ballSpeedX += ballIncreaseSpeedRate;
                if (ballSpeedX >= -ballStartSpeed)
                    ballSpeedX = -ballStartSpeed;
            }            
        }
    }
}

I used this Basic Pong Game tutorial for the based of the game, and then added sound, an AI the can lose/win, and the ball speeds up when it hits the top and bottom sides, and slows down when it hits the paddles.

Edited by Devlyn

Share this post


Link to post
Share on other sites
Advertisement
Looks pretty readable. Good job. Guess what I don't quite like:

1.) Try and see what it looks like when the consts get their own declaration block and the other member variables follow that block.

2.) Why are some of the consts in uppercase and others are not?

3.) The name gameArea is used in a weird way. I don't think it is chosen well. A game area does not manage a timer ... probably game would be better.

4.) There is a lot going on in the contructor. Guess it might be that I don't know enough about C#, but I suppose a lot of the logic should go somewhere else. I don't know where gameTime_Tick gets called, but wouldn't it be possible to constuct the object there and call an initialize method that contains a lot of the constructor code?

5.) I have a feeling that SpeedUpBall and SlowDownBall violate the DRY (don't repeat yourself) principle. Have not thought it through, but you can try to find a way to extract another convenience method that those two methods can use, e.g. with an increaseSpeed boolean parameter. Edited by DareDeveloper

Share this post


Link to post
Share on other sites

Thank DareDeveloper for your feedback.  One of the reasons why I am doing game development is to learn, and one thing I would like to understand more of, is good practices.

 


1.) Try and see what it looks like when the consts get their own declaration block and the other member variables follow that block.

 

I assume this is a good practice?

 


2.) Why are some of the consts in uppercase and others are not?

 

Well I first did the tutorial, and they only put SCREEN_WIDTH and SCREEN_HEIGHT in uppercase, and the rest were not.  What is the good practice when dealing with consts?

 


3.) The name gameArea is used in a weird way. I don't think it is chosen well. A game area does not manage a timer ... probably game would be better.

 

This is what the tutorial did it, but I do agree with you on this onewink.png

 


4.) There is a lot going on in the contructor. Guess it might be that I don't know enough about C#, but I suppose a lot of the logic should go somewhere else. I don't know where gameTime_Tick gets called, but wouldn't it be possible to constuct the object there and call an initialize method that contains a lot of the constructor code?

 

Its in my understanding (please correct me if I am wrong) that you do all your initializing code in the constructor.  This is just setting up the objects.  As for the gameTime_Tick, this I believe you would consider to be the main game loop.  If you look in the constructor, you will see:

gameTime.Tick += new EventHandler(gameTime_Tick);

In C# this is how you assign an event, in this case I am assigning an method 'gameTime_Tick' to the tick event of the gameTime.  Which will call every 1 millisecond.

But if you have a suggestion how to neaten this up. I am only happy to hear it.

 


5.) I have a feeling that SpeedUpBall and SlowDownBall violate the DRY (don't repeat yourself) principle. Have not thought it through, but you can try to find a way to extract another convenience method that those two methods can use, e.g. with an increaseSpeed boolean parameter.

So you are suggesting to combine SpeedUpBall and SlowDownBall into one method, that by the input parameter can increase the ball's speed or slow it down? I think that is possible. 

 

Well thanks again for your input, it been much appreciated!

 

Enjoy

Devlyn

Edited by Devlyn

Share this post


Link to post
Share on other sites

Nice, readable code!

 

I assume this is a good practice?

Keeping related member variables and functions together is usually easier to read. Usually people do it different ways but having, for example, the public interface on top and the private functions at the end seems logical, just like grouping together constants.

 

 

Well I first did the tutorial, and they only put SCREEN_WIDTH and SCREEN_HEIGHT in uppercase, and the rest were not.  What is the good practice when dealing with consts?

There is not really a rule for this, but at least it should be consistent, just like most things. Many people will already assume your SCREEN_WIDTH and SCREEN_HEIGHT variables are constant because they're uppercase, and they could assume the lower case variables aren't constants.

 

Your code is already pretty readable, but try more consistent naming. Most of the times, you use camelCase, but sometimes you throw in an underscore. wink.png

 

Also, I think that the constructor is indeed having a lot of code. I don't think there is anything wrong with initalizing everything inside the constructor, but only from the class itself. You're also initalizing a lot of other objects and systems. For example, you could group your sound code into a sound class. While it may not look really useful now, grouping related functionality into classes will make your code a lot more readable and easier to maintain.

 

It's definitely a very good start! (Much better than mine... unsure.png ) Also welcome on the forums!

Edited by ProtectedMode

Share this post


Link to post
Share on other sites

Great to hear I am doing well for myself, thanks ProtectedMode for you feedback, and welcoming me to the forum.  I do tend to wonder with my naming convention thounsure.png

Share this post


Link to post
Share on other sites

I assume this is a good practice?


Well I guess C# might have a standard way to do things and unfortunately I am only familiar with the Java (and a little with the C++) way.
It is save to say that being consistent is a good aproach, though ;-).
Same goes for Method and Class names. It should be possible to tell which is which depending on if the first letter is uppercase or not.
I prefer classes with uppercase letters, methods with lowercase. I would look at other C# projects though to see how most people do it.
Often it is up to teams or individuals to decide on what coding guidelines they want to commit to.
 

Well I first did the tutorial, and they only put SCREEN_WIDTH and SCREEN_HEIGHT in uppercase, and the rest were not. What is the good practice when dealing with consts?


I like making them distinguishable by making them all-uppercase. Again, I don't know if that is considered weird in the C# world.
 

This is what the tutorial did it, but I do agree with you on this one:wink:

:-)
 

Its in my understanding (please correct me if I am wrong) that you do all your initializing code in the constructor. This is just setting up the objects.


I guess that assumes that the classes are designed well and serve a single purpose. There should not be a lot of variables that need to be initialized and no logic if at all possible.

It is fine for this kind of project to not apply clean code principles and create a ton of classes, but I would keep the constructor as plain as possible and maybe even write several initialize methods that contain the initialization of certain logical parts of the game.
 

As for the gameTime_Tick, this I believe you would consider to be the main game loop. If you look in the constructor, you will see:

gameTime.Tick += new EventHandler(gameTime_Tick);
In C# this is how you assign an event, in this case I am assigning an method 'gameTime_Tick' to the tick event of the gameTime. Which will call every 1 millisecond.
But if you have a suggestion how to neaten this up. I am only happy to hear it.

 


Ah okay, but it is still the object creation that triggers the activation of the game.
What triggers the creation of the gameArea object? Is there a setting in the IDE that defines the class as an entry point?
Because it inherits from Form, I suppose that the context creates the object. Not sure if there is a way to call initialization and activation methods another way.

 

Guess somebody else has to step in here. My guts tell me that the initialization and activation should not be called by the constructor, but my Java background might cloud my judgement.
 

So you are suggesting to combine SpeedUpBall and SlowDownBall into one method, that by the input parameter can increase the ball's speed or slow it down? I think that is possible.


Not necessarily. Calling methods named SlowDownBall and SpeedUpBall seems pretty readable and sensible.
Calling a ChangeSpeed method instead might make it harder to read.
The implementation of those two methods could probably use another method like ChangeSpeed to be more compact.
 

Well thanks again for your input, it been much appreciated!

Enjoy
Devlyn

You are very welcome :-)

Edited by DareDeveloper

Share this post


Link to post
Share on other sites


Devlyn, on 21 Mar 2014 - 8:20 PM, said:
As for the gameTime_Tick, this I believe you would consider to be the main game loop. If you look in the constructor, you will see:
gameTime.Tick += new EventHandler(gameTime_Tick);
In C# this is how you assign an event, in this case I am assigning an method 'gameTime_Tick' to the tick event of the gameTime. Which will call every 1 millisecond.
But if you have a suggestion how to neaten this up. I am only happy to hear it.
 

Ah okay, but it is still the object creation that triggers the activation of the game.
What triggers the creation of the gameArea object? Is there a setting in the IDE that defines the class as an entry point?
I suppose because the context of the form creates the object.

 

Just so that you know; the creation of the gameArea is in other entry level class call Program, which has the static main method. (standard C# stuff)  It is generated by the IDE.wink.png

Share this post


Link to post
Share on other sites

Nice, that is pretty Java-esque. You can probably create an object of Program in the static method, no!?

 

If you want to can think about if it would make sense to move some code over to the Program class.

You might even be able to turn the GameArea class into an actual game area if you manage to move things like the timer to the Program class. That might be a refactoring effort beyond the beginner level, though.

Share this post


Link to post
Share on other sites
which has the static main method. (standard C# stuff)

Just something to know if you like learning about other languages too smile.png : most languages feature a fixed entry point, C# and Java both need to have a class with a static main method. C and C++ need a global mani method as entry point.

 

Not necessarily. Calling methods named SlowDownBall and SpeedUpBall seems pretty readable and sensible.
Calling a ChangeSpeed method instead might make it harder to read.
The implementation of those two methods could probably use another method like ChangeSpeed to be more compact.

I think an increaseSpeed() function which allows negative values could also be just as readable, but be more flexible. I guess it depends on the person.

 

I agree with DareDevelopers latest post that it's good practice to group related functionality into different classes. There's also some rule (I don't know the name, guessing university could be good for something huh.png ) that says a class should only have one purpose.

Edited by ProtectedMode

Share this post


Link to post
Share on other sites

 

which has the static main method. (standard C# stuff)

Just something to know if you like learning about other languages too smile.png : most languages feature a fixed entry point, C# and Java both need to have a class with a static main method. C and C++ need a global mani method as entry point.

 

Not necessarily. Calling methods named SlowDownBall and SpeedUpBall seems pretty readable and sensible.
Calling a ChangeSpeed method instead might make it harder to read.
The implementation of those two methods could probably use another method like ChangeSpeed to be more compact.

I think an increaseSpeed() function which allows negative values could also be just as readable, but be more flexible. I guess it depends on the person.

 

I agree with DareDevelopers latest post that it's good practice to group related functionality into different classes. There's also some rule (I don't know the name, guessing university could be good for something huh.png ) that says a class should only have one purpose.

 

SoC - separation of concerns - might be what you are thinking of ...

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!