28
\$\begingroup\$

Recently, I have been trying to learn SOLID principles. I learn the best through criticism.

I made this snake game in Unity with C#, where I put my current understanding of SOLID principles to test. The game itself works wonderful, but I am trying to write better code.

Main questions:

  1. What principles am I breaking? And how can I fix them?
  2. Should I use interfaces instead of polymorphism?

Other:

  1. Am I doing any bad practises?
  2. Is my design bad?

I don't know Unity and can't read your code

Dont worry, I barely use Unity in this game. Everything should be understandable.

Explaining my solution:

My solution is to have all objects placed in positions in a "grid". Multiple objects can be at any positions. Logic is handled in Before(), Next() and After() in all of the objects. These are called every 0.1 seconds (or per step).

Every object is independent. There are direction objects that are placed whenever the user presses a key. When a snakepart is at the same position of this direction object, then the snakepart gets that direction. The direction obj is deleted once it no longer is at a position with a snakepart.

IVector2 is simply a class with int x and int y. It has nothing to do with interfaces (bad naming, my bad).

Own Comment

The number of files is ridiculous for such a small game. And it took some time too. However, bugs were INCREDIBLE easy to find and deal with. Loved it.

If you like anything, feel free to use it. I may upload the project, if it is requested. I can also upload a video of the final project, if so desired.

Filenames overview:

  • Game.cs: Monobehavior - Start is called once on start, Update is called every frame
  • CreateGame.cs
  • DeleteGame.cs
  • GameOver.cs
  • InputHandler.cs
  • Score.cs
  • Snake.cs
  • ObjFactory.cs
  • Direction.cs
  • GameObjectInstantiator.cs

Objs

  • Obj.cs: base class
  • VisualObj.cs: Obj
  • AppleObj.cs: VisualObj
  • SnakePartObj.cs: VisualObj - Each individual part of the snake
  • BarrierObj.cs: VisualObj - GameOver if a snakepart is at a barrier
  • DirectionObj.cs: Obj - Changes the direction of snakePartObjs

Other

  • DataBase.cs - Here the objects are stored in this "grid"
  • MultipleValuesDictionary.cs - Used in Database

Game.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
namespace SnakeGameSOLID
{
    public class Game : MonoBehaviour
    {
        public GameObject sphere;

        ObjFactory factory;
        InputHandler input;
        Snake snake;
        CreateGame createGame;
        GameOver gameOver;
        Score score;

        Direction inputDirection = new Direction();

        float timer = 1.0f;

        public void Start()
        {
            factory = new ObjFactory(sphere);
            input = new InputHandler();
            score = new Score();
            snake = new Snake(factory,score);
            createGame = new CreateGame();
            createGame.Create(snake, factory);
            gameOver = new GameOver(createGame, factory, snake);
            snake.InjectGameOver(gameOver);

        }

        public void Update()
        {
            timer -= Time.deltaTime;
            input.HandleArrows(inputDirection);
            if (timer < 0)
            {
                input.UseInput(factory, snake, inputDirection);
                timer = 0.1f;
                snake.Before();
                factory.CallAllBefore();
                factory.CallAllNext();
                factory.CallAllAfter();

                Debug.Log("The Current Score Is: " + score.GetScore().ToString());
            }
        }
    }
}

CreateGame.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using Misc;
namespace SnakeGameSOLID
{
    public class CreateGame
    {

        public void Create(Snake snake, ObjFactory factory)
        {
            snake.CreateSnake(new IVector2(20,20),3);
            CreateEdgeBarriers(factory, 30, 30);
            //Five apples for fun
            for(int i = 0; i < 5; i ++)
                factory.CreateVisualObject<AppleObj>(new IVector2(Random.Range(1, 29), Random.Range(1, 29)), new Color(1, 0, 0, 1));
        }

        public void CreateEdgeBarriers(ObjFactory factory,int xSize, int ySize)
        {
            for (int x = 0; x < xSize; x++)
                for (int y = 0; y < ySize; y++)
                {
                    if (x == 0 || y == 0 || x == (xSize-1) || y == (ySize-1))
                        factory.CreateVisualObject<BarrierObj>(new IVector2(x, y), new Color(0,0,0,0));
                }
        }
    }
}

DeleteGame.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
namespace SnakeGameSOLID
{
    public class DeleteGame 
    {

