0
\$\begingroup\$

I am looking for feedback on my code's overall structure and design. I made a few concessions in reusability for the sake of not overcomplicating things (static/const public variables in the Game1 class) but think it's pretty usable otherwise. I would like to make this room generator easy to use and adjust for future projects, and would love any feedback on how I can better do so. Here is my code on GitHub.

Game1.cs

using System;
using System.Collections.Generic;
using System.ComponentModel;
using Microsoft.Xna.Framework;
using Microsoft.Xna.Framework.Graphics;
using Microsoft.Xna.Framework.Input;

namespace RoomGenerator
{
    public class Game1 : Game
    {
        public enum TileType
        {
            Floor,
            Wall
        }
        
        //Variables that are accessed by the eater class.
        public static TileType[,] TileMap { get; set; }
        public const int MapSize = 100; //The square dimensions of the tile map in terms of tiles.
        public const int MaxFloors = 1200;
        public static int CurrentFloors { get; set; } = MaxFloors;
        
        private GraphicsDeviceManager graphics;
        private SpriteBatch spriteBatch;
        private List<Eater> eaters;
        private List<Eater> eatersAccumulator; //Used to ensure an enumeration operation error does not occur.
        private Texture2D floor;
        private Texture2D wall;
        private const int TileSize = 8; //The square dimensions of each tile sprite.
        
        public Game1()
        {
            graphics = new GraphicsDeviceManager(this);
            Content.RootDirectory = "Content";
            IsMouseVisible = true;

            TileMap = new TileType[MapSize, MapSize];
            eaters = new List<Eater>();
            eatersAccumulator = new List<Eater>();
        }

        protected override void Initialize()
        {
            base.Initialize();

            //Resolution of window.
            graphics.PreferredBackBufferWidth = 1000;
            graphics.PreferredBackBufferHeight = 1000;
            graphics.ApplyChanges();
            
            Reset();
        }

        protected override void LoadContent()
        {
            spriteBatch = new SpriteBatch(GraphicsDevice);
            floor = Content.Load<Texture2D>("Ground");
            wall = Content.Load<Texture2D>("Wall");
        }

        protected override void Update(GameTime gameTime)
        {
            base.Update(gameTime);
            
            //Handle input
            if (GamePad.GetState(PlayerIndex.One).Buttons.Back == ButtonState.Pressed || Keyboard.GetState().IsKeyDown(Keys.Escape))
                Exit();

            if (Keyboard.GetState().IsKeyDown(Keys.R))
                Reset();

            foreach (var eater in eaters)
            {
                eater.Move();
            }

            foreach (var eater in eatersAccumulator)
            {
                eaters.Add(eater);
            }
            eatersAccumulator.Clear();

            Console.WriteLine($"Number floors created: {MaxFloors - CurrentFloors}, " +
                              $"Number Eaters: {eaters.Count}");

            int trueCount = 0;

            for (int i = 0; i < TileMap.GetLength(0); i++)
            {
                for (int j = 0; j < TileMap.GetLength(1); j++)
                {
                    if (TileMap[i, j] == TileType.Floor)
                        trueCount++;
                }
            }
            
            Console.WriteLine($"True Count: {trueCount}");
        }

