Pong Game in WinForms

Question

I've wanted to make a Pong game for awhile, so I eventually got around to it now, it didn't take that long except for the reflections which I unfortunately didn't end up being satisfied with. I wanted to implement a more elegant solution utilizing trigonometry to calculate ball curvature upon impact, but alas it was my shortcomings in math, not programming, that prevented me from implementing something like this.

I did a decent attempt at asking for helping (Math SE, Physics SE, Stack Overflow, Game Dev SE) and I had a look at other source codes of Pong. It seems that the approach people are taking to ball reflection is vastly different from version to version, some people also opt for very elegant solutions using vectors, trigonometry, angle of incidence and even taking the ball mass and radius into account, while others like me opt for a simple reflection.

Other than that I feel pretty satisfied, the code definitely isn't very light-weight compared to some other implementations that I've seen but I hope it makes up for that in being very readable and well-structured.

I'd love feedback on the rest of the code.

IGameView.cs

interface IGameView
    {
        Size Boundaries { get; }
        /// <summary>
        /// This method will draw all game objects. 
        /// </summary>
        /// <param name="ball"></param>
        /// <param name="paddle"></param>
        /// <param name="paddle2"></param>
        void Draw(Ball ball, Paddle paddle, Paddle paddle2);
        void PlayerWon(Player winner);
        /// <summary>
        /// Releases all the resources allocated by this IGameView.
        /// </summary>
        void Release();
    }

IGameController.cs

    internal enum Move
    {
        Up, Down
    }

    interface IGameController
    {
        Player[] Players { get; }
        PeriodicTick GameTicker { get; }
        /// <summary>
        /// Initializes this instance of the IGameController.
        /// </summary>
        void Start();
        void Refresh();
        void PlayerScore(Player player);
        void MovePaddle(int playerId, Move move);
        event PlayerWonHandler PlayerWin;
    }

Delegates.cs

namespace Pong
    {
        internal delegate void PlayerWonHandler(Player winner);
        internal delegate void TickEventHandler();
        internal delegate void CollisionHandler(CollisionType collisionType);
    }

NativeMethods.cs

internal static class NativeMethods
        {
            [DllImport("user32.dll")]
            [return: MarshalAs(UnmanagedType.Bool)]
            internal static extern bool GetKeyboardState(byte[] lpKeyState);
        }

Keyboard.cs

static class Keyboard
    {
        public static Keys[] GetPressedKeys()
        {
            Byte[] keys = new Byte[256];
            var pressedKeys = new List<Keys>(); 

            if (!NativeMethods.GetKeyboardState(keys))
            {
                int err = Marshal.GetLastWin32Error();
                throw new Win32Exception(err);
            }

            for (int i = 0; i < keys.Length; i++)
            {
                byte key = keys[i];

                if ((key & 0x80) != 0)
                {
                    pressedKeys.Add((Keys)i);
                }

            }
            return pressedKeys.ToArray();
        }
    }

Vector.cs

struct Vector
    {
        public int X;
        public int Y;

        public Vector(int x, int y)
        {
            X = x;
            Y = y;
        }
    }

PeriodicTick.cs

class PeriodicTick
    {
        private int tickInterval; // Defines how often you want the Tick event to be raised in milliseconds
        public CancellationTokenSource CancellationTokenSrc { get; set; }
        public Task TickTask { get; private set; }
        public event TickEventHandler Tick;

        public PeriodicTick(int tickInterval)
        {
            this.tickInterval = tickInterval;
            CancellationTokenSrc = new CancellationTokenSource();
        }

        public void Start()
        {
            var token = CancellationTokenSrc.Token;
            TickTask = Task.Factory.StartNew(
                () => 
                {
                    Thread.CurrentThread.Name = "TickThread";
                    while (true)
                    {
                        if (token.IsCancellationRequested) { break; }
                        Task.Delay(tickInterval).Wait(); // Wait for tickInterval milliseconds...
                        Tick(); // ...Then tick!
                    }
                },
                token);
        }
    }

Player.cs

class Player
    {
        private static int ctr;
        public readonly int Id;
        public int Score { get; set; }
        public Paddle Paddle { get; set; }

        public Player(Orientation orientation, IGameView view)
        {
            Paddle = new Paddle(orientation, view);
            Id = ctr++;
        }
    }

