3
\$\begingroup\$

Recently, I had a sick leave and to had some fun and improve coding skills I got a challenge to write a Snake Game for WPF.

Desired RESULT

1) Game must be in separate .dll library;

2) Must have settings;

3) Must use Canvas as playing field.

SOURCE

If you want to skip reading or just to watch and to make review - you're welcome here.

HOW TO PLAY

enter image description here

If you dowload source there would be additional Escape key to Close Main window

USAGE

This library must be used as you might expect. Just add library to your wpf-project, and instantiate SnakeGame class with input parameters:

public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
        Loaded += SnakeGameLoader;
        KeyDown += new KeyEventHandler(CloseOnEsc);
    }

    private void SnakeGameLoader(object sender, RoutedEventArgs e)
    {
        var snakeGame = new SnakeGame
        {
            Snake = new Snake(
                length: 3,
                startPosition: StartPosition.Center,
                bodyColor: Brushes.Red,
                headColor: Brushes.Black),
            GameField = new GameField(
                pixelSize: 50,
                playingField: playingField,
                pixelType: PixelType.Square),
            Difficulty = Difficulty.Hard,
            FoodColor = Brushes.GreenYellow,
            AmountOfFood = 6
        };
        snakeGame.Start();
    }

    private void CloseOnEsc(object sender, KeyEventArgs e)
    {
        if (e.Key == Key.Escape) Close();
    }
}

SnakeGame.cs

public class SnakeGame
{
    #region Properties

    public Snake Snake { get; set; }

    public GameField GameField { get; set; }

    public Difficulty Difficulty { get; set; }

    public int AmountOfFood { get; set; }

    public SolidColorBrush FoodColor { get; set; }

    #endregion

    #region Public methods

    public void Start()
    {
        if (GameField == null)
        {
            throw new ArgumentNullException(nameof(GameField));
        }

        if (Snake == null)
        {
            throw new ArgumentNullException(nameof(Snake));
        }

        if (Difficulty < Difficulty.Impossible || Difficulty > Difficulty.Easy)
        {
            throw new ArgumentException($"{nameof(Difficulty)} illegal enum value");
        }

        Snake.InitializeSnake(GameField, Difficulty);
        Snake.OnLackOfFood += InitializeFood;
        Snake.OnSnakeCrash += GameOver;
        Snake.StartMoving();
    }

    #endregion

    #region Private methods

    private void InitializeFood(object sender, EventArgs e)
    {
        if (FoodColor == null)
        {
            throw new ArgumentNullException(nameof(FoodColor));
        }
        if (AmountOfFood <= 0)
        {
            throw new ArgumentOutOfRangeException($"{nameof(AmountOfFood)} must be positive");
        }
        var emptyPixels = GameField.GetPixels().Where(s => s.GetFillColor == null).ToArray();
        var random = new Random();
        int foodCount = 0;
        do
        {
            var randomIndex = random.Next(0, emptyPixels.Length);
            emptyPixels[randomIndex].Fill(FoodColor);
            emptyPixels[randomIndex].IsFood = true;
        }
        while (++foodCount < AmountOfFood);
    }

    private void GameOver(object sender, EventArgs e)
    {
        GameField.Clear();
        GameField.ResultBoard(Snake.MovesCount, Snake.ScoreCount);
        Snake.OnLackOfFood -= InitializeFood;
    }

    #endregion
}

GameField.cs

Actually GameField is a Matrix[M,N] where M - amount of Rows, N - amount of Columns.

public class GameField
{
    #region Fields

    private readonly Pixel[,] _pixels;

    #endregion

    #region Ctor

