1
\$\begingroup\$

Anything that could be done better here?

public class ShuffleBag<T> : IEnumerable<T>
{
    private readonly int m_endIndex;
    private readonly FastRandom m_rng;
    private readonly T[] m_values;

    private int m_currentIndex;

    public ShuffleBag(T[] values, ulong state, ulong? stream = null) {
        m_currentIndex = 0;
        m_endIndex = (values.Length - 1);
        m_rng = (stream.HasValue ? new FastRandom(state, stream.Value) : new FastRandom(state));
        m_values = values;
    }
    public ShuffleBag(T[] values) : this(values, SecureRandom.GetUInt64()) { }

    IEnumerator IEnumerable.GetEnumerator() {
        return GetEnumerator();
    }
    public IEnumerator<T> GetEnumerator() {
        var currentIndex = m_currentIndex;
        var endIndex = m_endIndex;
        var rng = m_rng;
        var values = m_values;

        while (currentIndex <= endIndex) {
            var randomIndex = rng.NextInt32(currentIndex, endIndex);
            var temp = values[randomIndex];

            values[randomIndex] = values[currentIndex];
            values[currentIndex] = temp;

            m_currentIndex = currentIndex++;

            yield return temp;
        }
    }
}

For those that might inquire, here's the source for FastRandom and SecureRandom.

\$\endgroup\$
  • \$\begingroup\$ Quick observation - I see three different brace styles. Standardize on one. C# de-facto standard is opening and closing braces on their own lines. But that's not important - consistency is. So choose and go forth. \$\endgroup\$ – Jesse C. Slicer Apr 12 '18 at 21:50
  • \$\begingroup\$ @JesseC.Slicer I hesitate to get into the braces thing but will say that I wholeheartedly agree about consistency. The style above is enforced by the IDE automatically; have personally found that defining "classes" of braces in the reformatter helps me parse code easier/quicker (can easily be undone before checking into a repo). \$\endgroup\$ – Kittoes0124 Apr 12 '18 at 22:44
1
\$\begingroup\$

Enumerate once

I'm not sure whether you intended to write this to be iterable only once. Is this as designed?

[TestMethod]
public void CodeReview()
{
    var bag = new ShuffleBag<string>(new[] { "a", "b", "c", "d" });

    var e1 = bag.ToList();  // randomized: "b", "a", "c", "d"
    var e2 = bag.ToList();  // last element: "d"
    var e3 = bag.ToList();  // last element: "d"
}

Collections interfaces

I would let the bag inherit IReadOnlyCollection<T> to provide eager access to the bag's size:

public int Count => m_values.Length;
\$\endgroup\$
  • \$\begingroup\$ I did not intend for that bug but believe it has already been fixed. \$\endgroup\$ – Kittoes0124 Jul 27 at 16:55
  • \$\begingroup\$ Ah, you are the one that uses that New static factory method. I remember from a question about native code. I've never seen that naming convention for a factory used before. Does it come from another language? \$\endgroup\$ – dfhwze Jul 27 at 16:59
  • 1
    \$\begingroup\$ It was a technique that I lifted from Java; one can pass constructors around like any other function using MyClass::new. AFAIK, C# doesn't support anything like this so I came up with static T New as an acceptable alternative. One goes a step further and always makes the actual constructors private in order to force "proper usage" upon consumers (generally just myself). \$\endgroup\$ – Kittoes0124 Jul 27 at 18: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.