Sliding puzzle game of fifteen

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().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

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[] grid = new List[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[] Grid { get; } = new List[4];

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

StackExchange Code Review Q#150418, answer score: 2