Paddle.cs

internal enum Orientation
    {
        Left, Right
    }

    class Paddle
    {
        public Point Position;
        public readonly Orientation Orientation;
        public readonly Size Size = new Size(0, 20);

        public Paddle(Orientation orientation, IGameView view) 
        {
            Orientation = orientation;
            // Paddle starting position should be in the center Y-axis with differing X values
            int x = (orientation.Equals(Orientation.Left)) ? view.Boundaries.Width - (view.Boundaries.Width / 20) : view.Boundaries.Width / 20; 
            Position = new Point(x, view.Boundaries.Height / 2);
        }

        public Point[] GetHitbox()
        {
            var hitLocations = new List<Point>();

            for (int i = 0; i < this.Size.Height; i++)
            {
                for (int j = 0; j < 3; j++)
                {
                    if (this.Orientation.Equals(Orientation.Right))
                    {
                        hitLocations.Add(new Point(this.Position.X + j, this.Position.Y + i));
                    } else
                    {
                        hitLocations.Add(new Point(this.Position.X - j, this.Position.Y + i));
                    }
                }
            }
            return hitLocations.ToArray();
        }
    }

GameController.cs

class GameController : IGameController
    {
        private const int WINNING_SCORE = 10;
        private readonly Ball ball;
        public readonly IGameView view;
        public event PlayerWonHandler PlayerWin;
        public Player[] Players { get; private set; }
        public PeriodicTick GameTicker { get; private set; }

        public GameController(IGameView view, Player player, Player player2)
        {
            this.view = view;
            Players = new Player[] { player, player2 };
            ball = new Ball(view, this);
        }

        public void Start()
        {
            const int REFRESH_RATE = 25; // TIME IN MILLISECONDS IN WHICH THE GAME RECALCULATES ALL OBJECT POSITIONS
                                         // CHANGING THIS CAN DRAMATICALLY AFFECT GAME PERFORMANCE!!!!
            GameTicker = new PeriodicTick(REFRESH_RATE);
            GameTicker.Tick += Refresh; // Each time our PeriodicTick ticks we will refresh the ball position and angle

            //Start the ticker...
            GameTicker.Start();

            //...then get the ball moving
            ball.ballController.Center(Players[new Random().Next(Players.Length)]);
        }

        public void Refresh()
        {
            //Update positions of ball here...
            ball.ballController.UpdatePosition();

            //.. Then call Draw() in the IGameView
            view.Draw(ball, Players[0].Paddle, Players[1].Paddle);
        }

        public void MovePaddle(int playerId, Move move)
        {
            const int MOVE_MODIFIER = 15; // THIS DEFINES HOW MANY PIXELS TO MOVE THE PADDLE PER MOVE CALL
            int id = playerId;

            if (move.Equals(Move.Up))
            {
                Players[id].Paddle.Position.Y = Math.Max(Players[id].Paddle.Position.Y - MOVE_MODIFIER, 0);
            } 
            else
            {
                // We must add the length of the paddle here because it will transgress the boundary on it's other side
                Players[id].Paddle.Position.Y = Math.Min(
                    Players[id].Paddle.Position.Y + MOVE_MODIFIER, view.Boundaries.Height - Players[id].Paddle.Size.Height);
            }
        }

        public void PlayerScore(Player player)
        {
            if (++player.Score == (WINNING_SCORE)) // Check if player wins if so...
            {
                // ... Raise PlayerWin event, cancel the GameTicker 
                //     and release all resources used by this instance of IGameView
                PlayerWin(player);                                    
                GameTicker.CancellationTokenSrc.Cancel(); 
                view.Release();
            }

            ball.ballController.Center(player); // Recenter the ball
        }
    }

Ball.cs