        public void Delete(ObjFactory factory)
        {
            factory.Clear();
        }
    }
}

GameOver.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
namespace SnakeGameSOLID
{
    public class GameOver 
    {
        CreateGame createGame;
        DeleteGame deleteGame;
        ObjFactory factory;
        Snake snake;
        public GameOver(CreateGame createGame, ObjFactory factory, Snake snake)
        {
            this.snake = snake;
            this.factory = factory;
            this.createGame = createGame;
            deleteGame = new DeleteGame();
        }

        public void ResetGame()
        {
            deleteGame.Delete(factory);
            createGame.Create(snake, factory);
        }
    }
}

InputHandler.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
namespace SnakeGameSOLID
{
    public class InputHandler
    {
        public void HandleArrows(Direction dir)
        {
            if (Input.GetKeyUp(KeyCode.UpArrow))
            {
                dir.direction = Direction.names.Up;
            }
            if (Input.GetKeyUp(KeyCode.DownArrow))
            {
                dir.direction = Direction.names.Down;
            }
            if (Input.GetKeyUp(KeyCode.LeftArrow))
            {
                dir.direction = Direction.names.Left;
            }
            if (Input.GetKeyUp(KeyCode.RightArrow))
            {
                dir.direction = Direction.names.Right;
            }
        }

        public void UseInput(ObjFactory factory,Snake snake, Direction dir)
        {
            if (dir.direction == Direction.names.None)
                return;

            //Dont use oppisite input
            if (snake.GetHeadDirection().IsOppisiteDirection(dir.direction))
            {
                return;
            }

            DirectionObj o = (DirectionObj)factory.CreateObject<DirectionObj>(snake.GetHeadPos());
            o.dir.direction = dir.direction;



            dir.direction = Direction.names.None;
        }

    }
}

Score.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
namespace SnakeGameSOLID
{
    public class Score
    {
        int score = 0;
        public int GetScore() { return score; }
        public void AddPoint() { score++; }
        public void ResetScore() { score = 0; }
    }
}

Snake.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using Misc;
namespace SnakeGameSOLID
{
    public class Snake 
    {
        SnakePartObj head;
        SnakePartObj lastPart;
        IVector2 lastPositionOfLastPart;
        Direction.names lastDirOfLastPart;
        ObjFactory objectfactory;
        Color headColor;
        Color snakeColor;
        GameOver gameOver;
        Score score;

        public Snake(ObjFactory factory,Score score)
        {
            objectfactory = factory;
            headColor = new Color(0, 1, 0, 1);
            snakeColor = new Color(0, 0.5f, 0, 1);
            this.score = score;
        }

        public void CreateSnake(IVector2 headPos, int NumbOfParts)
        {
            head = CreateSnakeObj(headPos, headColor);

            IVector2 pos = headPos;
            pos.y--;
            for(int y = 1; y < (NumbOfParts-1); y++)
            {
                CreateSnakeObj(pos, snakeColor);
                pos.y--;
            }

            lastPart = CreateSnakeObj(pos, snakeColor);
        }

        SnakePartObj CreateSnakeObj(IVector2 pos, Color col)
        {
            SnakePartObj obj = objectfactory.CreateVisualObject<SnakePartObj>(pos, col);
            obj.InstallSnakePart(this);
            return obj;
        }

        //Skeptical on this, breaks ISP?
        public void GameOver()
        {
            score.ResetScore();
            gameOver.ResetGame();
        }

        public void InjectGameOver(GameOver gameOver)
        {
            this.gameOver = gameOver;
        }

        public void CreateNewPart()
        {
            score.AddPoint();

            lastPart = CreateSnakeObj(lastPositionOfLastPart, snakeColor);
            lastPart.dir.direction = lastDirOfLastPart;

        }

        public void Before()
        {
            lastPositionOfLastPart = lastPart.position;
            lastDirOfLastPart = lastPart.dir.direction;
        }

        public IVector2 GetHeadPos()
        {
            return head.position;
        }

        public Direction GetHeadDirection()
        {
            return head.dir;
        }




    }
}

ObjFactory.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using System;
using System.Linq;
using Misc;
namespace SnakeGameSOLID
{
    public class ObjFactory
    {
        GameObjectInstantiator instantiator;