        protected override void Draw(GameTime gameTime)
        {
            base.Draw(gameTime);
            
            GraphicsDevice.Clear(Color.SkyBlue);
            
            //Rendering to a RenderTarget and then later passing it to the back buffer.
            RenderTarget2D target = new RenderTarget2D(GraphicsDevice, MapSize * TileSize, MapSize * TileSize);
            GraphicsDevice.SetRenderTarget(target);
            
            spriteBatch.Begin();

            //Draw each tile in the proper position.
            for (int i = 0; i < TileMap.GetLength(0); i++)
            {
                for (int j = 0; j < TileMap.GetLength(1); j++)
                {
                    switch (TileMap[i, j])
                    {
                        case TileType.Floor:
                            spriteBatch.Draw(floor, 
                                new Rectangle(i * TileSize, j * TileSize, TileSize, TileSize), 
                                Color.White);
                            break;
                        case TileType.Wall:
                            spriteBatch.Draw(wall, 
                                new Rectangle(i * TileSize, j * TileSize, TileSize, TileSize), 
                                Color.White);
                            break;
                        default:
                            throw new InvalidEnumArgumentException("Given tile type is not supported.");
                    }
                }
            }
            
            spriteBatch.End();
            
            //Render target to back buffer.
            GraphicsDevice.SetRenderTarget(null);
            spriteBatch.Begin(SpriteSortMode.Deferred, BlendState.NonPremultiplied, SamplerState.PointClamp);
            spriteBatch.Draw(target, new Rectangle(0, 0, graphics.PreferredBackBufferWidth, graphics.PreferredBackBufferHeight), Color.White);
            spriteBatch.End();
        }

        private void Reset()
        {
            //Set all tiles back to wall.
            for (int i = 0; i < TileMap.GetLength(0); i++)
            {
                for (int j = 0; j < TileMap.GetLength(1); j++)
                {
                    TileMap[i, j] = TileType.Wall;
                }
            }

            CurrentFloors = MaxFloors;
            eaters.Clear();
            
            eaters.Add(new Eater(new Vector2(0, 0), 
                new Vector2(MapSize / 2, MapSize / 2)));
            eaters[0].GenerateDirection();
            eaters[0].AddEater += AddEater;
        }

        /// <summary>
        /// Triggered by Eaters to add more eaters to the generator. Allows for generation of separate corridors.
        /// </summary>
        private void AddEater(Vector2 startPosition)
        {
            Eater eater = new Eater(new Vector2(0, 0), startPosition);
            eater.GenerateDirection();
            eater.AddEater += AddEater; //Subscribe new eater to this method.
            
            eatersAccumulator.Add(eater);
        }
    }
}

Eater.cs

using System;
using Microsoft.Xna.Framework;

namespace RoomGenerator
{
    public class Eater
    {
        public Vector2 Direction { get; private set; }
        public Vector2 Position { get; private set; }
        public event Action<Vector2> AddEater;

        private Random random;

        public Eater(Vector2 direction, Vector2 position)
        {
            Direction = direction;
            Position = position;
            random = new Random();
        }

        /// <summary>
        /// Move the Eater in its Direction by one step, remove whatever wall is in the way.
        /// </summary>
        public void Move()
        {
            var newPos = Position + Direction;

            if (Game1.CurrentFloors <= 0)
                return;
            
            //Invalid direction, change direction and return.
            if (newPos.X < 0 || newPos.Y < 0 || newPos.X > Game1.MapSize - 1 || newPos.Y > Game1.MapSize - 1)
            {
                GenerateDirection();
                return;
            }

            Position = newPos;
            
            if (Game1.TileMap[(int) Position.X, (int) Position.Y] != Game1.TileType.Floor)
            {
                Game1.TileMap[(int) Position.X, (int) Position.Y] = Game1.TileType.Floor;
                Game1.CurrentFloors--;
            }
            else
            {
                GenerateDirection();
            }
            
            int genNewEater = random.Next(400);
            if (genNewEater == 0) //10% chance of adding new eater
            {
                AddEater?.Invoke(Position); 
            }
            
            GenerateDirection();
        }

        /// <summary>
        /// Generate a new direction for the eater.
        /// </summary>
        /// <exception cref="ArgumentException"></exception>
        public void GenerateDirection()
        {
            Random rand = new Random();
            int dir = rand.Next(4);

            switch (dir)
            {
                //Left
                case 0:
                    Direction = new Vector2(-1, 0);
                    break;
                //Up
                case 1:
                    Direction = new Vector2(0, 1);
                    break;
                //Right
                case 2:
                    Direction = new Vector2(1, 0);
                    break;
                //Down
                case 3:
                    Direction = new Vector2(0, -1);
                    break;
                default:
                    throw new ArgumentException("Invalid value generated for new direction.");
            }
        }
    }
}
\$\endgroup\$
2
  • 3
    \$\begingroup\$ Please tell us more about the purpose of the code. Does it function well? \$\endgroup\$ – Mast Jan 10 at 11:22
  • \$\begingroup\$ @Mast Yes, it does its job well. It is made to generate random rooms for usage in games. It does so by generating an Eater to remove tiles by moving in random directions, which can occasionally spawn new Eaters. \$\endgroup\$ – Ovicior Jan 10 at 22:07