    /// <summary>
    /// Initializes a new instance of GameField
    /// </summary>
    /// <param name="pixelSize">Size of squares in pixels (must be between 10 and 100)</param>
    /// <param name="playingField">Playing field</param>
    /// <param name="pixelType">Pixels type</param>
    public GameField(int pixelSize, Canvas playingField, PixelType pixelType)
    {
        if (pixelType < PixelType.Circle || pixelType > PixelType.Square)
        {
            throw new ArgumentException($"{nameof(pixelType)} illegal enum value");
        }
        Pixel.PixelType = pixelType;

        if (pixelSize < 10 || pixelSize > 100)
        {
            throw new ArgumentOutOfRangeException($"{nameof(pixelSize)} must be between '5' and '100'");
        }
        Pixel.Canvas = playingField ?? throw new ArgumentNullException(nameof(playingField));
        Pixel.Size = pixelSize;

        Rows = (int)Math.Floor(playingField.ActualHeight / pixelSize);
        Columns = (int)Math.Floor(playingField.ActualWidth / pixelSize);

        Pixel.Corrective = Tuple.Create(
            playingField.ActualHeight % pixelSize / Rows,
            playingField.ActualWidth % pixelSize / Columns);

        _pixels = new Pixel[Rows, Columns];
        for (int i = 0; i < Rows; i++)
        {
            for (int j = 0; j < Columns; j++)
            {
                _pixels[i, j] = new Pixel(i, j);
            }
        }
    }

    #endregion

    #region Public methods

    public void ResultBoard(int movesCount, double scoreCount)
    {
        var grid = new Grid
        {
            Height = Pixel.Canvas.ActualHeight,
            Width = Pixel.Canvas.ActualWidth
        };
        var label = new Label
        {
            Content = $"GAME OVER\nMoves: {movesCount}, Score: {scoreCount: 0}",
            Foreground = Brushes.GreenYellow,
            FontSize = 50,
            HorizontalAlignment = HorizontalAlignment.Center,
            VerticalAlignment = VerticalAlignment.Center
        };
        grid.Children.Add(label);

        Pixel.Canvas.Children.Add(grid);
    }

    internal IEnumerable<Pixel> GetPixels()
    {
        for (int i = 0; i < Rows; i++)
        {
            for (int j = 0; j < Columns; j++)
            {
                yield return _pixels[i, j];
            }
        }
    }

    public void Clear() => Pixel.Canvas.Children.Clear();

    public (int i, int j) GetStartingPoint(StartPosition startPosition)
    {
        switch (startPosition)
        {
            case StartPosition.Center:
                return (Rows / 2, Columns / 2);
            case StartPosition.LeftDownCorner:
                return (Rows - 2, 1);
            case StartPosition.LeftUpCorner:
                return (1, 1);
            case StartPosition.RightDownCorner:
                return (Rows - 2, Columns - 2);
            case StartPosition.RightUpCorner:
                return (1, Columns - 2);
            default: return (Rows / 2, Columns / 2);
        }
    }

    public bool IsLackOfFood => !GetPixels().Any(p => p.IsFood);

    #endregion

    #region Indexer

    internal Pixel this[int i, int j]
    {
        get
        {
            if (i < 0 || i >= Rows)
            {
                throw new ArgumentOutOfRangeException($"{nameof(i)} must be between '0' and '{Rows}'");
            }
            if (j < 0 || j >= Columns)
            {
                throw new ArgumentOutOfRangeException($"{nameof(j)} must be between '0' and '{Columns}'");
            }
            return _pixels[i, j];
        }
    }

    #endregion

    #region Properties

    public int Rows { get; }
    public int Columns { get; }

    #endregion
}

Pixel

Pixel instance is an element of Matrix(gamefield), where i - row, j - column. It has Shape - circle or square - depends on SnakeGame input settings.

internal class Pixel
{
    #region Fields

    private readonly Shape _pixel;

    #endregion

    #region Ctor

    public Pixel(int i, int j)
    {
        I = i;
        J = j;

        double yCoordinate = i * (Size + Corrective.Item1);
        double xCoordinate = j * (Size + Corrective.Item2);

        if (PixelType == PixelType.Circle)
        {
            _pixel = new Ellipse();
        }
        else
        {
            _pixel = new Rectangle();
        }

        IsFood = false;
        _pixel.Height = Size;
        _pixel.Width = Size;

        Canvas.Children.Add(_pixel);
        Canvas.SetLeft(_pixel, xCoordinate);
        Canvas.SetTop(_pixel, yCoordinate);
    }

    #endregion

    #region Public methods

    public void Fill(SolidColorBrush brush) => _pixel.Fill = brush;

    public void Unfill() => _pixel.Fill = null;

    #endregion