        //Hold all of the positions of all objects, and the objects themselves
        DataBase<IVector2, Obj> objectDatabase;
        public ObjFactory(GameObject prefab)
        {
            instantiator = new GameObjectInstantiator(prefab);
            objectDatabase = new DataBase<IVector2, Obj>();
        }

        //Create object of type Obj and install
        public T CreateObject<T>(IVector2 position) where T : Obj, new()
        {
            T obj = new T();
            obj.Install(position);
            InsertObject(position, obj);
            return obj;
        }

        //Create object of Type ObjVisual and install
        public T CreateVisualObject<T>(IVector2 position, Color color) where T : VisualObj, new()
        {
            T obj = new T();
            obj.InstallVisual(instantiator.CreateInstance(), color);
            obj.Install(position);
            InsertObject(position, obj);
            return obj;
        }

        public void Clear()
        {
            foreach (var o in objectDatabase.GetAllObjects().ToList())
            {
                o.DestroyThis(objectDatabase);
            }
            objectDatabase = new DataBase<IVector2, Obj>();
        }

        void InsertObject(IVector2 position, Obj o)
        {
            objectDatabase.AddEntry(position, o);
        }


        //Better way of doing these three functions?
        public void CallAllNext()
        {
            foreach (var o in objectDatabase.GetAllObjects().ToList())
            {
                o.Next(objectDatabase);
            }
        }

        public void CallAllAfter()
        {
            foreach (var o in objectDatabase.GetAllObjects().ToList())
            {
                o.After(objectDatabase);
            }
        }
        public void CallAllBefore()
        {
            foreach (var o in objectDatabase.GetAllObjects().ToList())
            {
                o.Before(objectDatabase);
            }
        }
    }
}

Direction.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using Misc;
namespace SnakeGameSOLID
{
    public class Direction
    {
        public enum names { Left, Right, Up, Down, None }
        public names direction { get; set; }

        public IVector2 GetVector()
        {
            switch (direction)
            {
                case names.Left:
                    return new IVector2(-1, 0);

                case names.Right:
                    return new IVector2(1, 0);
                case names.Up:
                    return new IVector2(0, 1);
                case names.Down:
                    return new IVector2(0, -1);
            }
            return new IVector2(0, 0);
        }

        public bool IsOppisiteDirection(names dir)
        {
            if(dir == names.Up && direction == names.Down)
            {
                return true;
            }

            if (dir == names.Down && direction == names.Up)
            {
                return true;
            }

            if (dir == names.Left && direction == names.Right)
            {
                return true;
            }

            if (dir == names.Right && direction == names.Left)
            {
                return true;
            }
            return false;
        }
    }
}

GameObjectInstantiator

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using UnityEngine;

namespace SnakeGameSOLID
{
    class GameObjectInstantiator
    {
        GameObject prefab;
        public GameObjectInstantiator(GameObject prefab)
        {
            this.prefab = prefab;
        }
        public GameObject CreateInstance()
        {
            return (GameObject)GameObject.Instantiate(prefab);
        }
    }
}

Obj classes

Obj.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using Misc;
namespace SnakeGameSOLID
{

    //Should i use interfaces instead? Combined? Or not?
    public class Obj
    {
        public IVector2 position { get; private set; }


        public Obj()
        {

        }
        public virtual void Install(IVector2 position)
        {
            SetPosition(position);
        }
        public virtual void Next(DataBase<IVector2,Obj> objects) {

        }

        public virtual void After(DataBase<IVector2, Obj> objects) { }
        public virtual void Before(DataBase<IVector2, Obj> objects) { }

        public void Move(DataBase<IVector2,Obj> objects, IVector2 newPos)
        {
            newPos += position;
            objects.MoveEntry(position, newPos, this);
            SetPosition(newPos);
        }

        public void Replace(DataBase<IVector2, Obj> objects, IVector2 newPos)
        {
            objects.MoveEntry(position, newPos, this);
            SetPosition(newPos);
        }

        protected virtual void SetPosition(IVector2 pos)
        {
            position = pos;

        }

        public virtual void DestroyThis(DataBase<IVector2, Obj> objects)
        {
            objects.RemoveEntry(position,this);
        }



    }
}

VisualObj.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using Misc;
namespace SnakeGameSOLID
{
    public class VisualObj : Obj
    {
        GameObject instance;



