Simple Ellipse Collision Mini Game

Question

I am doing an exercise to help me understand C#and OOP. The exercise is supposed to teach me about inheritance, interfaces and abstract classes. This is done by some small game "Bounce".

The game is about several moving balls and several stationary objects that can affect the moving balls.

I am posting my solution as I don't have anyone else knowledgeable to review my code and give me feedback.

The game functions :

There are 2 types of lines and 2 types of boxes.

Vertical lines reflect the ball back by changing their X-speed. Horizontal lines reflect it by changing the Y-speed.

The red boxes speed up the balls, while the blue boxes speed down the balls.

They both allow for the ball to pass through. The direction of the ball does not change while moving through them

The Requirements :

All obstacles (squares and lines) should be treated the same w.r.t. the engine; perhaps by using an interface. Do not use fully abstract classes!

All obstacles should share as much code as possible, but only code that makes sense to share; use abstract classes if necessary to avoid inheriting methods that do not make sense or empty methods.

---

What I am trying to get out of the review

Main questions

• Is the solution good enough to be classified as an OOP solution? If not what can I improve to achieve that?

• Is my implementation of the interface done as the requirements stated and also is the solution as a whole satisfying the requirements placed on the exercise?

Small stuff :

• The collision detection of the line with the ellipse is implemented from this questions Circle line-segment collision detection algorithm? answer. I don't know if my implementation is missing a part, but it is working as I want it.

• The speed down of the balls is not get implemented as of now it just moves the ball. I don't know how I would slow them down.

• Lastly I am not sure if Collision and it parts should be its own class. My thought process was that the engine should handle that. Still not happy about how it looks in the code any tips on improving it would be appreciated.

---

Code :

The Position, Vector, Ball and Engine classes where already given to me from the book. Any changes to them have been commented. Longer snippet of code have //myCode to distinct and make it clear what is mine and what is not.

Position :

public class Position
{
    public float X, Y;

    public Position(float x, float y)
    {
        X = x; Y = y;
    }
}

Vector :

public class Vector
{
    public float X, Y;

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

Ball :

public class Ball
{
    static Pen Pen = new Pen(Color.White);

    public Position Position { get; } //originally  Position Position;
    public Vector Speed { get; } //originally  Vector Speed;

    public float Radius { get; } //originally  float Radius;

    static Random Random = new Random();

    public Ball(float x, float y, float radius)
    {
        Position = new Position(x, y);
        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 Vector(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;
    }

}

Engine :

public class Engine
{
    MainForm Form = new MainForm();
    Timer Timer = new Timer();
    List<Ball> Balls = new List<Ball>();
    //myCode
    List<IObstacles> RBox = new List<IObstacles>();
    List<IObstacles> BBox = new List<IObstacles>();
    List<IObstacles> VLine = new List<IObstacles>();
    List<IObstacles> HLine = new List<IObstacles>();
    //myCode
    Random Random = new Random();

    public void Run()
    {
        Form.Paint += Draw;
        Timer.Tick += TimerEventHandler;
        Timer.Interval = 1000 / 25;
        Timer.Start();

        Application.Run(Form);
    }

    private void Form_Paint(object sender, PaintEventArgs e)
    {
        throw new NotImplementedException();
    }

    void TimerEventHandler(Object obj, EventArgs args)
    {

        if (Random.Next(100) < 25)
        {
            var ball = new Ball(400, 300, 10);
            Balls.Add(ball);
        }

        //myCode
        if (RBox.Count() < 1)
        {
            RBox.Add(new Redbox(100, 100));
        }

        if (BBox.Count() < 1)
        {
            BBox.Add(new Bluebox(500, 200));
        }

        if (VLine.Count() < 1)
        {

            VLine.Add(new VerticaLine(700, 300));
        }

        if (HLine.Count() < 1)
        {

            HLine.Add(new HorizonaLine(100, 500));
        }
        //myCode

        foreach (var ball in Balls)
        {
            ball.Move();
        }

        //myCode
        Collision();
        //myCode
        Form.Refresh();
    }
    //myCode
    float Clamp(float value, float min, float max)
    {
        if (min >= value) return min;
        else if (max <= value) return max;
        else return value;
    }

