7
\$\begingroup\$

I've previously came up with the question "Rolling dice in a method chain" and was interested if this could be improved. It could, so now I have this class:

public class DiceQueue : IEnumerable<int>
{
    public int Sides { get; }
    internal Random Rnd = new Random();
    public DiceQueue(int sides) { Sides = sides; }

    public int Roll() => Rnd.Next(Sides) + 1;
    public IEnumerator<int> GetEnumerator() { while (true) { yield return Roll(); } }
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

Very simple: just create a queue by calling the constructor with a number of sides and then it can be used as an infinite enumerator. But generally, you just want to take an X amount of values from the queue at once, or even just roll a single die. But this class isn't that interesting, as I want to do statistics with random rolls!
So, hence this class:

public class Statistics : IEnumerable<Point<long>>
{
    public string Name { get; private set; }
    public long Sum { get; private set; } = 0;
    public long Total { get; private set; } = 0;
    public long Min { get; private set; } = long.MaxValue;
    public long Max { get; private set; } = long.MinValue;
    public double Average => 1.0 * Sum / Total;
    public double TotalOf(int value) => Totals.ContainsKey(value) ? 100.0 * Totals[value] / Total : 0.0;
    protected Dictionary<long, long> Totals { get; set; } = new Dictionary<long, long>();
    public event EventHandler<StatisticsEventArgs> Added;
    protected virtual void OnAdded(StatisticsEventArgs e) { Added?.Invoke(this, e); }
    public Statistics() : this("No name") { }
    public Statistics(string name) { Name = name; }
    public Statistics Add(long value)
    {
        if (!Totals.ContainsKey(value)) { Totals.Add(value, 0); }
        Totals[value] += 1;
        Total++;
        Sum += value;
        if (Max < value) { Max = value; }
        if (value < Min) { Min = value; }
        OnAdded(new StatisticsEventArgs(this, value));
        return this;
    }
    public IEnumerable<string> Report()
    {
        yield return $"Report for: {Name}";
        yield return $"The total sum is {Sum} for {Total} values for an average value of {Average:0.00}.";
        yield return $"The values are all between {Min} and {Max}.";
        yield return "List of values";
        foreach (var point in this) { yield return $"* Value {point.X} occurred {point.Y} times: {100.0 * point.Y / Total:00.0}%."; }
        yield return new string('-', 40);
    }
    public override string ToString() => string.Join(Environment.NewLine, Report());
    public IEnumerator<Point<long>> GetEnumerator()
    {
        foreach (var total in Totals.OrderBy(pair => pair.Key)) { yield return new Point<long> { X = total.Key, Y = total.Value }; }
    }
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

It has a name and you just add values to it and it will keep various statistics about it. The Report() method will generate a textual report of the summary but it's mainly used for the ToString() method. Interesting way to make multiline reports, btw.
It also has an event which gets triggered whenever a new value is added, allowing the system to display a running total.
I have to show a few more types, though. First:

public class Point<T>
{
    public T X { get; internal set; }
    public T Y { get; internal set; }
}

Not really rocket science. And while C# does have a Point datatype somewhere, I wanted one that's more generic...

public class StatisticsEventArgs : EventArgs
{
    public Statistics Stats { get; }
    public long LastValue { get; }
    public StatisticsEventArgs(Statistics stats, long lastValue)
    {
        Stats = stats;
        LastValue = lastValue;
    }
}

Yeah, well... The event requires an argument. This one should be it. Trying to follow the standard for events here.
But I want to use the statistics in a chainable way and I want to do statistics on rolls of multiple dice, so I also have these extension methods:

public static class Toolkit
{
    public static IEnumerable<T> Do<T>(this IEnumerable<T> data, Action<T> action)
    {
        foreach (var item in data)
        {
            action(item);
            yield return item;
        }
    }
    public static IEnumerable<List<T>> TakeGroup<T>(this IEnumerable<T> data, int count)
    {
        while (true) { yield return data.Take(count).ToList(); }
    }
}

The Do() method just does some action with a value from an enumerator before passing the value on to the next method. Maybe not elegant, but I have no other options here, as far as I know.
The TakeGroup() method will grab an X amount of values and return them as a list, and keeps doing this until infinity. Too bad that this might cause trouble if the base enumeration is limited so I need to work out a solution for that. A Partitioner, perhaps? Well, still have to work that out but this works for my current purpose...
Then something to test the whole thing.

public static class StatisticsTest
{
    public static void Execute(int count)
    {
        Console.WriteLine($"We've rolled for a total of {D6X3RollQueue.Take(count).Count()} times.");
        Console.WriteLine(D6Stat);
        Console.WriteLine(D6X3MinStat);
        Console.WriteLine(D6X3MaxStat);
        Console.WriteLine(D6X3MinMaxStat);
        Console.WriteLine(D6X3SumStat);
    }
    private static readonly DiceQueue D6 = new DiceQueue(6);
    private static readonly Statistics D6Stat = new Statistics("Rolling d6");
    private static readonly Statistics D6X3MinStat = new Statistics("Minimum of 3D6 (1 to 6)");
    private static readonly Statistics D6X3MaxStat = new Statistics("Maximum of 3D6 (1 to 6)");
    private static readonly Statistics D6X3MinMaxStat = new Statistics("Sum(Min+Max) of 3D6 (2 to 12)");
    private static readonly Statistics D6X3SumStat = new Statistics("Sum of 3D6 (3 to 18)");
    private static readonly IEnumerable<int> D6RollQueue = D6.Do(d => D6Stat.Add(d));
    private static readonly IEnumerable<int> D6X3RollQueue = D6RollQueue.TakeGroup(3)
                                                                        .Do(d => D6X3MinStat.Add(d.Min()))
                                                                        .Do(d => D6X3MaxStat.Add(d.Max()))
                                                                        .Do(d => D6X3MinMaxStat.Add(d.Min() + d.Max()))
                                                                        .Select(d => d.Sum())
                                                                        .Do(d => D6X3SumStat.Add(d));

}

This shows why I want a dice queue. The queue allows me to collect various statistics for all the rolls made while I can roll as many times as I like. By declaring a queue like D6RollQueue I can take a single roll by using D6RollQueue.First() or an X amount of rolls by using `D6RollQueue.Take(X)' and then do something with the result, while the queue keeps track of all statistics. So when I create some dice game, I can just roll as often as I like while having various statistics about the rolls.
And the TakeGroup() method will allow me to maintain statistics about groups of rolls like rolling three dice and keep statistics about the minimum and maximum values.

So, go ahead and shoot at it. Can it be improved?
(It's not that important that the Random generator might be predictable.)

\$\endgroup\$
  • 3
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Nov 14 at 17:37
4
\$\begingroup\$
  • If you need to get a value of a Dictionary<TKey, TValue> you shouldn't use ContainsKey() together with the Item property getter but TryGetValue(), because by using ContainsKey() in combination with the Item getter you are doing the check if the key exists twice.
    From the refernce source

    ContainsKey(TKey)

    public bool ContainsKey(TKey key) {
        return FindEntry(key) >= 0;
    }
    

    this[TKey]

    public TValue this[TKey key] {
        get {
            int i = FindEntry(key);
            if (i >= 0) return entries[i].value;
            ThrowHelper.ThrowKeyNotFoundException();
            return default(TValue);
        }
        set {
            Insert(key, value, false);
        }
    }
    

    TryGetValue(TKey, out TValue)

    public bool TryGetValue(TKey key, out TValue value) {
        int i = FindEntry(key);
        if (i >= 0) {
            value = entries[i].value;
            return true;
        }
        value = default(TValue);
        return false;
    }
    

    Each of these public methods is calling the private FindEntry(TKey). As you see a combination of ContainsKey() together with the Item getter is doing the call twice.

    private int FindEntry(TKey key) {
        if( key == null) {
            ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
        }
    
        if (buckets != null) {
            int hashCode = comparer.GetHashCode(key) & 0x7FFFFFFF;
            for (int i = buckets[hashCode % buckets.Length]; i >= 0; i = entries[i].next) {
                if (entries[i].hashCode == hashCode && comparer.Equals(entries[i].key, key)) return i;
            }
        }
        return -1;
    }
    

    So this

    if (!Totals.ContainsKey(value)) { Totals.Add(value, 0); }
    Totals[value] += 1;  
    

    should be written like this

    Totals.TryGetValue(value, out long current);
    Totals[value] = current + 1;
    

    But just looking at the above code, it just seems strange to have ContainsKey(value). It would be better to rename value to something else.

  • I don't like such oneliners shown in the code. IMO it is harder to read the code and grasp at first glance what it is about.

  • The extension methods could use some improvement as well. You should add proper argument validation into the methods. Nobody wants to get a NullReferenceException out of a public method.

  • The Name property doesn't need a setter because you are assigning a value only in the constructor.

\$\endgroup\$
  • \$\begingroup\$ I don't like to use TryGetValue() in this case as I need to create new items for the dictionary. TryGetValue() requires me to declare an extra variable. So, this perhaps? if (Totals.ContainsKey(value)) { Totals[value] += 1; } else { Totals.Add(value, 1); } \$\endgroup\$ – Wim ten Brink Nov 14 at 16:02
  • \$\begingroup\$ You are creating new items by the way I showed and as a bonus this part will run faster as well. \$\endgroup\$ – Heslacher Nov 14 at 16:05
  • \$\begingroup\$ As for oneliners... We have different preferences as I like oneliners. It keeps the amount of lines shorter so more lines fit on the screen and they're generally very short functions so they would be 4 lines otherwise. (Plus a blank line.) But as I said, it's a personal preference... \$\endgroup\$ – Wim ten Brink Nov 14 at 16:06
4
\$\begingroup\$
  • The number of times a given value has been added to a statistics batch shouldn't be a long, it should be an int (maybe unsigned). (I was mistaken about what long was).
  • That would seem to break your Point implementation, but you probably shouldn't be using that in the first place. The items in question aren't "points"; they don't spatially relate to each other. Just use tuples.
  • The DiceQueue class yields ints, so it would be ideal if Statistics also handled ints. Can you make Statistics generic across numerical types?
  • I suspect that you can shorten/simplify a lot of this using Linq, maybe to the point where it no longer needs to be wrapped up in classes the way you have it. But I don't know what that would look like exactly.

It's hard to talk about the ideal way to do what you're trying to do without knowing what you're working toward. How will these tools be used?

\$\endgroup\$
  • \$\begingroup\$ Well, the statistics are meant to be shown in a graph, so that makes them points, not tuples. After all, the statistics for the 3D6 should display a bell curve. Points could use int instead, but sum might overflow int.MaxValue so that's why I use long. Making the Statistics class more generic would be nice, but as I need T.MinValue and T.MaxValue, it would be challenging to pick a base type. Any suggestions on how to make it more numerically generic? \$\endgroup\$ – Wim ten Brink Nov 14 at 18:14
  • \$\begingroup\$ As for what I'm trying to do, simple: I'm building a library with various functions and this one is for statistical purposes. I'm also working on linear regression and other statistics that could be used in similar method chains. The method chain is technically Linq, but I want statistics to be part of any Linq query without having to walk through a list multiple times. The statistics are technically running totals. \$\endgroup\$ – Wim ten Brink Nov 14 at 18:18
  • 1
    \$\begingroup\$ I guess there's no such thing as an INumerical in C#; pity. \$\endgroup\$ – ShapeOfMatter Nov 14 at 18:35
  • \$\begingroup\$ One suggestion, every numerical type, except perhaps BigInteger, can convert to double. That can be used to make sure you have a numerical type. \$\endgroup\$ – tinstaafl Nov 14 at 18:50
  • \$\begingroup\$ @tinstaafl True, I could use doubles. But floats are a bit harder to compare due to rounding errors. In floats, 1/3 and 2/6 can be different due to the binary representation. This makes the dictionary I use a bit more challenging. I tend to avoid float types, if possible, because of potential comparison problems... \$\endgroup\$ – Wim ten Brink Nov 14 at 21:54

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.