        protected override void SetPosition(IVector2 pos)
        {

            base.SetPosition(pos);
            instance.transform.position = new Vector3(pos.x, pos.y, 0);
        }

        public void InstallVisual(GameObject instance, Color color)
        {
            this.instance = instance;
            //Just setting the color of the visual object
            instance.GetComponent<MeshRenderer>().material.color = color;
        }

        public override void DestroyThis(DataBase<IVector2, Obj> objects)
        {
            GameObject.Destroy(instance);
            base.DestroyThis(objects);
        }
    }
}

AppleObj.cs

using System;
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using Misc;
namespace SnakeGameSOLID
{
    public class AppleObj : VisualObj
    {
        public AppleObj() : base()
        {
        }

        public override void Next(DataBase<IVector2, Obj> objects)
        {

            SnakePartObj snakePart = objects.GetObjectOfType<SnakePartObj>(position);

            //If there is a snake part at the apples position, then make the snake longer and place the apple in another position
            if(snakePart != null)
            {
                snakePart.GetSnake().CreateNewPart();
                Replace(objects, RandomPos());
            }
        }

        IVector2 RandomPos()
        {
            return new IVector2(UnityEngine.Random.Range(1, 27), UnityEngine.Random.Range(1, 27));
        }
    }
}

SnakePartObj.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using Misc;
namespace SnakeGameSOLID
{
    public class SnakePartObj : VisualObj
    {

        public Direction dir { get; set; }
        Direction directionChange = new Direction();
        Snake snake;
        public SnakePartObj() : base()
        {
            dir = new Direction();
            dir.direction = Direction.names.Up;
            directionChange.direction = Direction.names.Up;
        }

        public override void Next(DataBase<IVector2, Obj> objects)
        {
            base.Next(objects);
            Move(objects, dir.GetVector());
        }
        public override void After(DataBase<IVector2, Obj> objects)
        {


            var snakePartsAtPos = objects.GetObjectsOfType<SnakePartObj>(position);
            //There can only be maxinum one snake part at a position at all times
            if(snakePartsAtPos != null)
            if(snakePartsAtPos.Count > 1)
            {
                snake.GameOver();
            }
        }

        //Is this bad practice?
        public Snake GetSnake()
        {
            return snake;
        }

        public void InstallSnakePart(Snake snake)
        {
            this.snake = snake;
        }
    }
}

BarrierObj.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using Misc;
namespace SnakeGameSOLID
{
    public class BarrierObj : VisualObj
    {
        public BarrierObj() : base()
        {
        }


        public override void After(DataBase<IVector2, Obj> objects)
        {

            SnakePartObj snakePart = (SnakePartObj)objects.GetObjectOfType<SnakePartObj>(position);
            if (snakePart != null)
            {
                snakePart.GetSnake().GameOver();
            }
        }
    }
}

DirectionObj.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using Misc;
namespace SnakeGameSOLID
{
    public class DirectionObj : Obj
    {
        public Direction dir { get; set; }
        public DirectionObj() : base()
        {
            dir = new Direction();
            dir.direction = Direction.names.Up;
        }
        public Direction.names GetDirection() { return dir.direction; }

        public override void Before(DataBase<IVector2, Obj> objects)
        {
            base.Before(objects);

            SnakePartObj snakeObjAtPos = objects.GetObjectOfType<SnakePartObj>(position);

            //Change the direction of the snake part if it is on this directionObj
            ChangeSnakeDirection(snakeObjAtPos);
            //If there are no snake parts in this position, then delete this
            if (snakeObjAtPos == null)
                this.DestroyThis(objects);


        }

        void ChangeSnakeDirection(SnakePartObj part)
        {
            if (part == null)
                return;
            part.dir.direction = dir.direction;
        }

    }
}

Other

DataBase.cs

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using System.Linq;
using System;

namespace SnakeGameSOLID
{
    public class DataBase <key,obj>
    {
        MultipleValuesDictionary<key,obj> objects = new MultipleValuesDictionary<key, obj>();
        HashSet<obj> objectsHash = new HashSet<obj>();

        public void AddEntry(key key, obj obj)
        {
            objects.AddEntry(key, obj);
            objectsHash.Add(obj);
        }

        public void RemoveEntry(key key, obj obj)
        {
            objects.RemoveEntry(key, obj);
            objectsHash.Remove(obj);
        }

