WinForms dice roller

Question

I am writing a dice roller winforms application using C# 2012 VS. Dice roller is set up for playing Shadowrun tabletop. I feel that there might be too much code going into the GUI, but I am unsure how it should be formatted. I am also very open to general advice. This is my first program I am trying to construct on my own after graduating. I have never attempted a GUI application before.

GitHub

User Control Code:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace DiceRollerWinForms
{
    public partial class DiceRollerUserControl : Form
    {
        private RollDice diceRoll = new RollDice();
        private Roll currentRoll = new Roll();
        private Int32 rollNumber = 1;
        private Int32 currentNumDice = 1;
        private Int32 lastNumHit = 0;
        private Int32 lastNumDiceRolled = 0;
        private string numDiceText = "Number of Dice";

        public DiceRollerUserControl()
        {
            InitializeComponent();
        }

        private void newToolStripMenuItem_Click(object sender, EventArgs e)
        {

        }

        private void aboutToolStripMenuItem_Click(object sender, EventArgs e)
        {

        }

        private void splitContainer1_Panel1_Paint(object sender, PaintEventArgs e)
        {

        }

        private void mainSplitContainer_SplitterMoved(object sender, SplitterEventArgs e)
        {

        }

        private void tableLayoutPanel1_Paint(object sender, PaintEventArgs e)
        {

        }

        private void diceToRollBox_TextChanged(object sender, EventArgs e)
        {
            if (Int32.TryParse(diceToRollBox.Text, out currentNumDice))
            {
                currentNumDice = Int32.Parse(diceToRollBox.Text);
            }
            else
            {
                currentNumDice = 1;
            }
            diceToRollBox.Text = currentNumDice.ToString();
        }

        private void subtractDiceButton_Click(object sender, EventArgs e)
        {
            if (currentNumDice > 1)
            {
                currentNumDice--;
            }
            diceToRollBox.Text = currentNumDice.ToString();
        }

        private void addDiceButton_Click(object sender, EventArgs e)
        {
            if (currentNumDice < 100)
            {
                currentNumDice++;
            }
            diceToRollBox.Text = currentNumDice.ToString();
        }

        private void RollDiceButton_Click(object sender, EventArgs e)
        {
            currentRoll = diceRoll.RollTheDice(currentNumDice, false, false);
            ListViewItem i = new ListViewItem(rollNumber.ToString());
            i.SubItems.Add(currentRoll.numHits.ToString());
            i.SubItems.Add(currentRoll.rawRoll);
            i.SubItems.Add(currentRoll.isGlitch.ToString());
            i.SubItems.Add(currentRoll.isCritGlitch.ToString());
            resultView.Items.Add(i);
            rollNumber++;
        }

        private void RollDiceWithEdgeButton_Click(object sender, EventArgs e)
        {
            currentRoll = diceRoll.RollTheDice(currentNumDice, false, false);
            ListViewItem i = new ListViewItem(rollNumber.ToString());
            i.SubItems.Add(currentRoll.numHits.ToString());
            i.SubItems.Add(currentRoll.rawRoll);
            i.SubItems.Add(currentRoll.isGlitch.ToString());
            i.SubItems.Add(currentRoll.isCritGlitch.ToString());
            resultView.Items.Add(i);
            rollNumber++;
        }

        private void reRollDiceWithEdgeButton_Click(object sender, EventArgs e)
        {
            currentRoll = diceRoll.RollTheDice(currentNumDice, false, false);
            ListViewItem i = new ListViewItem(rollNumber.ToString());
            i.SubItems.Add(currentRoll.numHits.ToString());
            i.SubItems.Add(currentRoll.rawRoll);
            i.SubItems.Add(currentRoll.isGlitch.ToString());
            i.SubItems.Add(currentRoll.isCritGlitch.ToString());
            resultView.Items.Add(i);
            rollNumber++;
        }
        private void resultView_SelectedIndexChanged(object sender, EventArgs e)
        {

        }
    }
}

Other Code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Security.Cryptography;

namespace DiceRollerWinForms
{
    class Roll
    {
        public string rawRoll { get; set; }
        public Int32 numHits {get; set;}
        public bool isGlitch {get; set;}
        public bool isCritGlitch{get; set;}
        public Int32 lastNumDiceRolled { get; set; }
        public Int32 lastNumHitsRolled { get; set; }
        public bool lastRollWasEdge { get; set; }