    #region Properties

    public static Canvas Canvas { get; set; }

    public static PixelType PixelType { get; set; }

    public static Tuple<double,double> Corrective { get; set; }

    public static int Size { get; set; }

    public Brush GetFillColor => _pixel.Fill;

    public int I { get; set; }

    public int J { get; set; }

    public bool IsFood { get; set; }

    #endregion

}

Snake.cs

public class Snake
{
    #region Fields

    private GameField _gameField;
    private readonly DispatcherTimer _dispatcherTimer;

    #endregion

    #region Ctors

    private Snake(int length)
    {
        if (length <= 0)
        {
            throw new ArgumentOutOfRangeException($"{nameof(length)} must be positive");
        }
        Body = new List<Pixel>();
        MovingDirection = Direction.Right;
        _dispatcherTimer = new DispatcherTimer();
        _dispatcherTimer.Tick += Move;
        Length = length;
    }

    /// <summary>
    /// Initializes a new instance of Snake
    /// </summary>
    /// <param name="length">Length (must be between 3 and 10)</param>
    /// <param name="startPosition">Start position</param>
    /// <param name="bodyColor">Body Color</param>
    /// <param name="headColor">Head Color</param>
    public Snake(int length, StartPosition startPosition, SolidColorBrush bodyColor,
        SolidColorBrush headColor) : this(length)
    {
        StartPosition = startPosition;
        BodyColor = bodyColor;
        HeadColor = headColor;
    }

    #endregion

    #region Events

    /// <summary>
    /// Occurs when snake crashes
    /// </summary>
    public event EventHandler OnSnakeCrash;

    /// <summary>
    /// Occurs when there is no food
    /// </summary>
    public event EventHandler OnLackOfFood;

    #endregion

    #region Public methods

    /// <summary>
    /// Initializes a snake on game field
    /// </summary>
    /// <param name="gamefield">Game Field</param>
    /// <param name="difficulty">Difficulty</param>
    public void InitializeSnake(GameField gamefield, Difficulty difficulty)
    {
        _dispatcherTimer.Interval = new TimeSpan(0, 0, 0, 0, (int)difficulty);
        _gameField = gamefield ?? throw new ArgumentNullException(nameof(gamefield));
        var startingPoint = gamefield.GetStartingPoint(StartPosition);
        Head = gamefield[startingPoint.i, startingPoint.j];
        Head.Fill(HeadColor);

        for (int i = 1; i <= Length; i++)
        {
            var square = gamefield[startingPoint.i, startingPoint.j - i];
            square.Fill(BodyColor);
            Body.Add(square);
        }

        Pixel.Canvas.Focusable = true;
        Keyboard.Focus(Pixel.Canvas);
        Pixel.Canvas.KeyDown += OnKeyDown;
    }

    /// <summary>
    /// Starts moving in MovingDirection
    /// </summary>
    public void StartMoving() => _dispatcherTimer.Start();

    /// <summary>
    /// Stops moving
    /// </summary>
    public void StopMoving() => _dispatcherTimer.Stop();

    #endregion

    #region Properties

    public SolidColorBrush BodyColor { get; set; }

    public SolidColorBrush HeadColor { get; set; }

    public Direction MovingDirection { get; set; }

    public StartPosition StartPosition { get; set; }

    /// <summary>
    /// Length without Head
    /// </summary>
    public int Length { get; private set; }

    internal List<Pixel> Body { get; }

    internal Pixel Head { get; private set; }

    public int MovesCount { get; set; }

    public double ScoreCount { get; set; }

    #endregion

    #region Private methods

