4
\$\begingroup\$

I was asked to give a high level design of Pacman. Please review the OOP design, this is not a full implementation. only the design is important here.

Please comment if the design is missing anything. or can be improved? The main problem I think is to keep Single responsibility, I am having a hard time here not have all the parts of the game knowing each other.

namespace PacmacOopDesign
{
    class Program
    {
        static void Main(string[] args)
        {
            //we need to init the maze in the board
            Board board = new Board();

            BaseFigure Pacman = new Pacman(board.GetEmptyPosition());
            BaseFigure RedGhost = new Ghost(board.GetEmptyPosition(), System.Drawing.Color.FromName("Red"));
            //the logic for the entire game is here is kind of a loop which check many things in each turn.

            List<BaseFigure> figures = new List<BaseFigure>();
            figures.Add(Pacman);
            figures.Add(RedGhost);
            Game game = new Game(figures,board);
        }
    }
} 

My first issue is about how do I init the whole game play? do I create all the figures, and the maze and then let the game place them? or do I need to have it pre-defined?

BaseFigure.cs

    public abstract class BaseFigure
    {
        protected Position _position;
        protected System.Drawing.Color _color;
        //init the base position of the figure
        public BaseFigure(Position position,  Color color)
        {
            _position = position;
            _color = color;
        }

        public abstract void DrawShape();

        //if the ghost is moving toward the same position the player is moving game is ended
        // if pacman collected all the dots and fruits game is ended
    }

Board.cs

    public class Board
    {
        public int Level { get; set; }
        public int Width { get; set; }
        public int Height { get; set; }

        //the number of dots to eat in order of winning this specific Board
        public int NumOfDotsToEat { get; set; }

        //this is a 2d maze
        // 0 means wall
        // 1 - empty spot for player/computer figures
        // 2 means a dot will be placed
        // 2-10 fruits/ special 
        public Position[] Maze { get; set; }
        public Position GetEmptyPosition()
        {
            //get a random/ valid predefined position for a new figure on the board
            return new Position(0, 0);
        }

        public Board()
        {

        }
    }

Game.cs

public class Game
    {
        private bool _isWinCondition;
        private List<BaseFigure> _figures;
        private Board _board;
        public Game(List<BaseFigure> figures, Board board)
        {
            _isWinCondition = false;
            _figures = figures;
            _board = board;
        }

        public void GameStart()
        {
            while (!_isWinCondition)
            {
                foreach (var figure in _figures)
                {
                    GetNewLocation(figure);
                    figure.DrawShape();
                }
            }
        }

        private void GetNewLocation(BaseFigure figure)
        {
            //if figure is pacman check that user input is valid.
            //remove the dot if it has dot, and remove from total in Boards.
            //check if isWinCondition
            //if next frame has fruit in it, change to killing ghosts ability and speed up move.


            //if figure is Ghost check how close to pacman you are is less then minimal of the ghost
            //change the strategy of the ghost of chasing pacman, if more continue with old strategy.
            //if new location next to pacman when he ate fruit remove ghost from board
            //if new location next to pacman the game is ended.
        }
    }

Game class is the main problem I have, should it really handle all the logic and not each class/Figure handle her own strategy of moving?

Ghost.cs

 public class Ghost : BaseFigure
    {
        private int _minimalDistance;
        public Ghost(Position position, Color color) : base(position, color)
        {
            _minimalDistance = 5;
        }
        public override void DrawShape()
        {

        }

        public void LockOnFigure()
        {
            //if in minimal distance from other figure start to chase it
        }
    }

for example the ghost can have it's own strategy once it reaches a cetrain threshold to go after pacman, the problem is that it needs to be aware how does the board look and where Pacman is...

so the other option is for the Game to handle the strategy and tell the ghost where to go and which strategy to use.

What do you think?

Pacman.cs

public class Pacman : BaseFigure
    {
        public Pacman(Position position)
            : base(position, System.Drawing.Color.FromName("yellow"))
        {
            _position.Value = 2;
        }
        public override void DrawShape()
        {
            // draw the pacman shape
        }
    }

Position.cs

public class Position
    {
        public int X { get; set; }
        public int Y { get; set; }
        public int Value { get; set; }
        public Position(int x, int y)
        {
            X = x;
            Y = y;
        }
    }