1
\$\begingroup\$

Top smells of your code are

  • Comments. Names of classes, functions, variables should tell why it exists and what it does. If you want to write a comment, then your code is not self-descriptive and should be refactored.
  • Too big functions. 10 lines of code is a pretty big function. 5-6 is ok. You have functions 30-40 lines long. That also creates inconsistent level of abstraction in your functions.
  • Violation of Tell don't ask principle - you ask data from objects to perform some logic instead of asking objects to do what you want. Check how you use Game object in the Eater class.
  • Lot of magic values which have meaning and should be named properly. Check direction vectors, initial position, window size, etc.

I'm not going to refactor everything. But I'll show you idea. Let's take a look at your Game1 class. Of course it should not be named Game1. Name it CaveEatersGame or whatever. Here is self-descriptive game loop:

protected override void Update(GameTime gameTime)
{
    base.Update(gameTime);

    if (ExitRequested)
        Exit();

    if (RestartRequested)
        RestartGame();

    MoveEaters();
    SpawnNewEaters();
}

That's easy to achieve with small propertis and methods:

private void MoveEaters()
{
    foreach (var eater in eaters)
    {
        if (AllWallsGone) // Check condition in game loop, not in each eater
            EndGame();    // Whatever it means

        eater.TryMove();  // Eater does not always move
    }
}

private void SpawnNewEaters()
{
    // No events or additional accumulators required
    var newEaters = eaters.Where(Eater.IsLuckyToHaveChild).Select(Eater.CreateChild);
    eaters.AddRange(newEaters);
}

You can even pass direction to eater and simplify movement to

public void TryMove(Vector2 direction)
{
    var newPosition = Position + direction;
    if (!game.IsAvailableToMove(newPosition)) // move this logic to game
       return;

    MoveTo(newPosition);
}

Maybe you don't need eater class at all. Eventually that is just a vector. Especially if you'll move direction generation to separate class, making it focused on one thing and reusable:

public class RandomDirectionGenerator : IDirectionGenerator
{
    private readonly Vector2 up = new Vector2(0, 1);
    private readonly Vector2 left = new Vector2(-1, 0);
    private readonly Vector2 down = new Vector2(0, -1);
    private readonly Vector2 right = new Vector2(1, 0);
    private readonly Random random = new Random(); // inject for unit-testing

    public Vector2 GetNext() =>
        random.Next(4) switch
        {
            0 => left,
            1 => up,
            2 => right,
            _ => down
        };
}
\$\endgroup\$
3
  • \$\begingroup\$ Wow Sergey, that's amazing! Thanks so much for the extensive feedback. It's all really helpful and I'll be sure to integrate it. I made a lot of decisions based on the assumption that I wouldn't be touching the code again, so I wrote it a bit messily to make it easier to work. Thank you again! \$\endgroup\$ – Ovicior Jan 13 at 6:43
  • \$\begingroup\$ Sergey, I made many of the modifications you suggested. I had some trouble with the Linq implementation for SpawnNewEaters but I integrated most of your feedback. I'd appreciate it if you could take a quick look! github.com/Kakapio/room-generator \$\endgroup\$ – Ovicior Jan 16 at 10:24
  • \$\begingroup\$ @Ovicior for cave generation it will be totally fine to generate eaters in the game class. Just define a function that accepts eater and returns boolean if new eater should be generated (something like eater => 0 == random.Next(chance)) and function which creates new eater from detail of existing (something like eater => new Eater(eater.Position)) \$\endgroup\$ – Sergey Berezovskiy Jan 16 at 13:00

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.