        internal void FinalRollResults(Int32[] resultsRaw, Int32 numDice)
        {
            Int32[] rollResults = new Int32[6];

            for (Int32 i = 0; i < numDice; i++)
            {
                switch(resultsRaw[i])
                {
                    case 1:
                        rollResults[0]++;
                        break;
                    case 2:
                        rollResults[1]++;
                        break;
                    case 3:
                        rollResults[2]++;
                        break;
                    case 4:
                        rollResults[3]++;
                        break;
                    case 5:
                        rollResults[4]++;
                        break;
                    case 6:
                        rollResults[5]++;
                        break;
                }
            }

            numHits = rollResults[4] + rollResults[5];
            //If more than half the dice you rolled show a one, then you’ve got problems. This is called a glitch.
            if ((numDice / 2) < rollResults[0])
            {
                this.isGlitch = true;

                if (numHits == 0)
                {
                    this.isCritGlitch = true;
                }
            }

            rawRoll = string.Join(",", resultsRaw);

        }
        public Roll()
        {
             //six the number of sides on a dice
            rawRoll = ""; //its a magic number
            numHits = 0;
            isGlitch = false;
            isCritGlitch = false;
        }
    }
    class RollDice : Roll
    {
        private Int32 const_Delay = 0;

        private RNGCryptoServiceProvider RNGProvider = new RNGCryptoServiceProvider();

        public Roll RollTheDice(Int32 numberOfDiceToRoll, bool edgeRoll, bool reRollEdge)
        {
            Roll currentRoll = new Roll();
            Int32[] results = new Int32[numberOfDiceToRoll];



            for (int i = 0; i < numberOfDiceToRoll; i++)
            {
                System.Threading.Thread.Sleep(const_Delay);
                results[i] = RNGDiceRoll(RNGProvider);
            }

            currentRoll.FinalRollResults(results, numberOfDiceToRoll);
            currentRoll.lastNumDiceRolled = numberOfDiceToRoll;
            currentRoll.lastNumHitsRolled = currentRoll.numHits; 
            if (edgeRoll) 
            {
                lastRollWasEdge = true;
            }

            return currentRoll;
        }
        private Int32 RNGDiceRoll(RNGCryptoServiceProvider Provider)
        {
            byte[] arr = new byte[4];
            Int32 rand = 0;
            do
            {
                Provider.GetBytes(arr);
                rand = BitConverter.ToInt32(arr, 0);
            }
            while (rand < 1);
            Int32 roll = (rand % 6) + 1;
            return roll;
        }
    }
}

Answer 1

As an addendum to @retailcoder epic answer I offer...

Object Oriented Dice

Lots of your existing code will simply melt away when we take a more object oriented approach. The key is making classes for the basic things in dice game world and have each class responsible for doing what it is supposed to.

Die Class

A single Dice, that is.

public class Die {
    protected int sides = 6;
    protected Random generate = new Random();

    public int Roll() { return generate.Next(1,(sides+1)); }
}

notes

• This is one 6 sided thing. It can be rolled.
• Random class - See OP's concerns on this

Dice class

We have the concept of "rolling dice." OK then let's make some Dice.

public class Dice {
    List<Die> dice;

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

    public Dice (params Die[] theDice) { // see notes.
        foreach (Die die in theDice) { dice.Add(die); }
    }

    // this is an indexer. see notes.
    public Die this[int i] { get { return dice[i]; } }

    public int Roll() {
        int total = 0;

        foreach (Die die in dice) { total += die.Roll(); }

        return total;
    }

    public int Roll(int thisMany) {
        int total = 0;
        if ( thisMany > dice.Count ) return total;
        if ( thisMany <= 0 )         return total;

        for ( i=0; i < thisMany; i++; ) { total += dice[i].Roll(); }

        return total;
    }

}

notes

• Now rolling a single die is nicely abstracted. The code reads like what it does.
• params allows you to have a variable number of parameters. So we can pass in 1, 2, 10 dice if we want
• An indexer is cool. Read this.

DiceGame

Just a sketch. This could be a craps, yahtzee, lier's dice...

public class DiceGame { 
    protected Dice dice;

    public DiceGame(Dice theDice) { dice = theDice; }

