Sliding puzzle game of fifteen

Question

I've made the Game of Fifteen in Unity this is how it looks :

The code is rather short there's one class for the pieces and one for the game logic :

MovablePiece : MonoBehaviour

public class MovablePiece : MonoBehaviour
{
    public PuzzleGrid.SlotPosition CurrentSlotPosition { get; set; }

    private void Start()
    {
        GetComponent<Button>().onClick.AddListener(MoveOnClick);
    }

    private void MoveOnClick()
    {
        if (CurrentSlotPosition.Y == PuzzleGrid.CurrentEmptySlotPosition.Y &&
            (CurrentSlotPosition.X + 1 == PuzzleGrid.CurrentEmptySlotPosition.X ||
             CurrentSlotPosition.X - 1 == PuzzleGrid.CurrentEmptySlotPosition.X))
        {
            StartCoroutine(AwaitMovement());
        }
        if (CurrentSlotPosition.X == PuzzleGrid.CurrentEmptySlotPosition.X &&
            (CurrentSlotPosition.Y + 1 == PuzzleGrid.CurrentEmptySlotPosition.Y ||
             CurrentSlotPosition.Y - 1 == PuzzleGrid.CurrentEmptySlotPosition.Y))
        {
            StartCoroutine(AwaitMovement());
        }
    }

    private IEnumerator AwaitMovement()
    {
        Vector3 currentPosition = transform.position;
        while (transform.position.normalized != PuzzleGrid.EmptySlot.transform.position.normalized)
        {
            transform.position = Vector3.MoveTowards(transform.position, PuzzleGrid.EmptySlot.transform.position, 10*Time.deltaTime);
            yield return null;
        }
        GameObject temp = PuzzleGrid.grid[PuzzleGrid.CurrentEmptySlotPosition.X][PuzzleGrid.CurrentEmptySlotPosition.Y];
        PuzzleGrid.grid[PuzzleGrid.CurrentEmptySlotPosition.X][PuzzleGrid.CurrentEmptySlotPosition.Y] = gameObject;
        PuzzleGrid.grid[CurrentSlotPosition.X][CurrentSlotPosition.Y] = temp;
        PuzzleGrid.EmptySlot.transform.position = currentPosition;
        PuzzleGrid.SlotPosition tempPosition = PuzzleGrid.CurrentEmptySlotPosition;
        PuzzleGrid.CurrentEmptySlotPosition = CurrentSlotPosition;
        CurrentSlotPosition = tempPosition;
    }
}

The main function in the class is the courotine AwaitMovement which is called in the button click eventhandler, I figured that for a block to be considered movable it needs to be either on the same Y axis, or on the same X axis with a difference in the other of 1. Here's an illustration where 1,1 is the empty block, the rest are the blocks that can be currently moved.

This is what we are checking in the button click event. Inside the actual courotine we are updating the position of the empty space

PuzzleGrid : MonoBehaviour

public class PuzzleGrid : MonoBehaviour
{
    private const int GridSize = 16;
    public static List<GameObject>[] grid = new List<GameObject>[4];

    private static Text GameStateText;
    private static Text MovesText;

    private static int movesCount;
    private static SlotPosition previousSlotPosition;

    private static SlotPosition currentEmptySlotPosition;

    public static SlotPosition CurrentEmptySlotPosition
    {
        get { return currentEmptySlotPosition; }
        set
        {
            if (value.X == previousSlotPosition.X && value.Y == previousSlotPosition.Y)
            {
                movesCount--;
                currentEmptySlotPosition = default(SlotPosition);
            }
            else
            {
                movesCount++;
            }
            previousSlotPosition = currentEmptySlotPosition;
            MovesText.text = "Moves : " + movesCount;
            currentEmptySlotPosition = value;
            if (CheckWin())
            {
                GameStateText.text = @"You Won !";
            }
        }
    }

    public static GameObject EmptySlot { get; set; }

    private readonly System.Random rnd = new System.Random();