        public void RemoveEntry(key key)
        {
            HashSet<obj> values = objects.GetValues(key);
            foreach(var val in values)
            {
                objectsHash.Remove(val);
            }

            objects.RemoveEntry(key);

        }

        public void MoveEntry(key oldKey,key newKey, obj obj)
        {
            RemoveEntry(oldKey, obj);
            AddEntry(newKey, obj);
        }

        public Dictionary<key,HashSet<obj>> GetAllEntries()
        {
            return objects.GetDictionary();
        }

        public HashSet<obj> GetAllObjects()
        {
            return objectsHash;
        }


        public T GetObjectOfType<T>(key k) where T : obj
        {
            var objs = GetObjectsOfType<T>(k);
            if (objs == null)
                return default(T);
            if (objs.Count == 0)
                return default(T);

            return (T)objs[0];

        }

        public List<obj> GetObjectsOfType<T>(key k) where T : obj
        {
            HashSet<obj> objs = objects.GetValues(k);
            if (objs == null)
                return null;

            return objs.Where(o => (o.GetType() == typeof(T))).ToList();

        }

        public List<obj> GetObjectsOfType<T>() where T: obj
        {
            return objectsHash.Where(o => (o.GetType() == typeof(T))).ToList();
        }


    }
}

MultipleValuesDictionary.cs

using System;
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using System.Linq;
namespace SnakeGameSOLID
{
    //Solution to having multiple values in a dictionary
    public class MultipleValuesDictionary<key, value>
    {
        Dictionary<key, HashSet<value>> dictionary = new Dictionary<key, HashSet<value>>();

        public void AddEntry(key k, value v)
        {
            HashSet<value> values;
            if (dictionary.TryGetValue(k, out values))
            {
                if (values.Contains(v))
                {
                    //Exception?
                    return;
                }
                values.Add(v);
                return;

            }
            values = new HashSet<value>();

            values.Add(v);
            dictionary.Add(k, values);
        }

        public void RemoveEntry(key k)
        {
            dictionary.Remove(k);
        }

        public void RemoveEntry(key k, value val)
        {
            HashSet<value> values = GetValues(k);
            values.Remove(val);
        }

        public Dictionary<key,HashSet<value>> GetDictionary()
        {
            return dictionary;
        }
        public HashSet<value> GetValues(key k)
        {

            HashSet<value> values;
            dictionary.TryGetValue(k, out values);
            return values;
        }

        //Gets all values of a specific type at a position
        public List<value> GetValuesOfTypeAtKey<T>(key k) where T : value
        {
            HashSet<value> values = GetValues(k);
            return values.Where(o => (o.GetType() == typeof(T))).ToList();
        }

    }
}
\$\endgroup\$
  • 7
    \$\begingroup\$ Too many classes for my taste. I have some doubts if e.g. Direction really deserves to be it's own class. Also functional classes like CreateGame, DeleteGame seem to be better actions (functions) in a single class called Game. \$\endgroup\$ – πάντα ῥεῖ Jan 6 '17 at 13:38
  • 6
    \$\begingroup\$ Too much code for me to read through, but get rid of all the "obj" prefixes/suffixes - everything is an object, that goes without saying; you don't need to include that text in your names. \$\endgroup\$ – 404 Jan 6 '17 at 13:46
  • 3
    \$\begingroup\$ @Vermacian55 I find your question is ok and there is not too much introduction, on the contrary, a good overview of what's coming. True, there is a lot of code but I saw longer explanations and they were ok too. \$\endgroup\$ – t3chb0t Jan 6 '17 at 14:05
  • 4
    \$\begingroup\$ There are 17 classes and 600 lines of code. I'd say this time it might really be too much. I'd be probably better to ask smaller questions about snake design or scoring system etc. \$\endgroup\$ – t3chb0t Jan 6 '17 at 14:56
  • 5
    \$\begingroup\$ The code smell here is "design pattern run amok". Slavish adherence to a pattern, no matter how useful, tends to undermine the value of that pattern. You've got Class Overflow, it reminds me of FizzBuzz Enterprise Edition. Don't let readability and YAGNI go out the window. \$\endgroup\$ – Cody Gray Jan 6 '17 at 17:24
17
\$\begingroup\$

Just a few remarks. Reviewing bottom-up.


public class MultipleValuesDictionary<key, value>

