HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Beginner Dice Roller

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
c#csharpcodereviewstackoverflow
beginnerrollerdice

Problem

dRoll: Beginner Dice Roller

Roll dice base on a input string.

1/. Input.

 d  [/ ]


  • `, int indicate the number of dice.



  • d, delimiter.



  • , int dice value.



  • /, Optional delimiter for dice modifier.



  • , int only with / delimiter



Each valid input can be separated by an ;.

Exemple of input:

"2d8"     Valid
"1d8;1d8" Valid
"1d8/-2"  Valid


Any not valid input is ignored.

2/. Roll rules:

If the rolled value is equals to the die value "Ace". An other roll is add to the die result.

Exemple:

  • "1d4" Roll n°1=4 n°2=3 TotalValue= 7



  • "1d4/-2" Roll n°1=4 n°2=3 TotalValue= 5



A Die 6 is also roll for every roll as Bonus die. resultB

3/. Code

``
class dRoll
{
private string _input;
private List _dSets = new List();

public dRoll(string input) {
_input = input;
ValidateInput();
Run();
}

private void Run() {
foreach(var d in _dSets){
d.Roll();
d.PrintC();
}
}

private void ValidateInput() {

string[] argD = _input.Split(';');

foreach (string arg in argD)
{
if (string.IsNullOrWhiteSpace(arg))
{
continue;
}

string[] dSplit = arg.ToLower().Split('d');
if (dSplit.Length != 2)
{
continue;
}

int numT;
if (!(int.TryParse(dSplit[0], out numT) && numT > 0))
{
continue;
}

int valT; int mod = 0;
if (dSplit[1].Contains('/'))
{
string[] ModT = dSplit[1].Split('/');
if (ModT.Length != 2)
{
continue;
}
if (!(int.TryParse(ModT[0], out valT) && valT > 1))
{
continue;
}
if (!(int.TryParse(ModT[1], out mod) && mod > 0))
{

Solution

There are a couple of things I would have done differently in your code.

-
Validation

  • I suggest you should try and get familiar with regex. I know that it has a steep learning curve and it's difficult to understand at first glance, but it's going to save you a lot of time instead of coding things the alternative way.



-
In your ValidateInput function you do a lot of splitting and int parsing. Would it surprise you that with regex the whole input parsing could be reduced just to 4-5 lines of code?

Check this out:

string Input = "8d2/-4";
Match RegexMatch = Regex.Match(Input, @"^(\d+)d(\d+)(\/-?\d+)?$");
Console.WriteLine(RegexMatch.Groups[1].Value); // Output: 8
Console.WriteLine(RegexMatch.Groups[2].Value); // Output: 2
Console.WriteLine(string.IsNullOrEmpty(RegexMatch.Groups[3].Value) ? "Modifier not available" : RegexMatch.Groups[3].Value.Substring(1)); // Output: -4

string Input = "8d2";
Match RegexMatch = Regex.Match(Input, @"^(\d+)d(\d+)(\/-?\d+)?$");
Console.WriteLine(RegexMatch.Groups[1].Value); // Output: 8
Console.WriteLine(RegexMatch.Groups[2].Value); // Output: 2
Console.WriteLine(string.IsNullOrEmpty(RegexMatch.Groups[3].Value) ? "Modifier not available" : RegexMatch.Groups[3].Value.Substring(1)); // Output: Modifier not available

string Input = "8d2/";
Match RegexMatch = Regex.Match(Input, @"^(\d+)d(\d+)(\/-?\d+)?$");
Console.WriteLine(RegexMatch.Groups[1].Value); // Outputs nothing
Console.WriteLine(RegexMatch.Groups[2].Value); // Outputs nothing
Console.WriteLine(string.IsNullOrEmpty(RegexMatch.Groups[3].Value) ? "Modifier not available" : RegexMatch.Groups[3].Value.Substring(1)); // Output: Modifier not available

string Input = "8d2/abcd";
Match RegexMatch = Regex.Match(Input, @"^(\d+)d(\d+)(\/-?\d+)?$");
Console.WriteLine(RegexMatch.Groups[1].Value); // Outputs nothing
Console.WriteLine(RegexMatch.Groups[2].Value); // Outputs nothing
Console.WriteLine(string.IsNullOrEmpty(RegexMatch.Groups[3].Value) ? "Modifier not available" : RegexMatch.Groups[3].Value.Substring(1)); // Output: Modifier not available


-
Rolling

  • In your Roll function, you stop the for loop by setting NAce to false. While this solution works, there is definitely a cleaner one. The break statement terminates the closest enclosing loop, so you will exit the for loop immediately and won't need two booleans dedicated to stop the loop.



  • When the bonus Die 6 is rolled, you request a random number with random.Next(1, 6);. A regular Die 6 has six sides and six values, but the Next function of your random class will just return numbers from 1 to 5. 6 is going to be left out, because that is how the Random class in C# works. Your number is going to be greater or equal to the minimum value and less than the maximum value.



-
Coding style

  • This doesn't influence the way things will work, but will result in a cleaner code.



  • You should either use variable names that speak for themselves or document the purpose of your variables. It is hard to decode one's function with variables that make no sense to us.



  • When an if, for, while or using statement is followed by a single action, no scoping ( { and } ) is needed.



  • Lambda expressions have been introduced in C# 3.0. One particular is the forEach function for List<> that implemented the old foreach statement in a formal manner.



-
With C# 6.0 expression bodied functions and properties have been introduced. You can implement read-only (getter only) properties using expressions.

Instead of

public int Ntot {
    get {
        return resultN.Sum(x => x.Item2);
    }
}


you could write just

public int Ntot => resultN.Sum(x => x.Item2);

Code Snippets

string Input = "8d2/-4";
Match RegexMatch = Regex.Match(Input, @"^(\d+)d(\d+)(\/-?\d+)?$");
Console.WriteLine(RegexMatch.Groups[1].Value); // Output: 8
Console.WriteLine(RegexMatch.Groups[2].Value); // Output: 2
Console.WriteLine(string.IsNullOrEmpty(RegexMatch.Groups[3].Value) ? "Modifier not available" : RegexMatch.Groups[3].Value.Substring(1)); // Output: -4

string Input = "8d2";
Match RegexMatch = Regex.Match(Input, @"^(\d+)d(\d+)(\/-?\d+)?$");
Console.WriteLine(RegexMatch.Groups[1].Value); // Output: 8
Console.WriteLine(RegexMatch.Groups[2].Value); // Output: 2
Console.WriteLine(string.IsNullOrEmpty(RegexMatch.Groups[3].Value) ? "Modifier not available" : RegexMatch.Groups[3].Value.Substring(1)); // Output: Modifier not available

string Input = "8d2/";
Match RegexMatch = Regex.Match(Input, @"^(\d+)d(\d+)(\/-?\d+)?$");
Console.WriteLine(RegexMatch.Groups[1].Value); // Outputs nothing
Console.WriteLine(RegexMatch.Groups[2].Value); // Outputs nothing
Console.WriteLine(string.IsNullOrEmpty(RegexMatch.Groups[3].Value) ? "Modifier not available" : RegexMatch.Groups[3].Value.Substring(1)); // Output: Modifier not available

string Input = "8d2/abcd";
Match RegexMatch = Regex.Match(Input, @"^(\d+)d(\d+)(\/-?\d+)?$");
Console.WriteLine(RegexMatch.Groups[1].Value); // Outputs nothing
Console.WriteLine(RegexMatch.Groups[2].Value); // Outputs nothing
Console.WriteLine(string.IsNullOrEmpty(RegexMatch.Groups[3].Value) ? "Modifier not available" : RegexMatch.Groups[3].Value.Substring(1)); // Output: Modifier not available
public int Ntot {
    get {
        return resultN.Sum(x => x.Item2);
    }
}
public int Ntot => resultN.Sum(x => x.Item2);

Context

StackExchange Code Review Q#158312, answer score: 2

Revisions (0)

No revisions yet.