    private static readonly int[] orderNumbers = new int[GridSize - 1];

    private void Awake()
    {
        GetComponent<GridLayoutGroup>().enabled = true;
        movesCount = 0;
        Image[] imageObjects = FindObjectsOfType<Image>();
        GameStateText =
            imageObjects
                .Where(t => t.name == "GameStateText")
                .Select(t => t.GetComponentInChildren<Text>())
                .First();
        MovesText =
            imageObjects
                .Where(t => t.name == "MovesText")
                .Select(t => t.GetComponentInChildren<Text>())
                .First();
    }

    private void Start()
    {
        for (int i = 0; i < orderNumbers.Length; i++)
        {
            orderNumbers[i] = i + 1;
        }
        SetupGrid();
        SetupPieces();
    }

    private void SetupGrid()
    {
        GameObject[] pieces =
            GetComponentsInChildren<Image>().Where(c => c.tag == "Piece").Select(c => c.gameObject).ToArray();
        int index = 0;
        for (int i = 0; i < grid.Length; i++)
        {
            grid[i] = new List<GameObject>();
            for (int j = 0; j < grid.Length; j++)
            {
                grid[i].Add(pieces[index]);
                index++;
            }
        }
        GetComponent<GridLayoutGroup>().enabled = false;
    }

    private void SetupPieces()
    {
        List<int> numbers = new List<int>(GridSize);
        for (int i = 0; i < GridSize; i++)
        {
            numbers.Add(i + 1);
        }
        numbers = Shuffle(numbers).ToList();
        int indexToRemove = numbers.IndexOf(GridSize);
        numbers.RemoveAt(indexToRemove);
        currentEmptySlotPosition = new SlotPosition(indexToRemove/grid.Length, indexToRemove%grid.Length);
        EmptySlot = grid[CurrentEmptySlotPosition.X][CurrentEmptySlotPosition.Y];
        EmptySlot.SetActive(false);
        int index = 0;
        for (int i = 0; i < grid.Length; i++)
        {
            for (int j = 0; j < grid[i].Count; j++)
            {
                if (i != CurrentEmptySlotPosition.X || j != CurrentEmptySlotPosition.Y)
                {
                    grid[i][j].GetComponentInChildren<Text>().text = numbers[index].ToString();
                    grid[i][j].GetComponent<MovablePiece>().CurrentSlotPosition = new SlotPosition(i, j);
                    index++;
                }
            }
        }
    }

    public void StartNewGame()
    {
        SceneManager.LoadScene(SceneManager.GetActiveScene().name);
    }

    private IList<T> Shuffle<T>(IList<T> array)
    {
        int n = array.Count;
        for (int i = 0; i < n; i++)
        {
            int r = i + (int) (rnd.NextDouble()*(n - i));
            T t = array[r];
            array[r] = array[i];
            array[i] = t;
        }
        return array;
    }

    private static bool CheckWin()
    {
        int index = 0;
        for (int i = 0; i < grid.Length; i++)
        {
            for (int j = 0; j < grid.Length; j++)
            {
                if (orderNumbers[index] != int.Parse(grid[i][j].GetComponentInChildren<Text>().text))
                {
                    return false;
                }
                index++;
                if (index >= orderNumbers.Length)
                {
                    return true;
                }
            }
        }
        return true;
    }

    public struct SlotPosition
    {
        public int X { get; set; }
        public int Y { get; set; }

        public SlotPosition(int x, int y) : this()
        {
            X = x;
            Y = y;
        }
    }
}

My idea here was to create the board and the piece before the game is started because it's easier to sort them and arrange them in the order I want. I'm using a GridLayoutGroup to sort the buttons and once the game is started I'm removing it so that, whenever we pick the empty slot button and we disable it, it wont automatically rearrange to the last position (bottom right corner).

I was initially removing the empty slot button completely by destroying it which made my grid uneven on columns so I went for a jagged array but later, I figured I can just disable it and it will still be fine in which case a normal 2D array would still do but I decided to leave it as it is.