    public void Play() { // TBD
         int total = dice.Roll();  rolling all the dice at once

         // indexer allows individual Rolls.

         int firstDieRoll = dice[0].Roll();
         int secondDieRoll = dice[1].Roll();

         // when I don't care how many there are
         for (int i=0; i<dice.Count; i++) {
             dice[i].Roll();
         }
    }
}

notes

• Everything is in terms of Dice, and that sound logical. DiceGame is nicely abstracted as far as handling dice is concerned.
• The Game deals with Dice only, not individual Dies.
• It does not know how to roll the dice. The dice know, we just tell the dice to do it.
• Dice, in turn, does not know how to Roll individual Dies, we just tell the die to roll.

Inversion of Control

We still need a class to put it all together. But first IOC...

The knee-jerk way is to instantiate the DiceGame, inside of which we "new-up" some Dice, inside of which we make some Die objects. The better way is to invert this construction hierarchy. Make the smallest bits first, then pass them into the constructor of thing it belongs in and so on...

public DiceGameBuilder {
    protected Die die1 = new Die();
    protected Die die2 = new Die();

    protected Dice dice = new Dice(die1, die2); //params doing it's thing

    // DiceGameBuilder actually does not need die1, die2 objects
    // so the constructor call would look like this
    protected Dice dice = new Dice (new Die(), new Die());

    protected DiceGame craps = new DiceGame(dice);

    craps.Play();
}

Inversion of Control allows for dependency injection. By inverting the composition of objects we can inject different objects. It limits where we make changes for these different things. This is very powerful when doing unit testing.

IOC Makes Change Easier

So we're rolling 6-faced dice. What if you wanted to modify your code to also support 8, 12, or 20-faced dice? (I don't know what ShadowRun is, but maybe rules could be configurable?) And who says all dice must have the same number of faces? That makes quite a bunch of hard-coded 6's to fix!

Add a new Die constructor

public Die (int sides = 6) { this.sides = sides; }

Drumroll, please

• Optional constructor parameters :
  • Existing calls with no parameters does not need to change
  • Be sure to document the default behavior! XML comments would be just fine.
• ONLY the Die class is touched.
• Each Die can have a different number of sides
• Without a Die class this kind of modification would be a nightmare
• I did not made a Die class because I knew I was going to make this change. The Die is a logical, meaningful object in the dice game world - that's why.
• Dice needs no modification
• DiceGame needs no modification
• The change is very small because only a Die needs to know how many sides it has.
• The classes tend to be small because each class is responsible for doing it's own thing.
• Methods tend to be small - this is a symptom of well designed classes. Dice.Roll() is 3 lines of code!!
• We built all of this business layer code with zero regard for user interface. That's good.
• You should be able to "drive" the dice game without UI.
• The above points means there will be loose coupling with the UI when you write it.
• By building, driving, testing the business layer first (at least some minimally functioning part) the UI will be that much easier and less buggy.

---

Code Maintenance

This comment about rolling N dice is addressed here.

The lessons learned

• Method overloading is by far cleaner and clearer.
• Adding a new method is less error prone then modifying an existing one.
  • Existing client code calling Roll() is not at risk of breaking.
• Good OO design that "separates concerns", that follows the Single Responsibility Principle, makes change so much better in every way.
• The invisible hand of good design is at work here, just like the above example of modifying the Die constructor.

Answer 2

Don't commit an empty event handler without a comment like // todo: implement "New" toolbar button click - a comment like this would explain why you have such a handler, and why it's empty. Otherwise you're registering an event without doing anything about it and that's just plain YAGNI.

Your private fields are not following established naming conventions. They should be something like this - also if you're going to be referring to the String class with the C# string alias, you should also be referring to Int32 with the C# int alias, for consistency's sake:

    private RollDice _diceRoll = new RollDice();
    private Roll _currentRoll = new Roll();
    private int _rollNumber = 1;
    private int _currentNumDice = 1;
    private int _lastNumHit = 0;
    private int _lastNumDiceRolled = 0;
    private string _numDiceText = "Number of Dice";

Now this one is debatable, but I find explicitly saying what the type of everything is, is an annoyance - use var for implicit typing:

    Int32[] rollResults = new Int32[6];

Becomes

    var rollResults = new int[6];

That way if you decide to change that Int32 for an Int16 or something else, you only have one place to change it. Again, using var whenever possible is only a matter of personal preference... but I prefer that :)

