Pong game in WPF

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 = 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 = 760 && (_ball.Y > _rightPad.YPosition - 20 && _ball.Y _leftPad.YPosition - 20 && _ball.Y

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 _ri

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  _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  _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.




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







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.

StackExchange Code Review Q#38451, answer score: 6