Answer 1

Alright, so I've finally had time to go through this.

First, you are absolutely abusing the set method on a property.

public static SlotPosition CurrentEmptySlotPosition
{
    get { return currentEmptySlotPosition; }
    set
    {
        if (value.X == previousSlotPosition.X && value.Y == previousSlotPosition.Y)
        {
            movesCount--;
            currentEmptySlotPosition = default(SlotPosition);
        }
        else
        {
            movesCount++;
        }
        previousSlotPosition = currentEmptySlotPosition;
        MovesText.text = "Moves : " + movesCount;
        currentEmptySlotPosition = value;
        if (CheckWin())
        {
            GameStateText.text = @"You Won !";
        }
    }
}

That is way more than a set method should do. A good alternative to this would be to create an event for CurrentEmptySlotPositionChanged instead, and fire the event when set is changed.

---

private const int GridSize = 16;

Excellent. You know the importance of making 'magic numbers' not so magic, but what about this bit:

transform.position = Vector3.MoveTowards(transform.position, PuzzleGrid.EmptySlot.transform.position, 10*Time.deltaTime);

What is 10 for?

---

GameStateText.text = @"You Won !";

Absolutely no need for the @ there. Whether or not you keep it is up to you but the @ symbol eliminates the need for escape sequences in strings. It's mostly helpful with paths and Regex: @"C:\Some Folder\Some File.txt" as opposed to "C:\\Some Folder\\Some File.txt".

---

You don't have enough line breaks. Throw an empty line in-between logical sections to help disseminate the code.

---

private void MoveOnClick()
{
    if (CurrentSlotPosition.Y == PuzzleGrid.CurrentEmptySlotPosition.Y &&
        (CurrentSlotPosition.X + 1 == PuzzleGrid.CurrentEmptySlotPosition.X ||
         CurrentSlotPosition.X - 1 == PuzzleGrid.CurrentEmptySlotPosition.X))
    {
        StartCoroutine(AwaitMovement());
    }
    if (CurrentSlotPosition.X == PuzzleGrid.CurrentEmptySlotPosition.X &&
        (CurrentSlotPosition.Y + 1 == PuzzleGrid.CurrentEmptySlotPosition.Y ||
         CurrentSlotPosition.Y - 1 == PuzzleGrid.CurrentEmptySlotPosition.Y))
    {
        StartCoroutine(AwaitMovement());
    }
}

Here's what I would do: define an IsNeighborEmpty:

private bool IsNeighborEmpty(PuzzleGrid.SlotPosition currentPosition)
{
    var emptyPosition = PuzzleGrid.CurrentEmptySlotPosition;

    var xDiff = Math.Abs(emptyPosition.X - currentPosition.X);
    var yDiff = Math.Abs(emptyPosition.Y - currentPosition.Y);

    return xDiff == 0 && yDiff == 1 || yDiff == 0 && xDiff == 1;
}

You can cheat with the fact that determining if they are neighbors can simply involve subtraction of the position values. You could optionally remove the Math.Abs call and replace it with xDiff == 0 && (yDiff == 1 || yDiff == -1) || yDiff == 0 && (xDiff == 1 || xDiff == -1) which may or may not be a performance hit. In general this method will probably be slower than your version, but not by much and the readability and sensibility of it more than makes up for that.

---

public static List<GameObject>[] grid = new List<GameObject>[4];

private static Text GameStateText;
private static Text MovesText;

So that is all wrong.

In C#, you should know that public member naming is PascalCase and private member naming is camelCase. Yes, Unity tends to violate this, but generally speaking you shouldn't.

Second: any public-facing members are expected to be properties. For grid it's a simple fix:

public static List<GameObject>[] Grid { get; } = new List<GameObject>[4];

This will not affect your assignments of Grid[index] or Grid[index][index] anywhere, it will only make Grid itself immutable.