Don't name classes dictionary if they aren't one (implementing the IDictionary<,> interface). It's confusing.

You can derive a type from the dictionary instead of implementing only parts of it and inventing new vocabulary for it.

Don't come up with new generic parameters names. The conventions is to start the names with the uppercase letter T.

You could simply define it as:

public class MultipleValuesDictionary<TKey, TValue> : Dictionary<TKey, HashSet<TValue>> { }

public List<value> GetValuesOfTypeAtKey<T>(key k) where T : value
{
    HashSet<value> values = GetValues(k);
    return values.Where(o => (o.GetType() == typeof(T))).ToList();
}

No dictionary should have such method. It's a special case and you could easily get what you need with the OfType<T> extension:

 var results = multipleValueDictionary[key].OfType<MyType>().ToList();

public class DataBase <key,obj>

This is an interesting class with an even more interesing naming convention:

public void AddEntry(key key, obj obj)

Don't! Use TKey and TValue.

The private objectsHash doesn't seem like it was necessary. The only place you use it for is

public HashSet<obj> GetAllObjects()
{
    return objectsHash;
}

where you could simply use SelectMany

 dictionary.Values.SelectMany(x => x)

public class DirectionObj : Obj

Everything seems to be either an Obj or have the suffix -Obj. Calling everything obj in an OO language is like you ware calling every-thing a thing. No one would understand you.


public Direction dir { get; set; }

Property names -> PascalCase. No abbreviations.


dir.direction = Direction.names.Up;

The code lacks consistency but the Obj suffix :-)

\$\endgroup\$
  • \$\begingroup\$ Excellent! Bunch of things i did not know! Thank you. And .OfType seems really handy. Obj suffix <3 \$\endgroup\$ – Vermacian55 Jan 6 '17 at 16:09
14
\$\begingroup\$

I sense that you think too much about SOLID, padawon. You need to think about what the game IS, and the things that ARE. Objects do things. What they do defines what they are. Properties capture the state of a thing as it does stuff. Leave the search for the unified field theory of object oriented design for later.

Obsesssing over Open/Close results in one-method classes.

Single Responsibility is THE force w/in the SOLID universe. Your midiclorians are too low, padawon.

What, in all the universe, is not an object? To name a thing "obj" (or xxxObj) is to deny its purpose, padawon.

Send Obj.cs to CERN, for you have discovered the one, fundamental particle that underlies the fabric of the universe. It is an amorphous abstraction that informs all entities derived therefrom that they have no purpose; that each obj is a distinction without a difference.

The secret is, Mr. Anderson, that there is no spoon - or Apple, or Barrier, or anything; not in this Snake Game corner of the universe. There is not a single coherent class here.

\$\endgroup\$
  • 1
    \$\begingroup\$ I give you +1 for the funny story and I think he's called padawan. \$\endgroup\$ – t3chb0t Jan 6 '17 at 15:11
  • 2
    \$\begingroup\$ I talk w/ a southern accent sometimes. \$\endgroup\$ – radarbob Jan 6 '17 at 15:15
  • 1
    \$\begingroup\$ Hahahaha great. Master, i still got a lot to learn. When you say "Your midiclorians are too low", do you mean that i am not fulfilling SRP? Also, what should i do about the obj situasjon? Are you calling for a rename or that the design is flawed? If it is flawed how could i fix it? if i rename it to "DataBaseObject", it would not make a difference would it? \$\endgroup\$ – Vermacian55 Jan 6 '17 at 16:15
  • \$\begingroup\$ Oh of course. I can just do DataBase<IVector2, object> instead of DataBase<IVector2, Obj>. lol. \$\endgroup\$ – Vermacian55 Jan 6 '17 at 16:39
  • 1
    \$\begingroup\$ " what should i do about the obj situasjon? Are you calling for a rename or that the design is flawed?" The naming is a hint of a possibly flawed design. It suggests to me you're thinking in quantum particle terms, not "snake game" terms. I suggest this is where the SRP failure begins. Carefully talk your way through an explanation of the game, identifying the parts, what each part does, and how they interact. As a repetitive process you will identify specific parts and general parts, and their relationships. \$\endgroup\$ – radarbob Jan 6 '17 at 18:37
11
\$\begingroup\$