    private void Move(object sender, EventArgs e)
    {
        if (MovingDirection < Direction.Up || MovingDirection > Direction.Left)
        {
            throw new ArgumentException($"{nameof(MovingDirection)} illegal enum value");
        }

        // Speed changes every 100 moves
        if (++MovesCount % 100 == 0)
        {
            SpeedUp();
        }

        if (_gameField.IsLackOfFood)
        {
            OnLackOfFood?.Invoke(this, null);
        }

        Head.Fill(BodyColor);
        Body.Insert(0, Head); // Insert head into body - oh my Gosh

        try
        {
            switch (MovingDirection)
            {
                //gets a brand new Head ;)
                case Direction.Down:
                    Head = _gameField[Head.I + 1, Head.J];
                    break;
                case Direction.Up:
                    Head = _gameField[Head.I - 1, Head.J];
                    break;
                case Direction.Left:
                    Head = _gameField[Head.I, Head.J - 1];
                    break;
                case Direction.Right:
                    Head = _gameField[Head.I, Head.J + 1];
                    break;
            }
        }
        catch (ArgumentOutOfRangeException)
        {
            StopMoving();
            _dispatcherTimer.Tick -= Move;
            OnSnakeCrash?.Invoke(this, null);
        }

        Head.Fill(HeadColor);

        if (Head.IsFood)
        {
            Head.IsFood = false;
            ScoreCount = Math.Ceiling(10 + (0.5 * MovesCount));
            Length++;
        }
        else
        {
            Body.Last().Unfill();
            Body.RemoveAt(Length);
        }

        if (IsBodyRammedByHead())
        {
            StopMoving();
            _dispatcherTimer.Tick -= Move;
            OnSnakeCrash?.Invoke(this, null);
        }
    }

    private void OnKeyDown(object sender, KeyEventArgs eventArgs)
    {
        switch (eventArgs.Key)
        {
            case Key.W:
                if (MovingDirection != Direction.Down)
                MovingDirection = Direction.Up;
                break;
            case Key.S:
                if (MovingDirection != Direction.Up)
                MovingDirection = Direction.Down;
                break;
            case Key.A:
                if (MovingDirection != Direction.Right)
                MovingDirection = Direction.Left;
                break;
            case Key.D:
                if (MovingDirection != Direction.Left)
                MovingDirection = Direction.Right;
                break;
            case Key.P:
                if (_dispatcherTimer.IsEnabled)
                {
                    StopMoving();
                }
                else
                {
                    StartMoving();
                }
                break;
        }
    }

    /// <summary>
    /// Indicating whether Body was rammed by Head ;)
    /// </summary>
    /// <returns></returns>
    private bool IsBodyRammedByHead() => Body.Contains(Head);

    /// <summary>
    /// Accelerates snake movement
    /// </summary>
    private void SpeedUp()
    {
        int currentInterval = _dispatcherTimer.Interval.Milliseconds;
        int subtractor = currentInterval / 3;
        _dispatcherTimer.Interval = new TimeSpan(0, 0, 0, 0, currentInterval - subtractor);
    }

    #endregion
}

ENUMS

public enum Direction
{
    Up,
    Down,
    Right,
    Left
}

public enum PixelType
{
    Circle,
    Square
}

public enum StartPosition
{
    Center,
    [Obsolete("Not supported")]
    LeftUpCorner,
    [Obsolete("Not supported")]
    LeftDownCorner,
    [Obsolete("Not supported")]
    RightUpCorner,
    [Obsolete("Not supported")]
    RightDownCorner
}

public enum Difficulty
{
    Easy = 300,
    Normal = 250,
    Hard = 200,
    VeryHard = 180,
    Impossible = 140
}

What I wrote seems to work although would be very helpfull if you point to my strong and weak points in c# codding.

\$\endgroup\$
  • \$\begingroup\$ After a fast review : 1) Avoid using var it makes the code way less readable. 2) You have some bad indentation 3) SnakeGame.InitializeFood() in there you create a new Random() at every call, this behaviour should be avoided so you don't have possibly weird results \$\endgroup\$ – nalka Oct 8 '18 at 12:36
  • 3
    \$\begingroup\$ @nalka (1) is one of those developer holy wars (like tabs vs spaces, indentation size, or always bracketing one-line-ifs). I find that var increases readability by minimizing pedantry (Michael Brennan agrees with me, with many well-reasoned arguments). You disagree, and that's fine too (I've heard valid counterarguments as well); but there is a clear divide here between developers and there is no "right" choice. \$\endgroup\$ – Flater Oct 8 '18 at 12:44
  • \$\begingroup\$ @Flater I agree with the new ... and decreasing code coupling even if decreasing code coupling is just losing readablity to gain something else so which means the choice has to be determined case by case but as you said and as it often is in our world, there's no perfect answer \$\endgroup\$ – nalka Oct 8 '18 at 13:06
  • \$\begingroup\$ @nalka You have some bad indentation - what intendations? SnakeGame.InitializeFood() in there you create a new Random() at every call, this behaviour should be avoided so you don't have possibly weird results - what results? Give examples \$\endgroup\$ – HelloWorld Oct 8 '18 at 13:22
  • \$\begingroup\$ well i've experienced same result all the time when generating Random massively when there was also rnd = new Random(); being done massively. About indentation it's in Snake.OnKeyDown \$\endgroup\$ – nalka Oct 8 '18 at 13:48
