HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Custom collection implementing IList<T> saving current, next and previous element

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
ilistpreviousandcollectionnextcustomelementcurrentsavingimplementing

Problem

I'm developing a Tic Tac Toe game where you can change your pawn look

Now as You can see it has 2 buttons one for next one for previous, and You can also pick if you want to edit the O or the X sprite. Now here comes the custom collection I made and the reason for that is because I need something that can remember what was the last element shown/accessed. It's really similar to how IEnumerator works, but the problem is that IEnumerator cant move backwards, which is some functionality I need. It's my first time creating my own collection, so any flaws or ways to improve it are appreciated (and maybe a better name for it..). I also added the AddRange() function and also a converter ToIterable().

```
public class IterableList : IList
{
private readonly IList _iterableList = new List();

private int lastUsedElementIndex;

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

public T MovePrevious
{
get
{
int temp = lastUsedElementIndex;
lastUsedElementIndex = lastUsedElementIndex - 1 = _iterableList.Count)
{
return _iterableList[0];
}
return _iterableList[lastUsedElementIndex + 1];
}
}

public IterableList(int StartingIterableIndex = 0)
{
lastUsedElementIndex = StartingIterableIndex;
}

public IterableList AddRange(IList collection, int startingIterableIndex)
{
IterableList iterableCollection = new IterableList(startingIterableIndex);
foreach (var item in collection)
{
iterableCollection.Add((T)item);
}
return iterableCollection;
}

public IterableList AddRange(IList collection, int startingIterableIndexn)
{
IterableList iterabl

Solution


  • The getter of the properties MoveNext and MovePrevious change the internal state of the class. That is a really uncommon behavior. I would suggest you change the properties to methods.



  • The method AddRange creates a new list, but doesn't use the items of the list where AddRange was called. I would rename it to Create and make it static or remove that method and provide an appropriated constructor instead.



Actually, your use case does not require to sublcass a list. You need something that works on a list. Lets call it CircularListNavigator:

public class CircularListNavigator
{
    private readonly IList _internalList;
    public ListNavigator(IList list)
    {
        _internalList = list;
    }

    public TItem Current { get { ... } }
    public void MoveNext() { ... }
    public void MovePreviouse() { ... }
}

Code Snippets

public class CircularListNavigator<TItem>
{
    private readonly IList<TItem> _internalList;
    public ListNavigator(IList<TItem> list)
    {
        _internalList = list;
    }

    public TItem Current { get { ... } }
    public void MoveNext() { ... }
    public void MovePreviouse() { ... }
}

Context

StackExchange Code Review Q#137968, answer score: 5

Revisions (0)

No revisions yet.