WinForms dice roller

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.

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.