Here's a little problem I have in an exercise.

The task is to simplify this over-engineered, fancy looking code while maintaining the functionality of GameManager. The idea is to make it more readable as well. I'm at a loss on what to do and where to start since I do understand completely the code and how it functions as it is. The thought of converting the looping into a function of its own has crossed my mind but that's about it.

Any suggestions on how to proceed, what to cut out, what to change and how?

Thank you in advance.

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



namespace Game
{

    public class GameManager : MonoBehaviour
    {
        List<IUpdateable> _gameEntities = new List<IUpdateable>();
        int _npcCount = 1000;

        public void Awake()
        {
            for (int i = 0; i < _npcCount; i  )
            {
                var npc = new NPC();
                npc.ID = i;
                _gameEntities.Add(npc);
            }

            var player = new Player();
            player.ID = _npcCount   1;
            _gameEntities.Add(player);
        }

        public void ChangeNPCName(int id, string name)
        {
            foreach (var entity in _gameEntities)
            {
                if (entity is NPC)
                {
                    var npc = entity as NPC;
                    if (npc.ID == id)
                    {
                        npc.ChangeName(name);
                    }
                }
            }
        }

        public void ChangePlayerName(string name)
        {
            foreach (var entity in _gameEntities)
            {
                if (entity is Player)
                {
                    entity.ChangeName(name);
                    entity.Save();
                }
            }
        }
    }

    public interface IUpdateable
    {
        void ChangeName(string name);
        void Save();
    }

    public abstract class Entity<T> where T : IUpdateable
    {
        protected string _name;

        private int _id;
        public int ID { get => _id; set => _id = value; }

        public void ChangeName(string name)
        {
            _name = name;
        }
    }

    public class NPC : Entity<NPC>, IUpdateable
    {
        public void Save()
        {

        }
    }

    public class Player : Entity<Player>, IUpdateable
    {
        public void Save()
        {

        }
    }
}

CodePudding user response：

In terms of how to proceed, general advice would be to write some unit tests against the existing code, covering all of the functionality. Then you can begin incremental refactoring with confidence that your tests will verify that the behaviour of the code is still correct.

CodePudding user response：

From very first glance, the code is compact enough to understand it from the first glance. The only thing that could be addressed in terms of simplification is to get rid of classes/interface hierarchy.

1. Replace NPC and Player with their baser class Entity (just specify their kind via a property or field and maybe external functions to customize their behavior/strategy)
2. That way, you can get rid of both generic and IUpdateable so Entity can be used as a simple class in all the places.
3. Also, you can replace a list with array then to improve data locality (however, this is multi-aspect situation so you have to judge with some measurements involved)