# WinForms dice roller

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.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 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());
rollNumber++;
}

private void RollDiceWithEdgeButton_Click(object sender, EventArgs e)
{
currentRoll = diceRoll.RollTheDice(currentNumDice, false, false);
ListViewItem i = new ListViewItem(rollNumber.ToString());
rollNumber++;
}

private void reRollDiceWithEdgeButton_Click(object sender, EventArgs e)
{
currentRoll = diceRoll.RollTheDice(currentNumDice, false, false);
ListViewItem i = new ListViewItem(rollNumber.ToString());
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++)
{
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;
}
}
}


## 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

### 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(); }

}
}


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; }


• 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

The lessons learned

• 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.

.

public class Dice {
public int Roll() { } // don't need to touch this

public int Roll (int thisMany) {
if ( this.Count < thisMany ) return 0;

int total = 0;

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

}
}

• Epic answer yourself! +12 if I could! – Mathieu Guindon Oct 24 '13 at 4:34
• I really appreciate your advice on this. I'll be making changes:) I would like to make a huge note that this: This is one 6 sided thing. It can be rolled. Random class - use the .NET Framework! is absolute garbage for rolling random numbers in something like dice. go check it out its trash... if you check out my repository i compared that to RNG CRYPTO Service and a mersenne twister and the rng crypto was the only one worthy of rolling dice. – Andy Hoffman Oct 24 '13 at 14:43
• Note that this isn't a code review per se, it's really a continuation of my answer, picking up where I left off (I left out any object-oriented approach suggestion on purpose, to leave room for answers like this). One thing about DI: it's also addictive - and when you start having more complex dependencies which have their own dependencies which have their own dependencies, consider looking into an IoC container; take a look at Ninject for example. The IoC container does all the newing-up for you :) – Mathieu Guindon Oct 24 '13 at 15:33
• I would suggest renaming Roll() with RollAll(), and adding RollSome(int) method into your Dice class. Since it is a collection, and we use those verbs when playing a dice game it would be good to have that in there. RollSome could be tricky though if you have a game where you roll different side die (such as Risk 2210) – Robert Snyder Oct 26 '13 at 2:03
• Roll(), RollSome(count), RollSome(count, sides)... whoa! - why not just a Roll(count) overload? Roll() uses all dice and Roll(n) rolls n dice. One way to say "roll the dice". consistent, clean. Also, the RollSome(count,sides) smells like a game rule misplaced as a confusing dice method. Die hitCountDice = new Die(4); hitCountDice.Roll() in a Game class of some kind is far superior to Dice.RollSome(1,4). What in the Wide World of Sports is that supposed to mean? – radarbob Oct 26 '13 at 6:17

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());
_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());
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.
• Thanks for the Advice. I'll make some changes to it which should show up in github later. – Andy Hoffman Oct 22 '13 at 17:20
• Also I am curious to know where you get the naming conventions at. I did a brief look on MS's site to see about that but did not see much in the way of _for private ... – Andy Hoffman Oct 22 '13 at 18:10
• Hmm try downloading the ReSharper demo, it's enforcing them by default. The underscore prefix differenciates private fields from parameters so you can assign _context = context in a constructor, for example. – Mathieu Guindon Oct 22 '13 at 19:11
• Just a warning about R#: it's ADDICTIVE! :) – Mathieu Guindon Oct 22 '13 at 19:12
• AWESOME review. I take issue with saying that TableLayoutPanel is not a good idea. You mentioned WPF and Grid is essentially a TableLayoutPanel, which makes up more than 75% of my user controls in WPF. I've used TableLayoutPanel a few times in a old program I had to fix at work and it scaled my buttons, Labels, and Comboboxes nicely. The program ran on 2 tablets, one with a 9" screen, and the other with a 5" screen with very different resolutions. Just my 2 cents, but I like TableLayoutPanel for winforms, I just don't like winforms any more :) – Robert Snyder Oct 26 '13 at 1:47

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.

• +1 having the loop condition at the beginning of the loop makes it clear what you're getting yourself into. – Mathieu Guindon Oct 22 '13 at 16:28
• 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. – Malachi Oct 22 '13 at 16:32
• True, but most loop conditions can be written so as to put it at the top. Makes it more readable imho. – Mathieu Guindon Oct 22 '13 at 16:39
• I agree completely. I have heard some people say that, even though you can code Do..While loops that they are still bad coding practice unless you absolutely need them. – Malachi Oct 22 '13 at 16:45
• Thanks for the heads up. I had to read that again myself. I'll get that switched around. Still looking for over all comments on the code. I guess in particular the gui stuff and proper design patterns. – Andy Hoffman Oct 22 '13 at 17:06