class Ball
    {
        private readonly IGameView view;
        private readonly IGameController gameController;
        public readonly BallController ballController;
        private Vector velocity;
        private double angle;
        private event CollisionHandler Collision;
        public Point Point { get; private set; }

        public Ball(IGameView view, IGameController controller)
        {
            this.view = view;
            this.gameController = controller;
            ballController = new BallController(this);
        }

        internal class BallController
        {
            private Ball ball;
            private Random rng = new Random();
            private int baseMod;

            public BallController(Ball ball)
            {
                this.ball = ball;
                ball.Collision += OnCollision;
            }

            public void Center(Player server)
            {
                ball.Point = new Point(ball.view.Boundaries.Width / 2, ball.view.Boundaries.Height / 2);

                //The ball will start moving from the center Point towards one of the different sides
                ball.angle = (server.Paddle.Orientation.Equals(Orientation.Left)) ? Math.PI : Math.PI * 2;

                //Re-randomize the base velocity of the ball
                baseMod = Math.Max(3, rng.Next(6));
            }

            public void UpdatePosition()
            {
                ball.velocity.X = (ball.angle == Math.PI) ? -5 : 5;
                ball.Point = new Point(ball.Point.X + ball.velocity.X, ball.Point.Y + ball.velocity.Y);


                //Check if the suggested point is beyond the boundaries of the window
                if (ball.Point.X > ball.view.Boundaries.Width || ball.Point.Y > ball.view.Boundaries.Height || ball.Point.X < 0
                    || ball.Point.Y < 0)
                {
                    ball.Collision(CollisionType.Boundary); // If it does raise collision event
                }

                //Check if the new point collides with the hitbox of a player paddle
                if (ball.gameController.Players[0].Paddle.GetHitbox().Any(point => point.Equals(ball.Point)) ||
                    ball.gameController.Players[1].Paddle.GetHitbox().Any(point => point.Equals(ball.Point)))
                {
                    ball.Collision(CollisionType.Paddle);
                }
            }

            public void OnCollision(CollisionType collisionType)
            {
                switch (collisionType)
                {
                    case CollisionType.Paddle:
                        ball.angle = (ball.angle == Math.PI) ? Math.PI * 2 : Math.PI;
                        ball.velocity.Y = (ball.angle == Math.PI) ? 5 : -5;
                        break;
                    case CollisionType.Boundary:
                        // If the collision is with a window boundary check if we need to bounce the ball 
                        // or make a player score
                        if (ball.Point.X > ball.view.Boundaries.Width || ball.Point.X < 0)
                        {
                            // If the angle of the ball of the ball is greater than ½ rad then the left paddle was 
                            // the shooter so he should score 
                            if (ball.angle > Math.PI / 2)
                            {
                                var scoringPlayer = Array.Find(ball.gameController.Players, player => player.Paddle.Orientation.Equals(Orientation.Left));
                                ball.gameController.PlayerScore(scoringPlayer);
                            } else // If not, then it's the right paddle
                            {
                                var scoringPlayer = Array.Find(ball.gameController.Players, player => player.Paddle.Orientation.Equals(Orientation.Right));
                                ball.gameController.PlayerScore(scoringPlayer);
                            }
                        } else
                        {
                            ball.velocity.Y = (ball.angle == Math.PI) ? -5 : 5;
                        }
                        break;
                }
            }
        }
    }

CollisionType.cs

enum CollisionType
{
    Paddle,
    Boundary
}

GameForm.cs