4
\$\begingroup\$

Mixing responsibilities

var snakeGame = new SnakeGame
{
    Snake = new Snake(
        length: 3,
        startPosition: StartPosition.Center,
        bodyColor: Brushes.Red,
        headColor: Brushes.Black),
    GameField = new GameField(
        pixelSize: 50,
        playingField: playingField,
        pixelType: PixelType.Square),
    Difficulty = Difficulty.Hard,
    FoodColor = Brushes.GreenYellow,
    AmountOfFood = 6
};

Just add library to your wpf-project, and instantiate SnakeGame class with input parameters:

From your description, I infer that SnakeGame is part of your library class.

If it is, then you should not be passing colors to the library. Coloring is specifically a UI-based setting, and should thus reside in the UI layer of your solution. All other (library) classes should not have a reference to SolidColorBrush or any similar UI class. I'll omit that from the rest of the review.

The same applies to pixelSize. If you are talking about actual screen pixels, that is a UI-based setting. If you are referring to your board game cells as pixels, I suggest you rename this, as "pixel" is massively ambiguous with screen pixels (Suggested improvement: Cell).

Minor comment: I see you like using named parameters. There's nothing wrong with them, but I would in general suggest using object initialization over named parameters. You've actually used an object initializer for your SnakeGame object but have then switched to named parameters in the constructor for the Snake and GameField properties.

I'm actually apprehensive of stating that you shouldn't use named parameters (they serve largely the same purpose with essentially the same syntax), but I would suggest not mixing styles. Pick one and stick with it as much as you can.


Enums as data types

public enum Difficulty
{
    Easy = 300,
    Normal = 250,
    Hard = 200,
    VeryHard = 180,
    Impossible = 140
}

I'm not a fan of this. You're using the int value of your enum as an actually meaningful value. While it is technically possible, it irks me.

What happens if tomorrow you need two values for a given difficulty? You can no longer use your enum, will have to resort to classes, and are probably going to have to rework every location where you've been handling the enum.

And this is already creeping into your code:

if (Difficulty < Difficulty.Impossible || Difficulty > Difficulty.Easy)
{
    throw new ArgumentException($"{nameof(Difficulty)} illegal enum value");
}

What happens if tomorrow you want to make a toddler version of your game, and you create a VeryEasy option for the toddlers? You're going to have to make sure you remember to change this if check as well.

It makes no sense to validate your enum using this if block. If a particular enum exists, it is therefore a valid option. That's the entire point of an enum; it's a closed list of valid options. There is no need for this if check.

Suggested fix

Create a class that holds your difficulty settings (note that I gave an appropriate name to your int setting)

public class DifficultySettings
{
    public int IntervalTime { get; set; }
}

Provide a static dictionary where you define the difficulty settings.

public static Dictionary<Difficulty, DifficultySettings> DifficultySettings = 
    new Dictionary<Difficulty, DifficultySettings>()
    {
        { Difficulty.Easy, new DifficultySettings() { IntervalTime = 300 } },
        { Difficulty.Normal, new DifficultySettings() { IntervalTime = 250 } },
        { Difficulty.Hard, new DifficultySettings() { IntervalTime = 200 } },
        { Difficulty.VeryHard, new DifficultySettings() { IntervalTime = 180 } },
        { Difficulty.Impossible, new DifficultySettings() { IntervalTime = 140 } },
    }

