5
\$\begingroup\$

I've made the Simon says game in Unity

enter image description here

You start by guessing a pattern of 3 colors and each time you complete the pattern a new, random one, is generated and the length of the pattern is incremented by 1.

The game utilises a custom generic collection I wrote CircularListNavigator<T>. I also have a question about it here

public class CircularListNavigator<T> : IList<T>
{
    private readonly IList<T> _circularListNavigator = new List<T>();

    private int lastUsedElementIndex;

    public T MoveNext()
    {
        int temp = lastUsedElementIndex;
        lastUsedElementIndex = lastUsedElementIndex + 1 >= _circularListNavigator.Count ? 0 : lastUsedElementIndex + 1;
        return _circularListNavigator[temp];
    }

    public T MovePrevious()
    {
        int temp = lastUsedElementIndex;
        lastUsedElementIndex = lastUsedElementIndex - 1 < 0 ? _circularListNavigator.Count - 1 : lastUsedElementIndex - 1;
        return _circularListNavigator[temp];
    }

    public T Current
    {
        get
        {
            return _circularListNavigator.Count == 0 ? default(T) : _circularListNavigator[lastUsedElementIndex];
        }
    }

    public void Reset()
    {
        lastUsedElementIndex = 0;
    }

    public CircularListNavigator(int startingIterableIndex = 0)
    {
        lastUsedElementIndex = startingIterableIndex;
    }

    public CircularListNavigator(IEnumerable<T> source, int startingIterableIndex = 0)
    {
        _circularListNavigator = source.ToCircularListNavigator();
        lastUsedElementIndex = startingIterableIndex;
    }

    public CircularListNavigator<T> ConvertToCircularListNavigator(IEnumerable<T> collection, int startingIterableIndex)
    {
        CircularListNavigator<T> iterableCollection = new CircularListNavigator<T>(startingIterableIndex);
        foreach (var item in collection)
        {
            iterableCollection.Add(item);
        }
        return iterableCollection;
    }

    public T this[int index]
    {
        get { return _circularListNavigator[index]; }
        set { _circularListNavigator[index] = value; }
    }

    public IEnumerator<T> GetEnumerator()
    {
        return _circularListNavigator.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public int Count
    {
        get { return _circularListNavigator.Count; }
    }

    public bool IsReadOnly
    {
        get { return _circularListNavigator.IsReadOnly; }
    }

    public void Add(T item)
    {
        _circularListNavigator.Add(item);
    }

    public void Clear()
    {
        _circularListNavigator.Clear();
    }

    public bool Contains(T item)
    {
        return _circularListNavigator.Contains(item);
    }

    public void CopyTo(T[] array, int arrayIndex)
    {
        _circularListNavigator.CopyTo(array, arrayIndex);
    }

    public int IndexOf(T item)
    {
        return _circularListNavigator.IndexOf(item);
    }

    public void Insert(int index, T item)
    {
        _circularListNavigator.Insert(index, item);
    }

    public bool Remove(T item)
    {
        return _circularListNavigator.Remove(item);
    }

    public void RemoveAt(int index)
    {
        _circularListNavigator.RemoveAt(index);
    }
}

It has only 1 extension method which converts an IEnumerable<T> to CircularListNavigator<T>

public static CircularListNavigator<T> ToCircularListNavigator<T>(this IEnumerable<T> collection)
{
    CircularListNavigator<T> circularListNavigator = new CircularListNavigator<T>();
    foreach (object item in collection)
    {
        circularListNavigator.Add((T)item);
    }
    return circularListNavigator;
}

The actual game logic is held in a single class SimonHandler

public class SimonHandler : MonoBehaviour
{
    public Text WinLossText;
    public Button[] Buttons;
    public float ShowDuration;

    private CircularListNavigator<int> indexOrder;
    private const int startingMoveCount = 3;
    private int movesCount;

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

    private void Start()
    {
        foreach (Button button in Buttons)
        {
            button.GetFrameSelected().enabled = false;
        }
        movesCount = startingMoveCount;
        StartGame();
    }

    private void StartGame()
    {
        indexOrder = new CircularListNavigator<int>();
        int number = 0;
        for (int i = 0; i < movesCount; i++)
        {
            indexOrder.Add(number);
            number++;
            if (number > Buttons.Length - 1)
            {
                number = 0;
            }
        }
        indexOrder = new CircularListNavigator<int>(Shuffle(indexOrder));
        StartCoroutine(AwaitShowPattern());
    }

    private IEnumerator AwaitShowPattern()
    {
        string order = string.Empty;
        indexOrder.Reset();
        SetButtonInteractivity(false);
        int currentMove = 0;
        while (currentMove < movesCount)
        {
            order += indexOrder.Current;
            Buttons[indexOrder.Current].GetFrameSelected().enabled = true;
            yield return new WaitForSeconds(ShowDuration);
            Buttons[indexOrder.Current].GetFrameSelected().enabled = false;
            indexOrder.MoveNext();
            currentMove++;
            yield return null;
        }
        WinLossText.text = "Playing..";
        Debug.Log(order);
        SetButtonInteractivity(true);
    }

    private void SetButtonInteractivity(bool value)
    {
        foreach (Button button in Buttons)
        {
            button.interactable = value;
        }
    }