Beyond the SOLID principles, think about your classes and methods. Classes/objects are things that exist, methods are things they do. So when naming them, class names should be nouns, methods should be verbs. If you find yourself naming your classes after verbs, it indicates that you aren't thinking through what those classes are actually for.

As such, I have trouble with your CreateGame, DeleteGame, and GameOver classes. Creating and deleting is something you do with the Game object, not things that can exist by themselves. Whatever these do should be methods in the Game class, not classes unto themselves. Reading the code, I'm having trouble following what you are trying to do in instantiating CreateGame and DeleteGame objects. I think if you rethink these classes into methods you perform on the Game class you will also find your code to become simpler. Taken further, a method called Create probably isn't necessary for the Game class. Creating an instance of the class is what the constructor is for.

GameOver is a little different in that the name doesn't really describe a noun or a verb, but rather a state of the Game (that is, the state of a game that has ended). Consider rewriting this as a method on the Game class called End, or, since it appears all this is doing is deleting the game if I am reading your code right, just incorporate its logic into the Game.Delete method. Your ResetGame method you have in GamOver is, again, an action you are performing on Game, and should be part of the Game class.

Good general rule:

Class = noun (something that IS)

Method = verb (something a class DOES)

Another trick you can use is say to yourself what you are trying to accomplish with a sentence like "I need to {method} the {class/object}" (e.g. "I need to Start the Game" or "I need to Remove the SnakePart"). Then you know if that is a method that needs to be on that class. Similarly, you can apply this to code you have already written to see if it makes sense. If you end up with sentences that don't make sense like "I need to ResetGame the GameOver", it is likely something went askew and it is a good time to revise what you were trying to do there.

\$\endgroup\$
  • \$\begingroup\$ Yeah, the 'CreateGame' game and such classes is a clear mistake on my side. Thanks for the trick! I can see it being useful. \$\endgroup\$ – Vermacian55 Jan 7 '17 at 19:25
5
\$\begingroup\$

Overall design

First a summary of your design:

The Game class initializes the game and runs the main game loop (Update). Every step, it applies user input and updates all game objects (by calling their Before, Next and After methods).

Game objects (the ApplyObj, BarrierObj, SnakePartObj and DirectionObj classes) are positioned on a 2D grid (represented by a DataBase<IVector2, Obj> instance).

The player controls a snake (the Snake class) that consists of multiple parts (SnakePartObj). When an arrow key press is handled, an invisible direction marker is placed on the grid. This marker tells snake parts in which direction they should move next. This prevents parts from becoming disconnected.

Apples check every step whether they're colliding with a snake part. If so, they tell the snake to increment its length (which increases the players score) and they'll move to a random new location.

Barriers also check every step whether they're colloding with a snake part. If so, they trigger a game-over.

Overall design observations

A few things stand out in the above summary:

  • the direction markers
  • apples and barriers are constantly checking for snake parts
  • the Before/Next/After methods
  • an odd database class

Keeping the snake together could be solved more elegantly. For example by removing the tail part and adding a new head part, so other parts don't need to move at all (when the snake grows you simply don't remove the tail). Or by giving each part a reference to the next part, so they don't need direction markers to tell them where to move next.

Why do apples and barriers need to check for snake parts all the time? Collisions always happen with the snake head, so it's sensible to only let the snake (head) check for collisions instead. Bytheway, does your code take into account that the snake may hit itself?

Having 3 different update methods indicates that you were running into order problems. With the above two changes, you likely don't need these 3 methods anymore. In fact, a single Snake.Update call per step should be sufficient - all other game objects are passive anyway.

DataBase<key, obj> implies persistent storage, but it's actually used to represent a 2D grid that contains game objects. Grid (or Level, Map, Environment) would be a more descriptive name. The class is also only used with IVector2 as key and Obj as value (obj), and its method names heavily imply 2D movement, so there is no reason to make it generic. YAGNI: 'you ain't gonna need it'.

Class design

When we look further into the code, we'll see that game objects (Obj) are created (and also updated) via a factory (ObjFactory), which also registers them in the game grid (DataBase<IVector2, Obj>). There's an additional factory for creating visual parts (GameObjectInstantiator).

The DataBase<key, obj> class is gives you access to all game objects, and provides fast spatial lookup for collision checks. It's using a MultipleValuesDictionary<key, value> class internally.