Your class names don't match the file names - RollDice.cs contains class Roll which is sure to cause some trouble at one point or another, not to mention Roll (a verb) is a very bad name to use for a class (a noun).

...and only put 1 class per file, I had to scroll all the way down to find the RollDice class, which is also a non-obvious name, especially when I see another class called DiceRoller where I would expect a Roll() or RollDice() method; I suggest you refactor/rename DiceRoller to DiceRollerApp, since it's your application entry point and that would be clearer that way.

---

That's all I have time to cover for now, I'll edit with more when I have a minute, I think I'll even download your project tonight and see it in action ;)

---

Actual Code Review

Ok so I managed to open up your solution in VS2012 (read: not 2010 as mentioned in both this question and the GitHub repo).

The first, very first thing that strikes me, is this line of code:

Application.Run(new DiceRollerUserControl());

Why does it strike me? Because it's a lie - DiceRollerUserControl isn't a user control, it's a form:

public partial class DiceRollerUserControl : Form

So the fist thing I'm doing - before I even run the app, is rename this class to DiceRollerMainForm. Second thing I'm doing, build the app. And it's failing with some manifest error - I go to the project properties / "Signing" tab and uncheck the "Sign the ClickOnce manifests" box, and then it builds. And... another thing strikes me.

You're using a SplitContainer, and your form is resizable. That's good. Now I realize this isn't WPF, but in terms of layout, I'm sure you could do better:

The DiceRollerMainForm (the renamed DiceRollerUserControl) is tightly coupled with Dice and Roll classes, because of these lines:

    private Dice _diceRoll = new Dice();
    private Roll _currentRoll = new Roll();

But I'll leave that aside for now. Moving on to the event handlers:

    private void diceToRollBox_TextChanged(object sender, EventArgs e)
    {
        if (int.TryParse(diceToRollBox.Text, out _currentNumDice))
        {
            _currentNumDice = int.Parse(diceToRollBox.Text);
        }
        else
        {
            _currentNumDice = 1;
        }
        diceToRollBox.Text = _currentNumDice.ToString();
    }

This is setting the number of dice to be rolled next time we roll the dice - I think setting it to 1 whenever there's a problem with parsing the int value, is a bad decision. Instead, you should give the user some visual cue about the value being invalid, and while the value is invalid the commands that rely on it should be disabled.

    private void subtractDiceButton_Click(object sender, EventArgs e)
    {
        if (_currentNumDice > 1)
        {
            _currentNumDice--;
        }
        diceToRollBox.Text = _currentNumDice.ToString();
    }

    private void addDiceButton_Click(object sender, EventArgs e)
    {
        if (_currentNumDice < 100)
        {
            _currentNumDice++;
        }
        diceToRollBox.Text = _currentNumDice.ToString();
    }

This is the code behind the + and - buttons, respectively for adding and removing a dice. Again, you have a piece of logic here that's burried in an event handler, and using magic numbers: this is begging for a minimum and a maximum dice count constant or configuration setting.

Now there's a bug here: I put a breakpoint on all 3 handlers, and the TextChanged handler is never hit, regardless of what I put in there. So if _currentNumDice is 1 and I enter 10 and then click the + button, _currentNumDice is 2, not 11. And looking at the original code of InitializeComponents() directly on GitHub, the TextChanged event wasn't registered.

So I register it. And now there's another, more subtle bug: again I put a breakpoint on all 3 handlers, and when I click the + button, the handler for that button runs and sets the text in the textbox from 1 to 2, which is intended. But since the textbox text has just changed, the TextChanged event fires and the handler for that event runs, hitting a second breakpoint and then the procedure just re-assigns _currentNumDice to the value it was just assigned with.

So we Roll the dice.

    private void RollDiceButton_Click(object sender, EventArgs e)
    {
        _currentRoll = _diceRoll.RollTheDice(_currentNumDice, false, false);
        ListViewItem i = new ListViewItem(_rollNumber.ToString());
        i.SubItems.Add(_currentRoll.numHits.ToString());
        i.SubItems.Add(_currentRoll.rawRoll);
        i.SubItems.Add(_currentRoll.isGlitch.ToString());
        i.SubItems.Add(_currentRoll.isCritGlitch.ToString());
        resultView.Items.Add(i);
        _rollNumber++;
    }

