Bouncing balls program with speed up/speed down boxes and lines which change direction of ball

Question

I'm new to C#, background in Ada, C. This is my second lab in a course, output works correct but I feel like I've missed something about OOP. I've asked for feedback from lab 1, but didn't get any. Main objective of this lab is Inheritance, Abstract classes, Interface. Broad advice are as useful as detailed advice!

It uses WinForms. It's a "bouncing ball" program where lines will make the ball bounce, and boxes will speed up or speed down the ball.

Main will just run: Engine engine = new Engine(); engine.Run();

Engine class:

using System;
 using System.Collections.Generic;
 using System.Windows.Forms;
 namespace Bounce;
 public class Engine
 {
 MainForm Form = new MainForm();
 Timer Timer = new Timer();
 List<Ball> Balls = new List<Ball>();
 Random Random = new Random();
 List<Line> Lines = new List<Line>();
 List<SpeedBox> SpeedBoxes = new List<SpeedBox>();
 public void Run()
 {
 InitializeLinesBoxes();
 Form.BackColor = System.Drawing.Color.Black;
 Form.Paint += Draw;
 Timer.Tick += TimerEventHandler;
 Timer.Interval = 1000 / 25;
 Timer.Start();
 Application.Run(Form);
 }
 void InitializeLinesBoxes()
 {
 for (int i = 0; i < 2; i++)
 {
 var vertical = new VerticalLine(Random.Next(1, 100) + 600 * i, Random.Next(1, 100) - 15 * i, 450);
 Lines.Add(vertical);
 var horizontal = new HorizontalLine(Random.Next(1, 100) + 15 * i, Random.Next(1, 100) + 400 * i, 550);
 Lines.Add(horizontal);
 var speedUp = new SpeedUp(Random.Next(100, 700), Random.Next(100, 500), Random.Next(5, 20) * 10, Random.Next(5, 20) * 10);
 SpeedBoxes.Add(speedUp);
 var speedDown = new SpeedDown(Random.Next(100, 700), Random.Next(100, 500), Random.Next(5, 20) * 10, Random.Next(5, 20) * 10);
 SpeedBoxes.Add(speedDown);
 }
 }
 void TimerEventHandler(Object obj, EventArgs args)
 {
 if (Random.Next(400) < 25)
 {
 var ball = new Ball(400, 300, 10);
 Balls.Add(ball);
 }
 foreach (var ball in Balls) ball.Move();
 foreach (var ball in Balls) ball.CheckCollision(Lines, SpeedBoxes);
 Form.Refresh();
 }
 void Draw(Object obj, PaintEventArgs args)
 {
 foreach (var ball in Balls) ball.Draw(args.Graphics);
 foreach (var line in Lines) line.Draw(args.Graphics);
 foreach (var box in SpeedBoxes) box.Draw(args.Graphics);
 }
 }

Ball class:

using System;
using System.Collections.Generic;
using System.Drawing;
namespace Bounce;
public class Ball
 {
 Pen Pen = new Pen(Color.White);
 PointF Position;
 PointF Speed;
 float Radius;
 static Random Random = new Random();
 private Rectangle Bounds;
 public Ball(float x, float y, float radius)
 {
 Position = new PointF(x, y);
 Bounds = new Rectangle((int)x - (int)radius, (int)y - (int)radius, (int)radius * 2, (int)radius * 2);
 var xd = Random.Next(1, 6);
 var yd = Random.Next(1, 6);
 if (Random.Next(0, 2) == 0) xd = -xd;
 if (Random.Next(0, 2) == 0) yd = -yd;
 Speed = new PointF(xd, yd);
 Radius = radius;
 }
 public void Draw(Graphics g)
 {
 g.DrawEllipse(Pen, Position.X - Radius, Position.Y - Radius, 2 * Radius, 2 * Radius);
 }
 public void Move()
 {
 Position.X += Speed.X;
 Position.Y += Speed.Y;
 Bounds.X = (int)Position.X - (int)Radius;
 Bounds.Y = (int)Position.Y - (int)Radius;
 }
 public void CheckCollision(List<Line> lines, List<SpeedBox> speedBoxes)
 {
 foreach (var line in lines)
 {
 if (Bounds.IntersectsWith(line.Bounds))
 {
 Speed = line.ChangeDirection(Speed);
 }
 }
 foreach (var box in speedBoxes)
 {
 if (Bounds.IntersectsWith(box.Rectangle))
 {
 Speed = box.ChangeSpeed(Speed);
 }
 }
 }
 }

Line class:

using System.Drawing;
namespace Bounce
{
 public abstract class Line
 {
 protected Point LineStart;
 protected int Length;
 public Rectangle Bounds { get; protected set; }
 protected Line(int x, int y, int length)
 {
 LineStart = new Point(x, y);
 Length = length;
 }
 public abstract PointF ChangeDirection(PointF speed);
 public abstract void Draw(Graphics g);
 }
 public class VerticalLine : Line
 {
 Pen Pen = new Pen(Color.Yellow);
 public VerticalLine(int x, int y, int length) : base(x, y, length)
 {
 Bounds = new Rectangle(x, y, 1, length);
 }
 public override PointF ChangeDirection(PointF speed)
 {
 speed.X *= (-1);
 return speed;
 }
 public override void Draw(Graphics g)
 {
 g.DrawLine(Pen, LineStart.X, LineStart.Y, LineStart.X, LineStart.Y + Length);
 }
 }
 public class HorizontalLine : Line
 {
 Pen Pen = new Pen(Color.Green);
 public HorizontalLine(int x, int y, int length) : base(x, y, length)
 {
 Bounds = new Rectangle(x, y, length, 1);
 }
 public override PointF ChangeDirection(PointF speed)
 {
 speed.Y *= (-1);
 return speed;
 }
 public override void Draw(Graphics g)
 {
 g.DrawLine(Pen, LineStart.X, LineStart.Y, LineStart.X + Length, LineStart.Y);
 }
 }
}