public partial class GameForm : Form, IGameView
    {
        public Size Boundaries { get; private set; }
        private Bitmap gameObjects;
        private IGameController gameController;
        private readonly Pen pen = new Pen(Color.White, 5);
        private readonly Font myFont = new System.Drawing.Font("Helvetica", 40, FontStyle.Regular);
        private Keys[] inputKeys = new Keys[] { Keys.Up, Keys.Down, Keys.W, Keys.S };

        public GameForm()
        {
            InitializeComponent();
        }

        private void GameForm_Load(object sender, EventArgs e)
        {
            Boundaries = new Size(ClientSize.Width, ClientSize.Height); // Always set boundaries to size of the view control
            gameController = new GameController(this, new Player(Orientation.Left, this), new Player(Orientation.Right, this));
            gameController.PlayerWin += (this as IGameView).PlayerWon;
            // Start a seperate worker task for game logic
            Task.Factory.StartNew(
                () => 
                {
                    System.Threading.Thread.CurrentThread.Name = "WorkerThread";
                    gameController.Start(); 
                }); 
            gameObjects = new Bitmap(Boundaries.Width, Boundaries.Height);
        }

        void IGameView.PlayerWon(Player winner)
        {
            MessageBox.Show(String.Concat("Player ", winner.Id.ToString() + " won!"));
        }

        void IGameView.Draw(Ball ball, Paddle paddle, Paddle paddle2)
        {
            // If the game is over we need to cease drawing as that will throw an exception
            // Due to PlayerWin event calling a release of drawing resources in this instance of IGameView
            if (gameController.Players.Any(player => player.Score >= 10)) 
            {       
                return;
            }

            //Draw to bitmap
            using (Graphics gameObj = Graphics.FromImage(gameObjects))
            {
                gameObj.Clear(Color.Black); // Clear area to allow redrawing of all game objects
                gameObj.DrawEllipse(pen, new Rectangle(ball.Point, new Size(5, 5))); // Ball
                gameObj.DrawLine(pen, new Point(Size.Width / 2, 0), new Point(Size.Width / 2, Size.Height)); // Net
                gameObj.DrawString(gameController.Players[0].Score.ToString(), myFont, pen.Brush, Boundaries.Width / 2.7f, 0); // Score p1
                gameObj.DrawString(gameController.Players[1].Score.ToString(), myFont, pen.Brush, Boundaries.Width / 1.8f, 0); // Score p2
                gameObj.DrawLine(pen, paddle.Position, new Point(paddle.Position.X, paddle.Position.Y + paddle.Size.Height));
                gameObj.DrawLine(pen, paddle2.Position, new Point(paddle2.Position.X, paddle2.Position.Y + paddle2.Size.Height));
                Invalidate(); // Invalidate to force redraw of game objects      
            }
        }

        void IGameView.Release()
        {
            pen.Dispose();
            myFont.Dispose();
        }

        private void GameForm_Paint(object sender, PaintEventArgs e)
        {
            Graphics graphics = e.Graphics;
            graphics.DrawImage(gameObjects, new Point(0, 0));
            // No dispose here because we're using double buffering
        }

        private void GameForm_KeyDown(object sender, KeyEventArgs e)
        {
            //Check if pressed key equals any of the player assigned keys.. If so call MovePaddle()
            foreach (Keys key in Keyboard.GetPressedKeys())
            {
                if (inputKeys.Any(entry => entry.Equals(key)))
                {
                    switch (key)
                    {
                        case Keys.Up:
                            gameController.MovePaddle(0, Pong.Move.Up);
                            break;
                        case Keys.Down:
                            gameController.MovePaddle(0, Pong.Move.Down);
                            break;
                        case Keys.W:
                            gameController.MovePaddle(1, Pong.Move.Up);
                            break;
                        case Keys.S:
                            gameController.MovePaddle(1, Pong.Move.Down);
                            break;
                    }
                }
            }           
        }
    }

Answer 1

IGameView.cs

You have a method called Release with the comments of Release as Releases all the resources allocated by this IGameView. This is the purpose of the interface IDisposable in the System namespace. I would recommend changing to that instead of Release.

IGameController.cs

You have a method called Start, but your comment says that it initializes the instance. It's not until I go GameController do we see that you do both an initialization AND a start. This is a violation of the Single Responsibility Pattern (SRP). In short it says your class should be responsible for one thing, your methods should only do one thing, and your variables should only mean one thing. It would be better to use your constructor to initialize all your variables, or to make a method in your interface called Initialize(). The context would say that you should use a method in your interface. Your method MovePaddle takes a player ID and a direction, and uses that to move the paddle. Your method Refresh is a good idea, but when I look at the implementation I feel like you lie about it's purpose a little bit. You have a GameTicker that calls it... I think it would be better if PeriodicTick had a instance of IGameController instead of the other way around, and then put the Tick to call the refresh command from IGameController. Although it does the EXACT same thing, this would make more sense to put that in the Ticker because I have to ask my self...well what does this do? I can't tell by looking at PeriodicTick. all I know is that it can be cancled. Last note about this too is that it is not a good idea to have a interface depend on a class. Rather it is always a better idea to have a Class depend on a interface.