There are also several game-state related classes, such as CreateGame, DeleteGame and GameOver. CreateGame initializes the player snake and populates the game grid with barriers and apples, while DeleteGame erases the game grid, and GameOver uses the other two to restart the game.

Then there are the InputHandler, Score, Direction and IVector2 classes - utilities, mostly. InputHandler checks for arrow key presses and returns an appropriate direction, Score keeps track of the score, Direction is used in various places as movement direction (up, down, left or right) and IVector2 stores a 2D coordinate.

Class design observations

If we inspect the code, we'll note several things:

  • ObjFactory mixes object creation with updating (game logic)
  • a custom (semi-)dictionary class
  • the game grid must be kept up to date whenever an Obj is moved
  • some game-related functionality has been implemented with classes instead of methods
  • Direction is oddly designed

ObjFactory has too many responsibilities. Those CallAll* methods do not belong there. It's also doing very little, so I'm not really convinced that a factory is adding much value here. One disadvantage of this design is that it uses a new() constraint, so you can't use constructors to enforce proper initialization (SnakePartObj is not properly initialized until you call InstallSnakePart, but the factory doesn't do that for you).

There's no need for MultipleValuesDictionary<key, value> - it's essentially just a Dictionary<IVector2, HashSet<Obj>>. If that's too inconvenient to use then a few (extension) helper methods should be sufficient. You could also use a 2D array of HashSet<Obj>. Actually, without direction markers you don't need to support multiple objects per cell, so a 2D array of Obj is sufficient.

Keeping object positions in sync with the game grid obviously complicates things. It may be easier to just keep a list of game objects - there aren't that many and you only need to check for collisions once, for the snake's head, so it's unlikely that you'll run into performance problems. Or, with a different snake movement strategy (removing/adding parts instead of moving them) you don't need any movement support at all. Alternately, you could perform all movement via the grid directly, so game objects don't need those methods that require a game grid reference.

Others have already commented on your CreateGame, DeleteGame and GameOver classes: those should be methods in Game.

Direction is a mutable class that holds a names enum, and it contains some utility methods. Its mutable nature allows some rather unintuitive code: InputHandler.HandleArrows returns a direction by mutating its Direction argument. It would be much easier to understand (and harder to misuse) when Direction was an enum itself, and InputHandler.HandleArrows would just return a Direction (enum). You may want to read up on '(data) encapsulation'.

Conclusion

There's more to be said about the actual low-level code itself (adhering to standard C# naming conventions, using more descriptive method and class names, not hard-coding values, various other low-level improvements), but I feel this review is long enough as it is.

All in all, I think your code is overly complicated for what it does. SOLID is good and all, but don't forget KISS: 'keep it simple, stupid'. :)

\$\endgroup\$
  • \$\begingroup\$ The overall design summary is on point. "Bytheway, does your code take into account that the snake may hit itself?", yes. In SnakePartObj.cs After function, it checks if there are more than one snakePartObj at the same position. If so then it is gameover. When a snake "bites" itself, it will cause two snakeparts to be at the same position. And yes, i was running into order problems, which your examples fix. I felt like my design gave a lot of freedom, but then again as you said "YAGNI". \$\endgroup\$ – Vermacian55 Jan 7 '17 at 19:39
  • \$\begingroup\$ Dont quite get what you mean by "the game grid must be kept up to date whenever an Obj is moved", the grid is a class, and therefore a reference right? So it will be kept up to date automaticly, right? \$\endgroup\$ – Vermacian55 Jan 7 '17 at 19:40
  • \$\begingroup\$ "Keeping object positions in sync with the game grid obviously complicates things. It may be easier to just keep a list of game objects". I made the code in such a way as if it was a big project, to test the waters. But then again "YAGNI" \$\endgroup\$ – Vermacian55 Jan 7 '17 at 19:42
  • \$\begingroup\$ "_ One disadvantage of this design is that it uses a new() constraint, so you can't use constructors to enforce proper initialization (SnakePartObj is not properly initialized until you call InstallSnakePart, but the factory doesn't do that for you)._", do you know a way to get around that? \$\endgroup\$ – Vermacian55 Jan 7 '17 at 19:43
  • \$\begingroup\$ And yes indeed KISS. Thanks! This cleared up a ton of things for me! \$\endgroup\$ – Vermacian55 Jan 7 '17 at 19:46

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.