3
\$\begingroup\$

I wrote the next code chunk to save all the allowed positions from a 3x3 matrix, it's working but I think it could be better. Besides something similar happens in my Randomizer class, although I admit it's messy.

public class MatrixRegistry : MonoBehaviour
{
    private int matrixVolume;
    private int matrixCount;

    private GameObject[] matrix;

    private List<GameObject> dynamicCubes = new List<GameObject>();
    private List<Vector3> staticCubes = new List<Vector3>();
    private List<Vector3> allowedPositions = new List<Vector3>();

    public Func<List<GameObject>> GetDynamicCubes { get; private set; }
    public Func<List<Vector3>> GetAllowedPositions { get; private set; }

    private void Awake()
    {
        #region Getting all Matrix Cubes

        matrixCount = gameObject.transform.childCount;

        if (matrixCount != 0)
        {
            matrix = new GameObject[matrixCount];

            for (int i = 0; i < matrixCount; i++)
            {
                matrix[i] = gameObject.transform.GetChild(i).gameObject;

                if (matrix[i].GetComponent<CubeInfo>().cubeType == CubeSort.Dynamic)
                {
                    dynamicCubes.Add(matrix[i]);
                }

                if (matrix[i].GetComponent<CubeInfo>().cubeType == CubeSort.Static)
                {
                    staticCubes.Add(matrix[i].transform.position);
                }
            }
        }

        GetDynamicCubes = () => dynamicCubes;
        #endregion

        #region Setting Allowed Positions

        matrixVolume = 3; // Hard Coding

        for (int x = 0; x < matrixVolume; x++)
            {
                for (int y = 0; y < matrixVolume; y++)
                {
                    for (int z = 0; z < matrixVolume; z++)
                    {
                        Vector3 position = new Vector3
                        {
                            x = x,
                            y = y,
                            z = z
                        };

                        allowedPositions.Add(position);
                    }
                }
            }

        for (int f = 0; f < staticCubes.Count; f++)
        {
            for (int a = 0; a < allowedPositions.Count; a++)
            {
                if (allowedPositions[a] == staticCubes[f])
                {
                    allowedPositions.Remove(allowedPositions[a]);
                }
            }
        }

        GetAllowedPositions = () => allowedPositions;
        #endregion
    }
}

public class Randomizer
{
    private MatrixRegistry matrixRegistry;
    private EventManager eventManager;

    public Randomizer(MatrixRegistry matrixRegistry, EventManager eventManager)
    {
        this.matrixRegistry = matrixRegistry;
        this.eventManager = eventManager;

        eventManager.OnStartGame += Randomize;
    }

    private void Randomize()
    {
        for (int i = 0; i < matrixRegistry.GetDynamicCubes().Count; i++)
        {
            Vector3 randomPosition = new Vector3
            {
                x = Random.Range(0, 3),
                y = Random.Range(0, 3),
                z = Random.Range(0, 3)
            };

            for (int j = 0; j < matrixRegistry.GetAllowedPositions().Count; j++)
            {
                for (int k = 0; k < matrixRegistry.GetDynamicCubes().Count; k++)
                {
                    if (randomPosition == matrixRegistry.GetAllowedPositions()[j])
                    {
                        if (randomPosition != matrixRegistry.GetDynamicCubes()[k].transform.position)
                        {
                            matrixRegistry.GetDynamicCubes()[i].transform.position = randomPosition;
                        }
                    }
                }    
            }
        }
    }
}

The randomizer class should check all the allowed positions to get in place all the cubes and avoid overlapping issues. I know it's not the best, and this is why I'm here.

\$\endgroup\$
  • \$\begingroup\$ Why don't you post the complete class? I'd be much easier to review it having all the code rather then some random snippets. \$\endgroup\$ – t3chb0t Jan 14 at 7:24
  • 1
    \$\begingroup\$ @t3chb0t I didn't think it was necessary, it's just a for loop, but already I did it. \$\endgroup\$ – Lobsang White Jan 14 at 7:39
  • 1
    \$\begingroup\$ It looks much better now and it's much easier to understand. Thanks for updating ;-] \$\endgroup\$ – t3chb0t Jan 14 at 7:42
1
\$\begingroup\$

The way you are checking if any allowedPositions are in the staticCubes list seems very inefficient to me. I think it would be more efficient to check whether each position is in the staticCubes list as they are created then add them to the allowedPositions list.

Using a LINQ query for this wil also make it much more concise:

allowedPositions = (from int x in Enumerable.Range(0, matrixVolume)
                    from int y in Enumerable.Range(0, matrixVolume)
                    from int z in Enumerable.Range(0, matrixVolume)
                    let position = new Vector3(x, y, z)
                    where !staticCubes.Contains(position)
                    select position).ToList();
\$\endgroup\$
  • \$\begingroup\$ I don't know much about LINQ features, I'm going to read about it. About Randomizer class, the Randomize method could be improve using LINQ. I still think is awful. \$\endgroup\$ – Lobsang White Jan 15 at 8:35
  • \$\begingroup\$ @LobsangWhite - I added notes about the Randomizer class for you to look at. \$\endgroup\$ – tinstaafl Jan 15 at 14:20
  • \$\begingroup\$ Yes, but it's not swap among their own positions, it's a 3x3 matrix, that's means 27 positions, what it does is shuffle among all the possible positions, but the static ones. \$\endgroup\$ – Lobsang White Jan 15 at 17:34
  • \$\begingroup\$ @LobsangWhite - according to your code, the positions in dynamicCubes don't include the positions in staticCubes \$\endgroup\$ – tinstaafl Jan 15 at 18:08
  • \$\begingroup\$ Exactly, this is why I use GetAllowedPositions method, because I stored all the matrix positions there, except static cubes positions, and I check if the new position could be the right one if is equal to an allowed position from the list returned by GetAllowedPositions method. \$\endgroup\$ – Lobsang White Jan 15 at 19:18

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.