First thing that hits me here, is why the heck was _currentRoll initialised with = new Roll() in the first place, if we're going to replace it with the return value from _diceRoll.RollTheDice()? I think _currentRoll should be null until the dice are actually rolled. Actually, I don't think it should even be an instance variable / private field: it's only used for adding SubItems to the ListViewItem that populates the resultView. Variables should be as short-lived as possible, the meaning of _currentRoll has no significance whatsoever after the dice have rolled and the results were added to the resultView, and _rollNumber also only has a significant value within that scope, so a first round of refactor could start with this:

    private void RollDiceButton_Click(object sender, EventArgs e)
    {
        var rollIndex = 0;
        var roll = _diceRoll.RollTheDice(_currentNumDice, false, false);
        var item = new ListViewItem(rollIndex.ToString());
        item.SubItems.Add(roll.numHits.ToString());
        item.SubItems.Add(roll.rawRoll);
        item.SubItems.Add(roll.isGlitch.ToString());
        item.SubItems.Add(roll.isCritGlitch.ToString());
        resultView.Items.Add(item);
        rollIndex++;
    }

Now numHits, rawRoll, isGlitch and isCritGlitch are absolutely cryptic to me; I have to go to definition of the Roll class in the Dice.cs file (grrr...) and be lucky enough that you have included these comments:

    numHits = rollResults[4] + rollResults[5]; //hits are the number of dice that rolld a 5 or 6
    //If more than half the dice you rolled show a one, then you’ve got problems. 
    //This is called a glitch.

...And now we have a problem: how does rollResults[4] instinctively means a 5 was rolled"? Let's look at FinalRollResults:

    internal void FinalRollResults(int[] resultsRaw, int numDice)
    {
        var rollResults = new int[6];

        for (var i = 0; i < numDice; i++)
        {
            //is this legit?
            switch(resultsRaw[i])
            {
                case 1:
                    rollResults[0]++;
                    break;
                case 2:
                    rollResults[1]++;
                    break;
                case 3:
                    rollResults[2]++;
                    break;
                case 4:
                    rollResults[3]++;
                    break;
                case 5:
                    rollResults[4]++;
                    break;
                case 6:
                    rollResults[5]++;
                    break;
            }
        }

First off, internal is of no use here, since you have only 1 assembly. Did you mean private? Probably not, since the app doesn't build in that case. I'm making it public. Another thing, FinalRollResults is a very, very bad name for a method - it's a noun, which would be suitable for a class name. But this is a method (verb) and it returns void so the only way to find out what it does, is to read it.

So we're rolling 6-faced dice. What if you wanted to modify your code to also support 8, 12, or 20-faced dice? (I don't know what ShadowRun is, but maybe rules could be configurable?) And who says all dice must have the same number of faces? That makes quite a bunch of hard-coded 6's to fix!

I'll stop here, I think you have enough food for thought as it is!

Key Points

• TableLayoutPanel doesn't play well with buttons and resizable windows. Actually I've never used it for anything that turned out looking good.. but maybe it's just me.
• This is WinForms, make your form look pretty at a specific size, and don't allow resizing.
• The + and - buttons, as well as the textbox, could all be replaced with a NumericUpDown control, which already validates its value so you can't manually enter "hi there" for a value: essentially, you're reinventing the wheel here, and your wheel is square.
• Code-behind should only hold presentation logic, and that's exactly what you've got here. Kudos!
• I see a MenuStrip on your design-view form, but none of it at runtime. Not sure why, but I'm sure that menu is intended to be shown... or is it? There's no code for it!
• Three words: naming, naming, and naming:
  • Class names should be nouns that describe what the object is.
  • Method names should be verbs that describe what the method does.
  • Variable names should be meaningful. Always.

Answer 3

your Do..While threw me for a loop. I saw the While statement and not the Do statement preceding it and wondered where the code was.

you already set the rand variable to something less than 1 before the loop, so you should just write it

while (rand < 1)
{
  Provider.GetBytes(arr);
  rand = BitConverter.ToInt32(arr,0);
}

it is more readable and less code to write.

my Instructors didn't like the Do-While loops, they liked it better when we used the While Loops. it is harder to get into an infinite loop with a while loop as opposed to a do-while

Difference between Do and While

The 2 loops are different.

The Do loop will go through the block then decide whether to go through the loop again based on the while statement. the While loop will test the condition before going through the loop.