\$\endgroup\$
  • \$\begingroup\$ this is not a full implementation does this mean that you didn't implement it at all or just strippted the code for the sake of the review? \$\endgroup\$ – t3chb0t Jul 14 '17 at 15:41
  • \$\begingroup\$ I stripped part of it. Since the interview was only 30 minutes. I had to do only UML level white board design. I did this code to show my design in conecpt. \$\endgroup\$ – Gilad Jul 14 '17 at 17:31
5
\$\begingroup\$

Quick-fire comments:

  • You have no abstraction to decouple your UI, your game state and your game logic.
  • Your Ghost class is responsible for both the state of the pawn on the board, and the AI that drives it. That is a massive conflict of responsibilities. This also ties in to a lack of decoupling between UI, game state, and game logic.
  • You seem to mix between game/board/level without any clear distinction.
  • Your class definition implies that a Board has a level (= number). It would be better to invert this: a Level (object) should have a specific Board (the level "map", if you will).
  • Having the Pacman class set its own position is not a good design. Pacman should be told (by the game) what its position is, instead of deciding it for himself.
  • Pedantic: Going by the rules of Pacman, Board doesn't need NumOfDotsToEat. The object of the game is to eat all the dots on the board, not just a given number of them.
  • Pedantic: You've tied the AI behavior to the Ghost class. But one of the traits of the ghosts is that they each have a unique AI. One favors going left, another favors going right, etc. But even if the AI was the same for all ghosts, you still should have separated the AI from the ghost's pawn.

In-depth review:

public void GameStart()
{
    while (!_isWinCondition)
    {
        foreach (var figure in _figures)
        {
            GetNewLocation(figure);
            figure.DrawShape();
        }
    }
}

I don't like the design of the core of the gameplay. It doesn't sit well with me. This feels to me like you're putting the cart before the horse. Your algorithm seems to assume that the state of the game is derived from the position of the figures.
But it should be the other way around: the position of the figures should be derived from (and visualizing) the game state.

    private void GetNewLocation(BaseFigure figure)
    {
        //if figure is pacman check that user input is valid.
        //remove the dot if it has dot, and remove from total in Boards.
        //check if isWinCondition
        //if next frame has fruit in it, change to killing ghosts ability and speed up move.


        //if figure is Ghost check how close to pacman you are is less then minimal of the ghost
        //change the strategy of the ghost of chasing pacman, if more continue with old strategy.
        //if new location next to pacman when he ate fruit remove ghost from board
        //if new location next to pacman the game is ended.
    }

You've basically resigned yourself to putting all of the game's logic into a single method. Maybe you would've noticed it if you had needed to write the code that needs to go in this method.
Edit: I don't mean to come across as harsh here. Sorry if it sounds that way. This method is merely the culmination of minor design flaws in other locations. By not abstracting the game logic sufficiently, you've essentially bundled all the complex logic in a single method, as opposed to spreading it out over your classes (which is the main goal of an OOP approach).

