Simon says in Unity

I've made the Simon says game in Unity

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. I also have a question about it here

```
public class CircularListNavigator : IList
{
private readonly IList _circularListNavigator = new List();

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 source, int startingIterableIndex = 0)
{
_circularListNavigator = source.ToCircularListNavigator();
lastUsedElementIndex = startingIterableIndex;
}

public CircularListNavigator ConvertToCircularListNavigator(IEnumerable collection, int startingIterableIndex)
{
CircularListNavigator iterableCollection = new CircularListNavigator(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 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);

private readonly IList _circularListNavigator = new List();

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 number = 0;
    for (int i = 0; i  Buttons.Length - 1)
        {
            number = 0;
        }
    }
    indexOrder = new CircularListNavigator(Shuffle(indexOrder));
    StartCoroutine(AwaitShowPattern());
}

You use the the CircularListNavigator twice here but you discard the first one. I think using a List 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 TakeOrRepeat(this IEnumerable values, int count)
{
    var counter = 0;
    var enumerator = values.GetEnumerator();
    var moveNext = new Func(() =>
    {
        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(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 Shuffle(IList array)

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

public static CircularListNavigator ToCircularListNavigator(this IEnumerable collection)
{
    CircularListNavigator circularListNavigator = new CircularListNavigator();
    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.AddRange(..) or even better use the new List(..) constructor to initilize it from the source.

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

StackExchange Code Review Q#150829, answer score: 3