Pong game in WPF

Question

I would especially like to find out how this project should be constructed to be "fully MVVM". I did some reading about MVVM and I started to implement some ideas, but I don't fully understand it yet.

The game works, but I would like to get some advice regarding what could be done better/differently. Does it make sense to make such project MVVM in the first place?

MainWindow.xaml.cs:

using System;
using System.Windows;
using System.Windows.Input;
using System.Windows.Threading;

namespace PongGame
{

public partial class MainWindow
{
    public MainWindow()
    {
        InitializeComponent();
        DataContext = _ball;
        RightPad.DataContext = _rightPad;
        LeftPad.DataContext = _leftPad;
        Ball.DataContext = _ball;
        label4.DataContext = _rightPad;

        var timer = new DispatcherTimer();
        timer.Interval = TimeSpan.FromMilliseconds(10);
        timer.Start();
        timer.Tick += _timer_Tick;
    }

    private double _angle = 155;
    private double _speed = 5;
    private int _padSpeed = 7;

    void _timer_Tick(object sender, EventArgs e)
    {
        if (_ball.Y <= 0) _angle = _angle + (180 - 2*_angle);
        if (_ball.Y >= MainCanvas.ActualHeight - 20)_angle = _angle + (180 - 2*_angle);

        if (CheckCollision() == true)
        {
            ChangeAngle();
            ChangeDirection();
        }
        double radians = (Math.PI / 180) * _angle;
        Vector vector = new Vector { X = Math.Sin(radians), Y = -Math.Cos(radians) };
        _ball.X += vector.X * _speed;
        _ball.Y += vector.Y * _speed;

        if (_ball.X >= 790)
        {
            _ball.LeftResult += 1;
            GameReset();
        }
        if (_ball.X <= 5)
        {
            _ball.RightResult += 1;
            GameReset();
        }
    }

    private void GameReset()
    {
        _ball.Y = 210;
        _ball.X = 380;
    }

    private void ChangeAngle()
    {
        if (_ball.MovingRight == true) _angle = 270 - ((_ball.Y + 10) - (_rightPad.YPosition + 40));
        else if (_ball.MovingRight == false) _angle = 90 + ((_ball.Y + 10) - (_leftPad.YPosition + 40));
    }

    private void ChangeDirection()
    {
        if (_ball.MovingRight == true) _ball.MovingRight = false;
        else if (_ball.MovingRight == false) _ball.MovingRight = true;
    }

    private bool CheckCollision()
    {
        bool collisionResult = false;
        if (_ball.MovingRight == true)
            collisionResult = _ball.X >= 760 && (_ball.Y > _rightPad.YPosition - 20 && _ball.Y < _rightPad.YPosition + 80);

        if (_ball.MovingRight == false)
            collisionResult = _ball.X <= 20 && (_ball.Y > _leftPad.YPosition - 20 && _ball.Y < _leftPad.YPosition + 80);

        return collisionResult;
    }


    readonly Ball _ball = new Ball { X = 380, Y = 210 , MovingRight = true};

    readonly Pad _leftPad = new Pad { YPosition = 90 };
    readonly Pad _rightPad = new Pad { YPosition = 70 };


    private void MainWindow_OnKeyDown(object sender, KeyboardEventArgs e)
    {
        if (Keyboard.IsKeyDown(Key.W)) _leftPad.YPosition -= _padSpeed;
        if (Keyboard.IsKeyDown(Key.S)) _leftPad.YPosition += _padSpeed;
        if (Keyboard.IsKeyDown(Key.Up)) _rightPad.YPosition -= _padSpeed;
        if (Keyboard.IsKeyDown(Key.Down)) _rightPad.YPosition += _padSpeed;

    }
}
}

Ball.cs:

using System.ComponentModel;
using PongGame.Annotations;

namespace PongGame
{
class Ball : INotifyPropertyChanged
{
    private double _x;
    private double _y;
    private bool _movingRight;
    private int _leftResult;
    private int _rightResult;

    public double X
    {
        get { return _x; }
        set
        {
            _x = value;
            OnPropertyChanged("X");
        }
    }

    public double Y
    {
        get { return _y; }
        set
        {
            _y = value;
            OnPropertyChanged("Y");
        }
    }

    public bool MovingRight
    {
        get { return _movingRight; }
        set
        {
            _movingRight = value;
            OnPropertyChanged("MovingRight");
        }
    }