Just look at the responsibilities of this method:

  • Validating user input.
  • Changing PacMan's state based on user input (or lack thereof).
  • Changing Ghost states (killing, or getting killed by, Pacman)
  • Updating the Ghost AI based on the current state of the board and its pawns.
  • Changing the Board state (eating dots and fruit)
  • Validating and changing the Game state (Pacman's fruit power up, victory conditions, defeat conditions)

This is >90% of the game's logic. In a single method. The only things that are not in this method are the level composition, pawn positions, and drawing the image (which isn't even game logic, it's UI logic).


Suggested improvements:

Since the interview was only 30 minutes. I had to do only UML level white board design. I did this code to show my design in concept.

Note that I'm keeping your constraints in mind. I'm not going to delve into coding style or specific implementations. I'm only focusing on the overall approach to abstracting logic and responsibilities into OOP classes, and how to makes these objects communicate effectively.

Game

This is the main class of your game. It should focus on everything that is outside of the gameplay, e.g. the user menu, settings, level selection, ...

These should be subdivided into further classes, but I'm omitting this part as it's not as relevant as the core game logic.

The most important thing to take away here is the Game's responsibilities:

  • Rendering the graphics (best to do this in a subclass, but for simplicity's sake I'm omitting the specific UI implementation and imagining a functional myGame.DrawState(myLevel) method that draws all the needed graphics).
  • Creating a Level and starting it. The actual gameplay will be handled inside the Level. When an active level triggers a victory event, the Game creates the next Level and starts it.
  • We only store game-specific data (time played, score achieved, player name, player lives, ...)

Level

This is the bread and butter of our gameplay. A small overview of the data that we will store in separate properties:

  • The Board
  • The Pawns (List<Pawn>). We can also create specific properties that reference Pacman directly, but these properties would be a reference to the same object (in-memory) that is referenced in this list.
  • A reference back to its Game.

Note: during level composition, we will also add the pawns to the Board itself. We only use the myLevel.Pawns list for easy access to the game objects in order to handle events relating to them, e.g. when telling the board to move a pawn, we can include a reference to the pawn we want to move.

But the meat of this class will be in its logic. Its responsibilities include:

  • Setting the initial level state (setting pawn starting locations, setting dot/fruit nodes, ...)
  • Once started, processing the gameplay frame-by-frame. Each frame does the following:
    • Check for a victory/defeat condition.
    • If the user is pressing a directional key, Pacman's direction is updated accordingly.
    • Pacman automatically moves one square in the direction he is facing.
    • Each ghost moves one square. Their AI freely decides the direction based on the available game state.
  • Resolving interactions between game elements:
    • Collisions between pawns (who kills who?)
    • Collisions between pawns and board (eating food, activating fruit powerup)
    • Handling player death (resetting Pacman or ending the game)
    • Handling ghost death (resetting their position)
  • Returning to the Game with a Victory/Defeat return value.

Note that only the top-level logic will be contained in the Level class (e.g. allowing a pawn to make a certain move). The specific implementation (e.g. updating the actual positional value of a pawn) is handled by the relevant subclasses.

Board

The board is only concerned with the position of the game objects, and the board layout. The board class does not care about deciding which move to make, pawn interactions, user input, or validating win conditions.

Data stored:

  • A list of pawns (including their position).
  • A layout of the walls
  • A list of dots and fruits
  • A reference back to its Level.

Logic responsibilities:

  • Calculating the initial position of the pawns (including respawn positions for pacman and the ghosts) and populating the board with dots and fruit.
  • Moving a specific pawn in a specific direction. This includes validating if the move is legal (e.g. if Pacman is pushing against a wall, the "move" should not actually update the position since Pacman will stay in place).
  • Alerting the Level of collisions that occur. The board will raise an event which contains the colliding elements (i.e. Pacman+Ghost, Pacman+Fruit, Pacman+Dot). The board does nothing other than notify the Level.
  • Publically exposing the amount of dots that are left on the board.

Pawn

The base class from which Pacman and the ghosts derive. Contains all shared data properties:

  • Position of the pawn

Notice that this class (and any derived classes) implement no game logic.

Pacman

Further extends the pawn:

  • Direction of travel

Ghost

Further extends the pawn:

  • Color of the ghost
  • Reference to the AI object that controls this specific ghost.

AI

This class will contain a lot of logic which I am skipping here. You will most likely want to inherit this class into multiple derived classes, one for every type of AI behavior (Inky, Pinky, Blinky and Clyde all have unique behaviors).

Mainly, it will receive a reference to its Ghost and the Board. Based on the AI's configured behavior, it will decided which direction to move the ghost in, based on the current state of the board.


Notice that the above listed classes contain information, not graphics. The graphics should be handled by the engine. The engine should decide what graphics to use based on the game state.

Essentially, every time we wish to draw a frame, we pass the entire Level, including all contained objects, to the game engine. It will then look at every object (pawns, board layout, ...) and use the available data (position, is pacman powerup active, which dots/fruits are still on the board, ...) to render the image.


Tangential comments:

  • If you want a pause menu, that should be implemented on the Game level. This will also require the Game to be able to pause the Level (i.e. it stops processing frames while paused).
  • I've omitted things like level difficulty. This needs to be a property on the Level class, which is set by the Game class.
  • I'm contemplating removing the position from the pawn, and instead connecting it to the pawn inside the Board class. That way, the Level doesn't actually have access to the pawn's positions, only the board does. The level shouldn't care about the positions, as it is the board's reponsibility to track positions and raise collision events. At no point should the Level ever make a decision based on a positional value!
  • Whenever Pacman dies, the Level should try to take a life from the counter (in Game). If that fails (because there are no more lives left), the Level should register a Defeat condition.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.