11
\$\begingroup\$

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
    {
        /// <summary>
        /// The main entry point for the application.
        /// </summary>
        [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(OnGameTimeTick);

            Movable.mainForm = form;
            player = new Player();
            player.Size = new Size(20, 50);
            player.Location = new Point(player.Width / 2, form.Height / 2 - player.Height / 2);
            player.BackColor = Color.Aquamarine;

            player2 = new Player();
            player2.Size = new Size(20, 50);
            player2.Location = new Point(form.Width - (player2.Width * 2), form.Height / 2 - player2.Height / 2);
            player2.BackColor = Color.Aquamarine;

            /*
            ai = new AI();
            ai.AiSpeed = 3;
            ai.Size = new Size(20, 50);
            ai.Location = new Point(ClientSize.Width-(ai.Width+ai.Width / 2), ClientSize.Height / 2 - ai.Height / 2);
            ai.BackColor = Color.Blue;
            */
            ball = new Ball(3, 3);
            ball.Size = new Size(20, 20);
            ball.BackColor = Color.Red;
            ball.Location = new Point(form.Width / 2 - ball.Width / 2, form.Height / 2 - ball.Height / 2);
        }

        void OnGameTimeTick(object sender, EventArgs e)
        {
            ball.Location = new Point(ball.Location.X + ball.ballSpeedX, ball.Location.Y + ball.ballSpeedY);
            controller.CollisionGameArea(ball);
            player.move();
            controller.PaddleCollision(player, player2, ball);
            // ai.AiMove(ball);

        }

    }
}

GameArea.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
{
    public partial class GameArea : Form

    {
        Game game;
        const int GameAreaWidth = 1248;
        const int GameAreaHeight = 720;
        public GameArea()
        {

            InitializeComponent();
            this.Height = GameAreaHeight;
            this.Width = GameAreaWidth;
            this.StartPosition = FormStartPosition.CenterScreen;
            game = new Game(this);
            KeyDown += new KeyEventHandler(OnKeyDown);
        }
        private void Form1_Load(object sender, EventArgs e)
        {

        }
        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);
            }
        }
        private void GameArea_Paint(object sender, PaintEventArgs e)
        {

        }
    }
}

GameController.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
namespace Pong
{
    public class GameController
    {
        Form mainForm;

        protected int PointsPlayer1 { get; set; }
        protected int PointsPlayer2 { get; 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;
        }
        public void CollisionGameArea(Ball obj)
        {
            if (obj.Location.Y > mainForm.Height - obj.Height * 3 || obj.Location.Y < 0)
            {
                obj.ballSpeedY = -obj.ballSpeedY;
            }
            else if (obj.Location.X > mainForm.Width)
            {
                PointsPlayer1 += 1;
                obj.resetBall();
            }
            else if (obj.Location.X < 0)
            {
                PointsPlayer2 += 1;
                obj.resetBall();
            }
        }

        public GameController(Form form)
        {
            mainForm = form;
        }

    }
}

Movable.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Drawing;
namespace Pong
{
    public class Movable
    {
        private PictureBox pictureBox { get; set; }
        static public Form mainForm;
        public Rectangle Bounds
        {
            get { return pictureBox.Bounds; }
        }
        public int Height
        {
            get { return pictureBox.Size.Height; }
        }
        public int Width
        {
            get { return pictureBox.Size.Width; }
        }
        public Size Size
        {
            get { return pictureBox.Size; }
            set
            {
                if (value.Height > 0 && value.Width > 0)
                    pictureBox.Size = value;
            }
        }
        public Point Location
        {
            get { return pictureBox.Location; }
            set { pictureBox.Location = value; }
        }
        public Color BackColor
        {
            get { return pictureBox.BackColor; }
            set { pictureBox.BackColor = value; }
        }
        public Movable(Form form, int scoreBoardHeight)
        {
            mainForm = form;
            mainForm.Height -= scoreBoardHeight;
            pictureBox = new PictureBox();
            form.Controls.Add(pictureBox);
        }
        public Movable(Form form)
        {
            pictureBox = new PictureBox();
            form.Controls.Add(pictureBox);

        }
        public Movable()
        {
            pictureBox = new PictureBox();
            mainForm.Controls.Add(pictureBox);
        }
    }

}

Player.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Drawing;
namespace Pong
{
    public class Player : Movable
    {