    public int LeftResult
    {
        get { return _leftResult; }
        set
        {
            _leftResult = value;
            OnPropertyChanged("LeftResult");
        }
    }

    public int RightResult
    {
        get { return _rightResult; }
        set
        {
            _rightResult = value;
            OnPropertyChanged("RightResult");
        }
    }


    public event PropertyChangedEventHandler PropertyChanged;

    [NotifyPropertyChangedInvocator]
    protected virtual void OnPropertyChanged(string propertyName)
    {
        PropertyChangedEventHandler handler = PropertyChanged;
        if (handler != null) handler(this, new PropertyChangedEventArgs(propertyName));
    }
}
}

Pad.cs:

using System.ComponentModel;
using System.Linq;
using System.Text;
using PongGame.Annotations;


namespace PongGame 
{
class Pad : INotifyPropertyChanged
{
    private int _yPosition;

    public int YPosition
    {
        get { return _yPosition; }
        set { _yPosition = value; 
        OnPropertyChanged("YPosition");}
    }

    public event PropertyChangedEventHandler PropertyChanged;

    [NotifyPropertyChangedInvocator]
    protected virtual void OnPropertyChanged(string propertyName)
    {
        PropertyChangedEventHandler handler = PropertyChanged;
        if (handler != null) handler(this, new PropertyChangedEventArgs(propertyName));
    }
}
}

MinWindow.xaml:

    <Window x:Class="PongGame.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:system="clr-namespace:System;assembly=System"
        KeyDown="MainWindow_OnKeyDown"
        Title="MainWindow" Height="503" Width="824" ResizeMode="NoResize">
    <Canvas Width="800" Height="475" Name="MainCanvas" Background="#FFD1D1D1">

        <Rectangle Height="80" Width="20" Fill="Blue"
                   Canvas.Top="{Binding YPosition}"

                   Name="LeftPad"></Rectangle>
        <Rectangle Height="80" Width="20" Fill="Brown" 
                   Canvas.Top="{Binding YPosition}" 
                   Canvas.Left="780" 
                   Name="RightPad"></Rectangle>
        <Line X1="400" X2="400" Y2="500" Y1="0" StrokeThickness="2" Stroke="#FFDBB062"></Line>
        <Ellipse Width="20" Height="20" Canvas.Left="{Binding X}"
                 Canvas.Top="{Binding Y}" Name="Ball" DataContext="{Binding Path=ball}" StrokeThickness="0.1" Fill="#FFF84949">

        </Ellipse>
        <Label Canvas.Left="630" Canvas.Top="34" Content="{Binding Y}" Height="28" Name="label1" />
        <Label Canvas.Left="565" Canvas.Top="34" Content="Y of ball" Height="28" Name="label2" />
        <Label Canvas.Left="535" Canvas.Top="68" Content="Y of right pad" Height="28" Name="label3" />
        <Label Canvas.Left="630" Canvas.Top="68" Content="{Binding YPosition}" Height="28" Name="label4" />
        <Label Canvas.Left="638" Canvas.Top="128" Content="{Binding MovingRight}" Height="28" Name="label5" />
        <Label Canvas.Left="184" Canvas.Top="34" Content="{Binding X}" Height="28" Name="label6" />
        <Label Canvas.Left="119" Canvas.Top="34" Content="X of ball" Height="28" Name="label7" />
        <Label Canvas.Left="89" Canvas.Top="68" Content="X of right pad" Height="28" Name="label8" />
        <Label Canvas.Left="184" Canvas.Top="68" Content="{Binding YPosition}" Height="28" Name="label9" />
        <Label Canvas.Left="349" Canvas.Top="35" Content="{Binding LeftResult}" Height="auto" Name="label10" 
               FontSize="40" Foreground="Blue"/>
        <Label Canvas.Left="421" Canvas.Top="35" Content="{Binding RightResult}" Height="auto" Name="label11" 
               FontSize="40" Foreground="Brown"/>
    </Canvas>
</Window>

Answer 1

DataContext = _ball;
RightPad.DataContext = _rightPad;
LeftPad.DataContext = _leftPad;
Ball.DataContext = _ball;
label4.DataContext = _rightPad;

I don't think you should be setting DataContext for each control like this. Instead, you should have a view model object that you set as the DataContext of the whole window. That view model would contain all the context objects that you need and you would use binding in your XAML to get to them.

Also, label4 is a very bad name, you should use descriptive names for everything.

And if you do it this way, shouldn't label9 be here too? (If the first label was called something like LeftPadYPosition, then it would be much easier to see that the label for right pad is missing here.)