    void CollisionBoxes(List<Ball> balls, List<IObstacles> obstacle)
    {
        foreach (var ball in balls)
        {
            foreach (var item in obstacle)
            {
                float closestX = Clamp(ball.Position.X, item.Position.X, (item.Position.X + item.Width));
                float closestY = Clamp(ball.Position.Y, item.Position.Y, (item.Position.Y + item.Height));

                float distanceX = ball.Position.X - closestX;
                float distanceY = ball.Position.Y - closestY;

                float distanceSquared = (distanceX * distanceX) + (distanceY * distanceY);

                if (distanceSquared < (ball.Radius * ball.Radius) && item.Pen.Color == System.Drawing.Color.Red)
                {

                    ball.Move();
                }
                else if (distanceSquared < (ball.Radius * ball.Radius) && item.Pen.Color == System.Drawing.Color.Blue)
                {

                    ball.Move();//dont know how to spueed the ball down

                }
            }
        }
    }
    void CollisionLines(List<Ball> balls, List<IObstacles> obstacles)
    {

        foreach (var ball in balls)
        {
            foreach (var item in obstacles)
            {
                float dx = item.Width - item.Position.X;
                float dy = item.Height - item.Position.Y;

                float a = dx * dx + dy * dy;
                float b = 2 * (dx * (item.Position.X - ball.Position.X) + dy * (item.Position.Y - ball.Position.Y));
                float c = (item.Position.X - ball.Position.X) * (item.Position.X - ball.Position.X) + (item.Position.Y - ball.Position.Y) * (item.Position.Y - ball.Position.Y) - ball.Radius * ball.Radius;

                float discriminant = b * b - 4 * a * c;

                if (discriminant < 0)
                {
                }
                else
                {
                    discriminant = (float)Math.Sqrt(discriminant);

                    float t1 = (-b - discriminant) / (2 * a);
                    float t2 = (-b + discriminant) / (2 * a);

                    if ((t1 >= 0 && t1 <= 1) && item.Pen.Color == System.Drawing.Color.Yellow)
                    {
                        ball.Speed.X -= 1;
                    }
                    else if ((t1 >= 0 && t1 <= 1) && item.Pen.Color == System.Drawing.Color.Green)
                    {
                        ball.Speed.Y -= 1;
                    }

                    if ((t2 >= 0 && t2 <= 1) && item.Pen.Color == System.Drawing.Color.Yellow)
                    {

                        ball.Speed.X -= 1;
                    }
                    else if ((t2 >= 0 && t2 <= 1) && item.Pen.Color == System.Drawing.Color.Green)
                    {
                        ball.Speed.Y -= 1;
                    }
                }
            }
        }
    }

    void Collision()
    {
        CollisionBoxes(Balls, RBox);
        CollisionBoxes(Balls, BBox);
        CollisionLines(Balls, VLine);
        CollisionLines(Balls, HLine);

    }
    //myCode

    void Draw(Object obj, PaintEventArgs args)
    {
        foreach (var ball in Balls)
        {
            ball.Draw(args.Graphics);
        }

        //myCode
        foreach (var rbox in RBox)
        {
            rbox.Draw(args.Graphics);
        }

        foreach (var bbox in BBox)
        {
            bbox.Draw(args.Graphics);
        }

        foreach (var vline in VLine)
        {
            vline.Draw(args.Graphics);
        }

        foreach (var hline in HLine)
        {
            hline.Draw(args.Graphics);
        }
        //myCode

    }

}

IObstacles :

interface IObstacles
{
    void Draw(Graphics g);

    Pen Pen { get; }
    Position Position { get; }
    float Width { get; }
    float Height { get; }

}

class Redbox : IObstacles
{
    public Pen Pen { get; }

    Random Random = new Random();

    public Position Position { get; }
    public float Width { get; }
    public float Height { get; }

    public Redbox(float x, float y)
    {
        Position = new Position(x, y);
        Pen = new Pen(Color.Red);

        Width = 150;
        Height = 150;

    }


    public void Draw(Graphics g)
    {
        g.DrawRectangle(Pen, Position.X, Position.Y, Width, Height);
    }
}

class Bluebox : IObstacles
{
    public Pen Pen { get; }

    Random Random = new Random();