GameController.cs

there are 2 instances where you have a constant defined in the code. MOVE_MODIFER being one, REFRESH_RATE being another. Nothing wrong with having these, but as your comment says it critically affects the performance of the game. Things like this should be put into a config file, or a setting that can be edited from your form. It may be a good refresh for your computer, but what about mine? how about all 4 of my computers?

In General

Try not to expose your variables in your classes. Expose your methods that act upon your variables. And example of this would be something like this.

public static void main(string[] args)
{
    BadGasTank badGasTank = new BadGasTank(15);//initialize with 15gallons
    car = new Car(badGasTank);
    car.Drive(100); //drive 100 miles
    double gallonsUsed = 15 - badGasTank.Gallons;

    GoodGasTank gasTank = new GoodGasTank(SomeUnknownAmount);
    car = new Car(gasTank);
    car.Drive(100); //drive 100 miles
    gallonsUsed = gasTank.getGallonsUsed();
}

So in your case it would be better to change your PeriodicTick to something like this.

class PeriodicTick
{
    private int tickInterval; // Defines how often you want the Tick event to be raised in milliseconds
    private readonly IGameController gameController;
    private CancellationTokenSource CancellationTokenSrc;

    public PeriodicTick(int tickInterval, IGameController controller)
    {
        this.tickInterval = tickInterval;
        CancellationTokenSrc = new CancellationTokenSource();
        this.gameController = controller;
    }

    public void Start()
    {
        var token = CancellationTokenSrc.Token;
        var tickTask = Task.Factory.StartNew(
            () =>
            {
                Thread.CurrentThread.Name = "TickThread";
                while (true)
                {
                    if (token.IsCancellationRequested) { break; }
                    Task.Delay(tickInterval).Wait(); // Wait for tickInterval milliseconds...
                    this.gameController.Refresh();
                }
            },
            token);
    }

    public void Stop()
    {
        CancellationTokenSrc.Cancel();
    }
}

Really it comes down to it is a bad idea to manipulate/depend on another classes properties. As is the case here gameController.Players.Any(player => player.Score >= 10) It would be better to have change it to something like gameController.IsGameOver()

Keyboard.cs

this class can be a little tricky. But with the principle as before we want to make sure that we don't depend on another classes variables. Granted it may not feel like a variable in this case because you are calling GetKeyboardState from NativeMethods. (side note: I personally would move GetKeyboardState to Keyboard, and mark it as private, but there is nothing wrong with the way you have it now) Now making Keyboard a static class might not be the best idea here. In your GameForm you expressly ask for which keys are down. This might benefit from a queue working in the background and firing an event. Some pseudo code:

class Keyboard()
{
    private readonly Keys[] keysToWatchFor;
    private bool keepThreadAlive;//or use cancel token like you did before
    public Keyboard(params Keys[] keysToWatchFor)
    {
        this.keysToWatchFor = keysToWatchFor;
    }

    public void Start()
    {
        //Make a thread and start it
    }
    public void Stop()
    {
        //Cancel Thread 
    }
    private void MonitorKeyPresses()
    {
        while(keepThreadAlive)
        {
            Thread.Sleep(10);
            byte[] keys = nativeMethods.GetKeyboardState(); //<- Notice this! good idea
            for(int i=0; i<keysToWatchFor.Length; i++)
            {
                int key = keysToWatchFor[i];
                if(IsKeyDown(keys[key]))
                    FireKeyPressed(key);
            }
        }
    }
    private static bool IsKeyDown(byte keyValue)
    {
        return ((keyValue & 0x80) != 0);
    }

    public event KeyPressedEventHandler KeyPressed;
    private void FireKeyPressed(int key)
    {
        if(KeyPressed != null) KeyPressed(key);
    }
}

I would consider putting in a few methods in Player such as MovePaddleUp, and MovePaddleDown. this could possible lead to you not needing the enum Move.

In the end I would think about the few principles I've stated and think over the rest of your classes that I did not cover and see if you can't work them just a little bit more. Lastly, I've ALWAYS hated it when when a Form extends something other than a Form. This makes it very fragile. Instead just override/subscribe to events in your form that you want your controller to do (like you already do with KeyDown)