Skip to main content
deleted 2 characters in body
Source Link
radarbob
  • 8.2k
  • 21
  • 35
    Board snakeBoardsnakeMaze = new Board(...);
    snakeBoardsnakeMaze.Draw();
    
    Board snakeBoard = new Board(...);
    snakeBoard.Draw();
    
    Board snakeMaze = new Board(...);
    snakeMaze.Draw();
    
added 314 characters in body
Source Link
radarbob
  • 8.2k
  • 21
  • 35
Abstracting - It's Turtles All the Way Down

Below requires reading the loop code and often reading deeper to make sure I understand what is "true"ly going on:

while (true) {
   // ....
}

All that effort goes away when:

while (! isBodyCollision) {
   // ....
}
Abstracting - It's Turtles All the Way Down

Below requires reading the loop code and often reading deeper to make sure I understand what is "true"ly going on:

while (true) {
   // ....
}

All that effort goes away when:

while (! isBodyCollision) {
   // ....
}
Source Link
radarbob
  • 8.2k
  • 21
  • 35

Program.cs

Well done! Rare in the extreme to see good OO with appropriate abstraction level, and decoupling the application from the environment entry point.


No public setters

public class Game
{
    public Board Board { get; protected set; }
    public Snake Snake { get; protected set; }
    public Apple Apple { get;  protected set; }
    public Score Score { get;  protected set; }
    

A good class exposes functionality and hides state. The way the code is already written you doesn't need them. NEVER publicly expose internal state.


Not this:

       if (Position[0].y - 1 == 0)
                {
                    return true;
                }
                return false;
                

This:

    return (Position[0].y - 1 == 0);
    

also the operator overload methods:

       return (p1.x == p2.x && p1.y == p2.y)
       

Easy Complexity Reduction Technique

Return immediately on terminating conditions. Use the ternary operator. This reduces technical complexity scores because those are mainly about the total possible branches.

private Direction UpdateCurrentDirection(Direction newDirection)
{
    if (newDirection == Direction.previousDirection) return currentDirection;
        
    if (Position.Count == 1)  return newDirection;

    return CheckIfOppositeDirection(newDirection)) ? 
              currentDirection : newDirection;
}

One line if w/out brackets super-sizes comprehension speed. The secret to getting away with that is to leave white space above & below.

BEFORE:

private Direction UpdateCurrentDirection(Direction newDirection)
{
    if (newDirection != Direction.previousDirection)
    {
        if (Position.Count == 1)
        { 
            return newDirection;
        }
        if (CheckIfOppositeDirection(newDirection) == true)
        {
            return currentDirection;
        }
        else
        {
            return newDirection;
        }
    }
    else
    {
        return currentDirection;
    }
}

as long as true is true this truthy true is true

 public void Start()
    {
        // ...
         while (true) {
             // ...
             bool isBodyCollision = Snake.Move(direction, Board.Width, Board.Height);
            if (isBodyCollision) break;
           // ...
        }

Name the rules controlling game play. Move isBodyCollision declaration out of the loop.

      bool isBodyCollision = false
       while (! isBodyCollision) {
             // ...
            isBodyCollision = Snake.Move(direction, Board.Width, Board.Height);
            if (isBodyCollision) continue;
           // ...
        }
 

Let the loop conditional control execution. It tends to help prevent temperal coupling as code changes and complexity increases.


Leaky Abstraction

I like the variable name "Position"

public class Snake {  ...  Position = new List<Point>();  ... }

Playing the game we talk about the Snake's position, not a collection of positions. But then the abstraction leaks its implementation somewhat with a method name:

UpdateSnakePositionList

Change that to

UpdateSnakePosition

Each layer of code structure, method, and class is an opportunity for abstraction into actual Snake Game terminology, concepts, and game play. As a general rule use names and verbs abstracting away, not hanging onto, actual code implementaiton.


Class name gives context to methods

This: Draw Not: DrawBoard

Concise reading enjoyment:

    Board snakeBoard = new Board(...);
    snakeBoard.Draw();
    

switch case & fall-through formatting

Always put space between a case break/return and the next case. Fall-through cases in particular stand out much better.

       case ConsoleKey.D:
       case ConsoleKey.RightArrow:
             return Direction.right;
        
        case ConsoleKey.S:
        case ConsoleKey.DownArrow:
              return Direction.down;
                
        case ConsoleKey.A:
        

Check 'check' in method names

"Check" is such a generic term that it means nothing. But I'll admit I can't help myself sometimes. Try very hard to replace "check" with something else. It is easier to fix when returning boolean, prefix with "is"

  • CheckIfEaten - IsEaten
  • CheckIfOppositeDirection - ChangedDirection
  • CheckIfCollisionWithBoundary - IsBoundryCollision

Concise reading enjoyment:

if ( Apple.IsEaten())
if (ChangedDirection())

Get is a given

Methods like this: GetSnakeHead(). After about 7,602 GET-prefixed methods, you will realize it is superfluous. SnakeHead() is better. Make it a property - Snake.Head is better still.

Concise reading enjoyment:

  if (Apple.IsEaten(Snake.Head))
  

Uh, oh. I now see a problem. Not kidding - I thought of it only after I wrote that. The code feels "backwards" and I think "inversion of control"


Inversion of Control (IOC)

 public class Start {
     .....
       if (Apple.CheckIfEaten(Snake.GetSnakeHead())) 
     ....
} // Start

The Apple should not be responsible for determining if the snake ate it. That's for the snake to do. Due to the "inverted" (backward, inside-out) referencing (dependencies) the Start class must know Snake internal details, head vs tail vs otherParts. Apple also, for it's part.

In other words Start must know how the snake eats because Snake does not know how to eat - it has no eating behavior (method). If you have ever experienced a waiter suddenly shoving any part of your body into a plate of food, then you understand the issue here.

Concise reading enjoyment:

    if (Snake.Eats(Apple)) 

Decouple the game from the UI

There should be a new class that makes console calls for UI interaction. There will be no console code anywhere in the Snake Game classes. Then make Game class methods/properties for score, board, etc. The UI class talks to Game object and no others. This is the principle of least knowledge.

Suggestion: Board.ToString() returns the board/grid as a string with/n separating rows. Same thing for Score.ToString() and any other UI bound output.

Then Game has the interface used by UI objects.

public class Game {
    public string Score() { return Score.ToString(); }
    public string Board() { return Board.ToString(); } 
    public override string  ToString() {  return Score() + `/n/n` +  Board();   }  
}

Concise reading enjoyment:

 console.WriteLine(mySnakeGame);     // implicit ToString call