        public void move()
        {
            if (mainForm.PointToClient(Control.MousePosition).Y >= Height / 2 && mainForm.PointToClient(Control.MousePosition).Y <= mainForm.Height - Height)
            {
                int playerX = Width / 2;
                int playerY = mainForm.PointToClient(Control.MousePosition).Y - Height / 2;
                Location = new Point(playerX, playerY);
            }
        }

    }
}

Ball.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Drawing;
namespace Pong
{
    public class Ball : Movable
    {
        public int ballSpeedX { set; get; }
        public int ballSpeedY { set; get; }
        public void resetBall()
        {

            Location = new Point(mainForm.Width / 2 - Width / 2, mainForm.Height / 2 - Height / 2);
        }
        public Ball(int ballSpeedX, int ballSpeedY)
        {
            this.ballSpeedX = ballSpeedX;
            this.ballSpeedY = ballSpeedY;
        }


    }
}
\$\endgroup\$
11
\$\begingroup\$

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 the Player class, and call it instead of putting this logic into the form class.

Lastly, you really need to clean up the indentation/whitespace, in Visual Studio CTRL + K then CTRL + D will automatically clean a lot of it up for you.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot!!! Can you also tell me whether the approaches I took for KeyDown event I took? is it correct? Is it fine to be left in the form class? \$\endgroup\$ – Jam0131 Oct 24 '16 at 15:18
  • \$\begingroup\$ @Jam0131 I added a blurb about that. \$\endgroup\$ – Der Kommissar Oct 24 '16 at 15:26
  • \$\begingroup\$ Thank you very much, but there is still a small little thing I want to clear up, I actually wanted to add KeyDown to a player class, but I already have a move method that operates based on mouse. I wasn't sure how should I implement two methods for moving for a player. What I mean is, should I create some sort of player ID in a constructor that dinstingushes which player should move on mouse and which one based on keyboard? \$\endgroup\$ – Jam0131 Oct 24 '16 at 15:35
  • \$\begingroup\$ @Jam0131 Yes, that would be an acceptable solution. Ideally you would create two instances of a Player class in your form, and have the Form decide which one to move based on input, and then call appropriatePlayer.Move(distance). \$\endgroup\$ – Der Kommissar Nov 7 '16 at 17:17
  • \$\begingroup\$ You may want to make BallSpeed immutable since it is a struct/value type. \$\endgroup\$ – Dan Lyons Nov 7 '16 at 19:08
6
\$\begingroup\$

I will try to answer your question in a very general and abstract way, using examples from your code only when necessary.

Give software entities a NAME

This is the most beneficial thing that you can do, and do easily, to your code. Once you do this, everything else will come as a logical consequence. Name everything, including:

  • variables - I see you named them pretty well
  • methods
  • parts of methods - you can write some tiny comments before some logical block of code and separate it visually from the rest
  • regions of class - when you think there are some orthogonal parts of a class, separate them and add some comment or #region Once you name parts of methods, you will see some hidden variables - name them too.

Unnamed code HIDES information instead of REVEALING it - you look at the code and wonder "what was it supposed to do?". The truth is that every line of your code does SOMETHING specific, something that you had in mind.

Naming conventions

When you name your entities, use some naming schema. It doesn't matter what's the convention, what matters is using it throughout the code. Conventions let you read your code faster, because you can say many things of some object just by looking at it's name.

Example:

Before writing those two lines, you probably thought "I want to place player1 in upper/lower-left corner of the window, with a size(20, 50)." And then you translated it to very a specific set of computations. But then your intent was lost in those complicated computations. Now you have to reverse engineer your intent to know what's going on. Of course, at the BEGINNING(1) of development, YOU(2) remember everything. But as the codebase grows(1), time passes(1), and probably OTHER PEOPLE(2) get to this code, nothing is obvious anymore.

So try to write the code, that will not hide your intent. You could come up with something like this at the beginning:

player.Size = new Size(20, 50); // set the default player size
player.Location = new Point(player.Width / 2, form.Height / 2 - player.Height / 2); // move to upper-left corner of the form

DIVIDE

Once you do the naming, it's very easy to DIVIDE long elements by EXTRACTING some smaller ones. In the given example, we could make this:

var defaultPlayerSize = new Size(20, 50);
...
player.Size = defaultPlayerSize;
player.Location = UpperLeftCorner(player, form);
...
public Point UpperLeftCorner(Movable movable, Control container)
{
  return new Point(movable.Width / 2, container.Height / 2 - movable.Height / 2);
}

The details are not crucial - given good problem knowledge, you could think of better names. DefaultPlayerSize could become class constant.

Extracting methods is the most common way of simplifying code. And trust me - more often than required codebase gets way too much complicated and no one is sure what's exactly going on.

Many tools offer automated refactorings, which help you do extracting, without breaking anything. Early Visual Studio for example had only few refactorings, but Extract Method was the main one.

Remove DUPLICATION

After you name and divide some bigger elements, you will start to see common intents, which were hidden during translation into code. You will extract a method, and will get "Oh, but I already have that method - maybe I could use it?". And you will use that method. And the duplication will get smaller. And the cost of changing things will get cheaper.

As Robert C. Martin says (https://www.informit.com/articles/article.aspx?p=1313447):

Duplicate code is the root of all evil in software design. When a system is littered with many snippets of indentical, or nearly identical code, it is indicative of sloppiness, carelessness, and sheer unprofessionalism. It is the guilt-edged responsibility of all software developers to root out and eliminate duplication whenever they find it.

Do your best to minimize it.

Find proper place for your code.

When you extract some elements, you often put them nearby, in the same class or region of a class. But then you look at few extracted methods and start to think that they have something in common, something that distinguishes them from the rest. They do ONE JOB, have one RESPONSIBILITY. That's a signal that they should find some other place to live, and leave those others alone. You want them obay SINGLE RESPONSIBILITY(*) principle That is, you want to put similar things close, and different things separated. You use some coordinate computations in more than one class in your code. I think it could be beneficial to extract those computations, and then try to fit them into some new software entity - static class could do the job.

(*) What I said above may not be exact meaning of this principle, but I think it is some part of it.

Other things:

static public Movable.mainForm

This code is bad, I think, because it introduces static global state, that is manipulated through other classes. It was probably some kind of a shortcut, that let you create movables without passing form each time into constructor. But it's not clean. What happens when you write "new Movable()" at the beginning of your app? It will crash, throwing NRE.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot!!! So in the end, is it better to pass the form to every new object? I did it before but i thought it is a duplication ( :( ), so i changed it for this static property... Should I have a non static form variable in movable class and pass it in a constructor on creation of every moving object? \$\endgroup\$ – Jam0131 Oct 25 '16 at 13:23
  • 1
    \$\begingroup\$ You needed this mainForm because you add moveables to the form on creation time. There are two options: 1) You pass this form every time in the constructor and they add themselve 2) You add them to the form explicitly, on the caller's side, not inside the moveables - you could add all children in one line, passing an array of children Both solutions are exemplified in Windows Forms - check Control constructors and ways to add Control to Form/Parent. \$\endgroup\$ – JSparrow Oct 25 '16 at 13:58
  • \$\begingroup\$ Could you tell me how should I approach making a menu/selection screen in the game? \$\endgroup\$ – Jam0131 Oct 25 '16 at 14:38
  • \$\begingroup\$ I am not sure what you mean by menu/selection game and what you would like it to look like. You have to put more details, sample code if you got one, and probably start a new question, not necesarily on codereview site. \$\endgroup\$ – JSparrow Oct 25 '16 at 18:40
  • \$\begingroup\$ Well, when the game is switched on I want to have another "scene" where, there is menu ie single player, multiplayer choice, options etc., I do not know which way to create another 'scene" in the form is the most efficient. \$\endgroup\$ – Jam0131 Oct 25 '16 at 19:16
5
\$\begingroup\$

@EBrown has provided you high quality and comprehensive comments. I would suggest a few things to improve. If you'd like you can simplify some expressions, but that's a minor change.

Location = new Point((mainForm.Width - Width) / 2, (mainForm.Height - Height) / 2);

Split long if statements for better readability:

if (mainForm.PointToClient(Control.MousePosition).Y >= Height / 2 && 
    mainForm.PointToClient(Control.MousePosition).Y <= mainForm.Height - Height)

Use C# incrementation by one like:

PointsPlayer1++;

Consider using System.Configuration and App.config to set up Game parameters or use another configuration system.

Anyway, good job if you are starting with C#.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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