---

var timer = new DispatcherTimer();

I believe this timer will be collected as soon as the garbage collector runs, which means it will stop ticking then. You should keep the timer in a field.

---

if (_ball.Y <= 0) _angle = _angle + (180 - 2*_angle);

One line conditions are quite unreadable, the statement should be on its own line.

---

if (CheckCollision() == true)

This could be simplified to just if (CheckCollision()).

The same applies to similar conditions throughout your code. And == false should be replaced with !.

---

double radians = (Math.PI / 180) * _angle;
Vector vector = new Vector { X = Math.Sin(radians), Y = -Math.Cos(radians) };

Converting angle to coordinates on the unit circle sounds like something that should be in its own method. And so does converting degrees to radians.

---

if (_ball.X >= 790) 

Why exactly 790? Constants like these should be named. This makes the code clearer and easier to modify.

---

bool collisionResult = false;
if (_ball.MovingRight == true)
    collisionResult = _ball.X >= 760 && (_ball.Y > _rightPad.YPosition - 20 && _ball.Y < _rightPad.YPosition + 80);

if (_ball.MovingRight == false)
    collisionResult = _ball.X <= 20 && (_ball.Y > _leftPad.YPosition - 20 && _ball.Y < _leftPad.YPosition + 80);

return collisionResult;

You could simplify this to:

if (_ball.MovingRight)
    return _ball.X >= 760 && (_ball.Y > _rightPad.YPosition - 20 && _ball.Y < _rightPad.YPosition + 80);
else
    return _ball.X <= 20 && (_ball.Y > _leftPad.YPosition - 20 && _ball.Y < _leftPad.YPosition + 80);

(That else isn't actually necessary, but I like the symmetry.)

The previous comment about named constants also applies here.

And it would also help if you extracted the second part of the condition into a separate method, taking Pad as a parameter. This way, you avoid duplicated code.

---

<Rectangle Height="80" Width="20" Fill="Blue"
           Canvas.Top="{Binding YPosition}"
           Name="LeftPad"></Rectangle>

Consider using empty element tags, they are shorter and imply that you don't intend to add anything inside the element later:

<Rectangle Height="80" Width="20" Fill="Blue"
           Canvas.Top="{Binding YPosition}"
           Name="LeftPad" />

---

<Label Canvas.Left="630" Canvas.Top="34" Content="{Binding Y}" Height="28" Name="label1" />

In cases like this, using WPF panels to place the controls is much better: you don't have to specify the exact position, the layout is easier to change and it also works better with resizing.

Answer 2

quick look; What the heck is this for ?

public event PropertyChangedEventHandler PropertyChanged;

[NotifyPropertyChangedInvocator]
protected virtual void OnPropertyChanged(string propertyName)
{
    PropertyChangedEventHandler handler = PropertyChanged;
    if (handler != null) handler(this, new PropertyChangedEventArgs(propertyName));

I've never needed or used [NotifyPropertyChangedInvocator] (re-sharper thing, not part of c#)

Why re-assign the external variable? Just do

if (PropertyChanged!= null) PropertyChanged(this, new
PropertyChangedEventArgs(propertyName)); ....

It is also fairly common these days to let the compiler do the work;

void OnPropertyChanged([CallerMemberName] String T = "")
{
    if (PropertyChanged != null)
        PropertyChanged(this, new PropertyChangedEventArgs(T));
}

So your update code is cleaner e.g.;

    float _FeedRate = 5f;
    [Category("Machine")]
    public float FeedRate { get { return _FeedRate; } set { _FeedRate = value; OnPropertyChanged(); } }

Consider INotifyPropertyChanged vs Dependency properties. They do the same thing. I tend to keep visual info in depencies, data model stuff in propertyChanged.

You really have no data model per se. The paddle locations are indeed visual, but to me, the ball location, and collision logic should be the data Model part of mvvm.

As you probably know by now, canvas has some nasty re-sizing problems, but that does not reflect on your mvvm strategy.

Answer 3

This bit of code is much longer than it needs to be:

private void ChangeDirection()
{
    if (_ball.MovingRight == true) _ball.MovingRight = false;
    else if (_ball.MovingRight == false) _ball.MovingRight = true;
}

It can simplified to:

private void ChangeDirection()
{
    _ball.MovingRight = !_ball.MovingRight;
}

With bool conditionals, you never need to use (variable == true) or (variable == false). Rather just use (variable) or (!variable) respectively.