    public void PressButton(Button button)
    {
        int index = Array.IndexOf(Buttons, button);
        if (index == indexOrder.Current)
        {
            if (indexOrder.Count <= 1)
            {
                WinLossText.text = "Win";
                movesCount++;
                StartGame();
            }
            else
            {
                indexOrder.Remove(indexOrder.Current);
            }
        }
        else
        {
            WinLossText.text = "Loss";
            movesCount = startingMoveCount;
            StartGame();
        }
    }

    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;
    }
}

There is one additional extension method used here GetFrameSelected<T> which is implemented as follows :

public static Image GetFrameSelected<T>(this T source) where T : MonoBehaviour
{
    return source.GetComponentsInChildren<Image>().SingleOrDefault(img => img.name == "Frame");
}

I'm looking for comments/improvements/tips on the game logic or the collection regrading code-style primarily, since performance doesn't concern me that much as the game is rather short and simple.

\$\endgroup\$
3
\$\begingroup\$
private readonly IList<T> _circularListNavigator = new List<T>();

This isn't a navigator yet. I suggest naming it simply values.


public Text WinLossText;
public Button[] Buttons;
public float ShowDuration;

Ouch! public non-static non-readonly fields.


private void StartGame()
{
    indexOrder = new CircularListNavigator<int>();
    int number = 0;
    for (int i = 0; i < movesCount; i++)
    {
        indexOrder.Add(number);
        number++;
        if (number > Buttons.Length - 1)
        {
            number = 0;
        }
    }
    indexOrder = new CircularListNavigator<int>(Shuffle(indexOrder));
    StartCoroutine(AwaitShowPattern());
}

You use the the CircularListNavigator twice here but you discard the first one. I think using a List<T> to collect the numbers would be enough and then you can use this list to shuffle it and create the navigator.


One more thing about it. I think the loop is a good candidate for a general purpose extension, I named it TakeOrRepeat

public static IEnumerable<T> TakeOrRepeat<T>(this IEnumerable<T> values, int count)
{
    var counter = 0;
    var enumerator = values.GetEnumerator();
    var moveNext = new Func<bool>(() =>
    {
        if (enumerator.MoveNext())
        {
            return true;
        }
        // Could not move-next. Reset enumerator and try again.
        enumerator = values.GetEnumerator();
        return enumerator.MoveNext();
    });

    while (counter++ < count)
    {
        if (moveNext())
        {
            yield return enumerator.Current;
        }
        else
        {
            yield break;
        }
    }
}

If you made the Shuffle method an extension too, the StartGame method could then be reduced to just a few lines of code of mostly LINQ:

var indicies = 
    Enumerable
    .Range(0, Buttons.Length)
    .TakeOrRepeat(movesCount)
    .ToList()
    .Shuffle();
indexOrder = new CircularListNavigator<int>(indicies);
StartCoroutine(AwaitShowPattern());

or if you like to just two because you already have one more extension the ToCircularListNavigator:

indexOrder = 
    Enumerable
    .Range(0, Buttons.Length)
    .TakeOrRepeat(movesCount)
    .ToList()
    .Shuffle()
    .ToCircularListNavigator();
StartCoroutine(AwaitShowPattern());

private IList<T> Shuffle<T>(IList<T> array)

In cases like this the parameter is usually named just values. The name array is a little bit confusing.


public static CircularListNavigator<T> ToCircularListNavigator<T>(this IEnumerable<T> collection)
{
    CircularListNavigator<T> circularListNavigator = new CircularListNavigator<T>();
    foreach (object item in collection)
    {
        circularListNavigator.Add((T)item);
    }
    return circularListNavigator;
}

If you made the item of type T then you woundn't need the cast... but the CircularListNavigator already has a constructor that accepts a collection... but it uses the extension. This is a vicious circle.

You should move the loop into the constructor or use the List<T>.AddRange(..) or even better use the new List<T>(..) constructor to initilize it from the source.

The CircularListNavigator should not depend on the extension. The extension should depend on the CircularListNavigator.

\$\endgroup\$
  • \$\begingroup\$ TL;DR field are ok in Unity .I agree on most of your points except for Ouch! public non-static non-readonly fields., public fields are a little bit more special in Unity if you make the field public or use the [SerializeField] attribute your field will show up in the inspector which is quite important if you dont want magic numbers and since object inheriting MonoBehaviour cant be instantiate this is your only way of settings values, I honestly hate this so much, I either need to make a [SerializeField]private variable to back my property or I need to use a public field.. \$\endgroup\$ – Denis Dec 26 '16 at 9:40
  • \$\begingroup\$ @denis I had googled for this before I commented because I my gut toldm those fields might be related to unity but when I saw they were using properties in their examples I wasn't sure anymore... I didn't think it might be because of the serialization. I'm inclined to support your field-decision after all ;-] \$\endgroup\$ – t3chb0t Dec 26 '16 at 9:53
  • \$\begingroup\$ Maybe in the video you've watched they're class was not inheriting from MonoBehaviour or they just didn't want the variable to be initialized from the editor. It really depends if the project is big I'm always using properties even if I need to create 2 variables for the same purpose, but here the project is really small and having a property with a private field would've just added more unnecessary lines.. \$\endgroup\$ – Denis Dec 26 '16 at 10:00
  • \$\begingroup\$ Oh btw there should definitely be a check if the IEnumerable<T> values .Count() > 0 else it will cause infinite loop. \$\endgroup\$ – Denis Dec 26 '16 at 10:52
  • \$\begingroup\$ @denis you're right, sorry, this was the old extension, I posted an improved version. I works without Count. If it could not MoveNext twice then there is nothing to enumerate. \$\endgroup\$ – t3chb0t Dec 26 '16 at 11:01

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.