Box class:

using System.Drawing;
 namespace Bounce;
public abstract class SpeedBox
 {
 public Rectangle Rectangle { get; protected set; }
 public SpeedBox(int x, int y, int width, int height)
 {
 Rectangle = new Rectangle(x, y, width, height);
 }
 public abstract void Draw(Graphics g);
 public abstract PointF ChangeSpeed(PointF speed);
 }
 public class SpeedUp : SpeedBox
 {
 Pen Pen = new Pen(Color.Red);
 public SpeedUp(int x, int y, int width, int height) : base(x, y, width, height)
 {
 }
 public override PointF ChangeSpeed(PointF speed)
 {
 speed.X *= 1.02f;
 speed.Y *= 1.02f;
 return speed;
 }
 public override void Draw(Graphics g)
 {
 g.DrawRectangle(Pen, Rectangle);
 }
 }
 public class SpeedDown : SpeedBox
 {
 Pen Pen = new Pen(Color.Blue);
 public SpeedDown(int x, int y, int width, int height) : base(x, y, width, height)
 {
 }
 public override PointF ChangeSpeed(PointF speed)
 {
 speed.X *= 0.98f;
 speed.Y *= 0.98f;
 return speed;
 }
 public override void Draw(Graphics g)
 {
 g.DrawRectangle(Pen, Rectangle);
 }
 }
```

• Jackiie: @PeterCsala, I tried to check beforehand if this sort of question is "bad" to put here, but it looked like this would be the right place as function works, but OOP standard may be so-so. I wasn't sure whether or not I would post this here. Is my questions up to standard for CodeReview.stackexchange or is it too broad/lazy? Thank you!
• Jackiie: @PeterCsala I missed to tag you first time (and now I can't see that comment?), but it's in WinForms!
• radarbob: why aren't you using PointF methods & operators?, otherwise it feels like instantiating PointF objects is pointless.
• Jackiie: @radarbob Mostly because I didn't know they existed, thank you!

Answer 1

Welcome to Code Review. You claim your code runs fine, and I will take your word at that since I don't have time to try to load and run it. Since you (1) have working code and (2) want a code review with constructive advice, then I think your post is acceptable here.

It might help to know which version of .NET you are using: .NET 7, .NET 6, .NET Framework, etc.

The Good

Your code is easy enough to read. Decent indenting and other white spacing. Names are spelled out without cryptic abbreviations.

I do take exception to a variable named Speed that is a PointF or location. I would expect speed to be a float with some factor. 1.0f is normal starting speed. And you increase ( Speed *= 1.02 ) or decrease ( Speed *= 0.98 ) depending upon some play action such as colliding with a speed box.

I like that your Main is minimal. This probably reflects your experiences with other languages. Many newbies to coding in general will try to shove as much as they can into the Program class.

Opportunites

Yes, you could benefit from better OOP and to adhering to some C# principles.

Ball Class

Consider the Ball class that has Position, Speed, Radius, etc. These are defined as fields. I use the principle that if something is private or constant that I make it a field. Otherwise, I make it a Property where at the very least the getter is public. Example:

public float Radius { get; private set; }

And that would be only if you allow Radius to be changed during a ball's lifetime. If you expect it to never change, you would want it to be readonly, as:

public float Radius { get; }

Although it looks like Position and Speed can definitely change, in which case you want to keep the private setter.

I see Pen is a field. Since Pen doesn't seem to change value, it could be static. However, in the future you may desire to have a ball change color, in which case I would make Pen a public property. Whether the setter is private or not depends upon your uses.

SpeedBox stuff

My problems with your implementation is (1) that it is not OOP-ish. There are 2 methods there, both un-related to each other, and the ChangeSpeed method has ZERO interaction with any of a speed boxes fields/properties.

The ball class checks for collisions, and if a collision is detected the speed is changed. I think ChangeSpeed should be moved to the Ball class. That leaves SpeedBox as minimalist with nothing more than a size, location, pen, and a Draw method. And that's all SpeedBox should be. It's an object at some location. Leave it to other classes to determine if they have collided with that location as well as what should be done in response to the collision. During all that, the SpeedBox occupies a location and is totally oblivious to a collision with some other object as well as what that colliding object was. And after the collison, the SpeedBox is unchanged.

• Jackiie: Thanks, that helped! I've change my code fairly substantially now, and will look it over a few times more. For the part "I do take exception to a variable named Speed that is a PointF or location. I would expect speed to be a float with some factor", I understand that you think I should change it to a float. Not sure how that would work, as the X+Y speed together gives a vector. Or do you just mean two floats?