And then you can retrieve the settings whenever you want:

    _dispatcherTimer.Interval = TimeSpan.FromMilliseconds( DifficultySettings[difficulty].IntervalTime );
  • Note that you can use TimeSpan.FromMilliseconds instead of new TimeSpan, which removed the need for having all those zeroes.
  • If a second property is added to the DifficultySettings, all you need to do is add and initialize the property. You will have no conflicts with your existing code.
  • Note that I hardcoded the values for the sake of simplicity. There is an argument to be made to have these values from from a config file (often a .ini files is used for game configuration). However, I kept it simple here, to keep it readable.

More UI in the library

public enum PixelType
{
    Circle,
    Square
}

This shouldn't be in the library. How to visualize the snake is up to the UI, not the game library. For example, consider that you want to add custom themes to your library. The gameplay (=library) remains unchanged, but the UI will change dramatically. You can't actually guarantee when you'll be using circles and squares.


Reworking the library class responsibilities

Many of your library classes are dirtied due to pushing the UI responsibilities in your library. It's not that easy to point them all out, it would dramatically change the classes. Instead, I'm going to list what your class' responsibilities should be, so that you know which code belongs inside and which doesn't:

  • GameField should only track values, not visuals.
    • No matter how you implement it, you're basically going to end up with some sort of collection of Cell (previously named Pixel) values, and positional values for the snake's body and the food on the board.
    • No colors, no UI shapes, no screen coordinates (you can use board coordinates, but not screen coordinates)
  • Cell (previously named Pixel) should only contain boardcell-related information. This means that it's mostly a data class which will contain some handy methods, such as e.g. telling you what the current content of the board cell is (empty/snake/food/...)
  • Snake should contain all information pertaining to the snake, but not what it looks like.

Smaller comments

    if (pixelType < PixelType.Circle || pixelType > PixelType.Square)
    {
        throw new ArgumentException($"{nameof(pixelType)} illegal enum value");
    }

Similar to before, this is an irrelevant check. Don't use weird and undocumented enum values. If the enum value is included in the list, it should by definition be an allowed value.

    if (pixelSize < 10 || pixelSize > 100)
    {
        throw new ArgumentOutOfRangeException($"{nameof(pixelSize)} must be between '5' and '100'");
    }

First of all, your validation (between 10 and 100) does not match your message (between 5 and 100). You should change these magic values into const values, so that you only have to adjust the value in one place and both the check and the message stay consistent.

But more importantly, why should you care about the pixelsize of the board?

If "pixel" refers to a screen pixel, you should not care at all, as this is a UI consideration.
If "pixel" refers to a cell on the board, then why do you feel the need to limit the board size? Why can't I play on a 200x200 board if I want to?

    Snake.OnLackOfFood += InitializeFood;
    Snake.OnSnakeCrash += GameOver;

There is nothing wrong with using events, but... events are notoriously hard to debug. There are similar approaches that are easier to debug. Look into delegates (and lambdas) if you're interested.

Also, while I'd need to delve much deeper into the code that I have for this code review, I would suggest not relying on spreading the game state information across multiple classes. I suggest having one class control the game state and trigger the needed events, and use the underlying classes (such as Snake) simply to store the data.

This advice is unique to game development. Running the logic for a particular game is often better not divided into separate classes as it may be hard to figure out what's going wrong when you don't have a unified "judge" to evaluate the game state and decided major events.


\$\endgroup\$
  • \$\begingroup\$ Enum are non-restrictive. Technically, Difficulty randomValue = (Difficulty)33; is valid C#. It would be possible for a user to pass in a "nonsensical", though completely valid enum value. As this is a snake library, it should validate user input and throw exceptions in these circumstances. \$\endgroup\$ – pinkfloydx33 Oct 14 '18 at 16:03
  • \$\begingroup\$ @pinkfloydx33 Not using ints directly both avoids this problem and increases readability. \$\endgroup\$ – Flater Oct 14 '18 at 16:32
  • \$\begingroup\$ My point is that you can cast any integral value to an enum type, whether or not that value is actually defined in the enum. (PixelType)666 is valid and could theoretically be passed into the library by the consumer. In that respect validating the range of pixelType is most definitely appropriate (though perhaps better done by validating the value is defined for the enum type) \$\endgroup\$ – pinkfloydx33 Oct 14 '18 at 16:36

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.