    public Position Position { get; }
    public float Width { get; }
    public float Height { get; }

    public Bluebox(float x, float y)
    {
        Position = new Position(x, y);
        Pen = new Pen(Color.Blue);

        Width = 125;
        Height = 125;
    }

    public void Draw(Graphics g)
    {
        g.DrawRectangle(Pen, Position.X, Position.Y, Width, Height);
    }

}

class VerticaLine : IObstacles
{
    public Pen Pen { get; }

    public Position Position { get; }
    public float Width { get; }
    public float Height { get; }


    public VerticaLine(float x, float y)
    {
        Pen = new Pen(Color.Yellow);
        Position = new Position(x, y);
        Height = 100;
        Width = x;
    }

    public void Draw(Graphics g)
    {
        g.DrawLine(Pen, Position.X, Position.Y, Width, Height);
    }
}

class HorizonaLine : IObstacles
{
    public Pen Pen { get; }

    public Position Position { get; }
    public float Width { get; }
    public float Height { get; }


    public HorizonaLine(float x, float y)
    {
        Pen = new Pen(Color.Green);
        Position = new Position(x, y);
        Width = 600;
        Height = y;
    }

    public void Draw(Graphics g)
    {
        g.DrawLine(Pen, Position.X, Position.Y, Width, Height);
    }
}

---

I appreciate any feedback small or big and thanks again for reviewing my bad code.

Answer 1

Diagnosis

The code destroys the concept of an "obstacle" and the classes are incoherent. An obstacle only draws. The Engine (not an obstacle), is colliding, not the designated obstacles Boxes and Lines. Subclass purpose is to breed subclasses because box color and line orientation is a distinction without a (subclass) difference.

The code is truly anti-matter to the Open/Close principle. To add more obstacle classes means altering code everywhere.

I see all of that caused, ultimately, by avoiding necessary type casting.

---

Abstrace Class instead of Interface?

By the way, if you want to keep and expose separate lists for each obstacle-type, then all this IObstacle stuff is unnecessary abstraction overkill. Then you might as well go with an abstract class and no interface at all. Implementing "Collide" is enough.

P.S. "...no interface at all" - Here I mean the C# keyword interface, a language feature. The public members of an abstract class or any concrete class is by definition its interface. The maxim "code to interface not implementation" is not violated just because my code does not have a C#-keyword-interface thing.

---

---

Solution

• The interface should declare "collision", not "draw".
• Don't subclass obstacle classes
• Create a custom collection. Implement Count, HasMinCount, etc.
  • There be type casting in here!
  • Create an enum of all obstacle types. Iterate the enum to count all objects in the collection by type

What is an IObstacles ?

The name should be singular - IObstacle - not plural.

An Obstacle should be about colliding and not at all about drawing.

---

Don't Subclass Boxes and Lines

Classes and subclasses are distinguished by behavior. Objects are distinguished by property values.

Box, BlueBox, RedBox all have the same behavior. They differ only by the value in the Color property. Don't subclass. The same idea applies to Line classes.

Subclassing here is not necessarily wrong. But when is a subclass for every color, line orientation, etc. too many subclasses?

---

Boxes and Lines collide, Engines don't

Give Box and Line their rightful collision methods because you obviously intend these to be Obstacles, not the Engine.

---

Obstacles Custom Collection Class

Keeping type casting confined to this collection class keeps Engine and other client code referencing only Obstacles, not the objects' types; thus addressing the Open/Close issue. The enum also helps this issue when used as a method parameter as in Count().

It is going to need this:

public enum Obstacles {
  Box, BlueBox, RedBox, FoxInBox, Line, VertLine, HorizLine
}

see Iterate the enum

This is a good place for collection oriented tasks.

class Obstacles 
{
 // do not inherit List - read MSDN docs
  protected List<IObstacle> Obstacles  { get; set; }

  // if we want to silently ignore a non-addition then 
  // return void instead
  public bool Add(IObstacle anObstacle) {
     if( anObstacle == null) return false;

     Obstacles.Add(anObstacle);
     return true;
  }

  public int Count(Obstacles obsticleType) { ... }

  public bool HasMinimum(Obstacles obstacleType, int atLeastThisMany ) { ... }
  
}