4
\$\begingroup\$

Hello Everyone!

Fellow human beings, today I present to you my dice rolling script! Behold, in my amateurish attempt at some raw C# code. Perhaps this will remain here regardless of the dozens of other dice rolling scripts out there.

Contains a 'for' loop, a 'switch', and a method that returns a value.

using System;

namespace C_
{
    class RollDice : Program
    {


        //Variables

        int numOfDice;
        int numOfSides;

        int maxDice = 100;
        int minDice = 1;
        int maxSides = 100;
        int minSides = 1;
        int totalRolled;

        //Start method for number of Dice to throw - take the player input in InputMethod() which returns an int. If returning 0, then restart. Clamp the value between min/max. Continue to next function.

        public void GetDice()
        {
            Console.WriteLine("Welcome to Roll Dice!");
            Console.WriteLine("How many dice do you want to throw?");
            numOfDice = InputMethod();
            if(numOfDice == 0)
            {
                GetDice();
            }
            if (numOfDice > maxDice || numOfDice < minDice)
            {
                numOfDice = Math.Clamp(numOfDice, minDice, maxDice);
                Console.WriteLine($"Number of Dice set to {numOfDice}");
            }

            GetSides();
        }

        // Get the sides of each dice by using a similar process. Roll() for each dice, then sum the totals in RollTotal().

        public void GetSides()
        {
            Console.WriteLine("How many sides does each dice have?");
            numOfSides = InputMethod();
            if(numOfSides == 0)
            {
                GetSides();
            }
            if (numOfSides > maxSides || numOfSides < minSides)
            {
                numOfSides = Math.Clamp(numOfSides, minSides, maxSides);
                Console.WriteLine($"Number of Sides set to {numOfSides}");
            }

            for (int x = 1; x <= numOfDice; x++)
            {
                Roll();
            }

            RollTotal();
        }


        // Create a new Random() instance for the number of sides and add it to the totalRolled variable.

         public void Roll()
        {
            Console.WriteLine(".....");
            Console.WriteLine("Rolling");
            Random r = new Random();
            var result = r.Next(1, numOfSides + 1);
            Console.WriteLine($"You rolled {result}");
            totalRolled += result;
        }
        
        // Output the total and prompt to roll again or quit. Reset the total..

        public void RollTotal()
        {
            Console.WriteLine($"Total sum rolled is {totalRolled}");
            Console.WriteLine(" ");
            Console.WriteLine("Enter 1 to re-roll or 2 to quit.");
            var answer = InputMethod();

            switch (answer)
            {
                case 0:
                RollTotal();
                break; 
            
                case 1:
                totalRolled = 0;
                GetDice();
                break;           
            
                case 2:
                totalRolled = 0;
                Environment.Exit(0);
                break;
            
                default:
                Environment.Exit(0);
                break;
            }
        }

        //Convert the input to an integer if valid and return the value. Else return 0.

        public int InputMethod()
        {
            var input = Console.ReadLine();
            if(!Int32.TryParse(input, out int convertedStringToInt))

            {
                Console.WriteLine("You need to enter a valid number!");
                return 0;
            }
            
            else 
            
            {
                return Convert.ToInt32(input);
            }

        }

    }
}
\$\endgroup\$
2

3 Answers 3

4
\$\begingroup\$

I have few tips:

  • Infinite recursion.
    The app would never finish until you call Environment.Exit because you have infinite recursion. The result of the infinite recursion is always StackOveflowException, sooner or later. Take the trigger: if you forced to use Environment.Exit to stop the app, then something went wrong. About correct usage of the recursion: on some level of calling the recursive method must return not make a recursive call again. Then the recursion would finish normally. Btw, you need no recursion here.

  • Naming
    Give all the items: classes, namespaces, variables, fields, etc. names that explains its purpose or containing value. It would make the code maintaneable. Also I use the following standard.

    private int _fieldName;
    
    public int PropertyName { get; set; }
    
    public void DoSomething()
    {
        int localVariableName = 0;
    }
    

    It allows explicitly differ fields and local variables in a code even if I'm reading the code outside of IDE editor.

  • Incapsulation. The OOP principle that helps you in development to avoid popular mistakes. What method should I call when I create an instance of RollDice class? All the methods are public. Leave public only that class components which you allow to access outside of the class.

  • Small tips.
    Remove inheritance class RollDice : Program, it does nothing. Let the methods and classes do some single job. (see S in SOLID principles for details).

Here's some alternative implementation of the same to play.

class Program
{
    static void Main(string[] args)
    {
        RollDice dice = new RollDice();
        dice.Run();
        Console.ReadKey();
    }
}

public class RollDice
{
    private const int maxDice = 100;
    private const int minDice = 1;
    private const int maxSides = 100;
    private const int minSides = 2;

    private Dice[] _dices;

    private List<string> _menuItems = new List<string>
    {
        "Exit.",
        "Setup Roll Dice.",
        "Roll!"
    };

    public void Run()
    {
        Console.Write("Welcome to Roll Dice!");
        while (true)
        {
            ShowMenu(_menuItems);
            switch (ConsoleHelper.ReadInteger("Enter your selection: ", 0, _menuItems.Count - 1))
            {
                case 1:
                    Setup();
                    break;
                case 2:
                    Roll();
                    break;
                case 0:
                    Console.WriteLine("Bye.");
                    return;
                default:
                    Console.WriteLine("Error: Unexpected menu input!"); // this would never happen
                    break;
            }
        }
    }

    private void Roll()
    {
        if (_dices == null)
        {
            Console.WriteLine("Please setup the Roll Dice first.");
            return;
        }

        Console.WriteLine("Rolling...");
        for (int i = 0; i < _dices.Length; i++)
        {
            _dices[i].Roll();
        }
        Console.WriteLine($"You rolled: {string.Join(", ", (object[])_dices)}");

        int total = 0;
        for (int i = 0; i < _dices.Length; i++)
        {
            total += _dices[i].Value;
        }
        Console.WriteLine($"Total sum rolled is {total}");
    }

    private void Setup()
    {
        int diceCount = ConsoleHelper.ReadInteger("How many dice do you want to throw? ", minDice, maxDice);
        int sides = ConsoleHelper.ReadInteger("How many sides does each dice have? ", minSides, maxSides);
        _menuItems[1] = $"Change Roll Dice setup. (currently {diceCount} dice of {sides} sides).";
        _dices = CreateDice(diceCount, sides);
    }

    private Dice[] CreateDice(int diceCount, int sides)
    {
        Dice[] dices = new Dice[diceCount];
        for (int i = 0; i < diceCount; i++)
        {
            dices[i] = new Dice(sides);
        }
        return dices;
    }

    private void ShowMenu(List<string> items)
    {
        Console.WriteLine();
        for (int i = 1; i < items.Count; i++)
            Console.WriteLine($"[{i}] {items[i]}");
        Console.WriteLine($"[0] {items[0]}");
    }
}

public class Dice
{
    private static Random rndGen = new Random(); // one random generator for all dices would be enough

    private int _sides;

    public int Value { get; private set; }

    public Dice(int sides)
    {
        _sides = sides;
    }

    public void Roll()
    {
        Value = rndGen.Next(0, _sides) + 1;
    }

    public override string ToString()
    {
        return $"[{Value}]";
    }
}

public static class ConsoleHelper
{
    public static int ReadInteger(string prompt, int min, int max)
    {
        while (true)
        {
            Console.Write(prompt);
            if (int.TryParse(Console.ReadLine(), out int option) && option >= min && option <= max)
                return option;
            Console.WriteLine($"You need to enter a valid number from {min} to {max}!");
        }
    }
}

Note for other reviewers: I'm not including const or readonly warnings in the review to leave it as simple as possible.

No recursion above.

\$\endgroup\$
4
\$\begingroup\$

Feedback points listed in order that I come across them.

Method names

The method names are not descriptive. What does InputMethod() do? In this case, ReadInteger() would've been a much better name that explains exactly what it does.

Whenever you try to name a method, look at how you would use it. That is to say, don't look at the method body. Explain it to me in words, without either of us seeing the code.

Method signature

You're using class fields and are relying on them to pass your data, and your methods take in no parameters. That's not a great approach.

To be fair, it is not always easy to define the line between what should be a class field and what should be a parameter. But there are a few clear cut cases in your code, such a RollTotal(), which is blindly relying on the totalRolled field to be set before the method is called.

When your method relies on a specific value being set, then it's a good chance that this should've been a method parameter. Another great example here is Roll. What are you rolling? It's very weird to call the same method many times and conceptually expect it to perform a "different" roll.

This roll logic gets into an OOP concept I will touch on later.

Recursion

Aepot already addressed this in their answer, but you should really avoid recursion here. While it probably won't matter for the amount of times you'll roll some dice, every time you recurse you go another level deeper, and the runtime can only go so deep. At some point, your application will crash.

Where possible, favor regular loops over recursion to prevent this issue. Your intended logic can easily be rewritten as a loop, which avoids the stack overflow exception entirely without changing the application behavior.

New random

You're creating a new random for each roll. This is a common mistake. It's much better to keep the same Random object and reuse it for your re-rolls.

public void Roll()
{
    Random r = new Random();
    var result = r.Next(1, numOfSides + 1);
}

You were overusing class fields, but funnily enough you missed this one :)

Inheritance

class RollDice : Program

You're not using this inheritance for anything, and it doesn't quite make sense, so just remove it.

Inheritance is not a small beast to tackle, and it seems you're on your first steps of OOP, so I suggest not using it until you've got a grasp on basic OOP usage first.

Don't expose your privates

Or, less wittily, don't make everything public by default. You only want to make public what you want other classes to be able to call directly. For your logic, there should only be one public entrypoint, and everything else happens during that method (in private submethods).

For a beginner, I recommend specifically making everything private, and only making things public when you need them to be. And for everything you make public, really evaluate if this is something you want outside forces to be able to control.
Using a real world example, bank applications have a "add money to account" method somewhere, but they very clearly don't want just anyone to be able to access it. You need to think similarly defensively about what you make public.

OOP and classes

Big title because big section.

The very first step of using OOP is to learn how to let go of static code (mostly). It seems like that's what you've done here, especially since you explicitly call it out as "a non-static C# class".

But your current code is still using a single class to handle everything. While technically not static, it's still a god class. This is the next step of learning OOP: breaking your logic down into separate classes, each with their responsibility.

And you actually have a prime example here, i.e. Die.

public class Die
{
    private static int MaxSides = 100;
    private static int MinSides = 1;

    private readonly int sides;
    private readonly Random rand;

    public Die(int sides)
    {
        this.sides = Math.Clamp(sides, MinSides , MaxSides);
        this.Rand = new Random();
    }

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

This approach immediately yields benefits. Not just in a much easier handling logic in your other class (which we'll get into), but I've also added a sneaky feature: every Die can have a different number of faces.
This might not be a feature that you need, but it is a very good example on the benefits of OOP and how to use it, so I couldn't pass it up for the sake of example here.

Note that we're still using static here for minor global values, in this case the range limits on the amount of sides a die can have. This is not a per-die setting, and so it shouldn't be a class field/property. This is why I said that OOP is about mostly letting go of static, but not fully.

Also note that I chose to give each Die its own Random. I was considering just using one Random and passing it into the Roll() method as a parameter, but I prefer this because it keeps the Die logic cleaner to use (and there are some additional advanced benefits that don't really matter to you yet).

I am going to slightly alter the flow of your application based on each die having a different amount of faces, but I want you to mostly focus here on how we create Die objects, how we store them, and how we roll the dice.

Dice creation

private static int MaxDice = 100;
private static int MinDice = 1;

private readonly List<Die> Dice = new List<Die>();

public void GenerateDice()
{
    Console.WriteLine("How many dice do you want to throw?");
    int diceCount = ReadInteger();

    if(diceCount < this.MinDice || diceCount > this.MaxDice)
    {
        diceCount = Math.Clamp(diceCount, this.MinDice, this.MaxDice);
        Console.WriteLine($"Set dice count to {diceCount}"); 
    }

    for(int dieNumber = 0; dieNumber < diceCount; i++)
    {
        GenerateDie();
    }
}

public void GenerateDie()
{
    Console.WriteLine($"How many sides does the die have?");
    int sideCount = ReadInteger();

    var newDie = new Die(sideCount);

    this.Dice.Add(newDie);
}

Some notes:

  • I've made it possible to add one die or multiple. By separating the "create many" from the "create one" logic, I was able to reuse the creation logic for both without needing to copy/paste between them.
  • You could improve the UI here but I am focusing on the OOP aspect.
  • Notice that I am not rolling the dice here. I intentionally separated the creation from the rolling, so that you would be able to roll the same set of dice multiple times. This also ties into moving away from "god logic" that does everything in a single breath.
  • In more advanced situations, you'd need to separate your UI logic (Console) from your actual business logic more, but I'm choosing to not do that here because you are (a) a beginner and (b) it keeps the example easier to read.

Die rolling

I'll first give you the simpler logic (i.e. the style of loops you know):

public void RollDice()
{
    var results = new List<int>();
    int total = 0;

    foreach(var die in this.Dice)
    {
        var result = die.Roll();

        results.Add(result);
        total += result;
    }

    // Print individual rolls

    var resultString = String.Join(", ", results); // e.g. "1, 2, 3, 4"
    Console.WriteLine($"Results : {resultString}");        

    // Print total

    Console.WriteLine($"Total   : {total}");
}

However, I prefer using LINQ. Quite often, loop logic tends to be repetitively the same (select the same thing for each element in the list, sum all the values, ...) and instead of writing the whole loop, LINQ provides simple methods that do the same thing but don't require as much code.

The LINQ variant would be:

var results = this.Dice.Select(die => die.Roll()).ToList();
var total = results.Sum();

The rest of the code is the same.

Some notes

  • In more advanced situations, you'd need to separate your UI logic (Console) from your actual business logic more, but I'm choosing to not do that here because you are (a) a beginner and (b) it keeps the example easier to read.

Application logic

Notice how this logic is only concerned with generating and rolling the dice, not the general application flow (e.g. repeating the process). That is because I am intentionally starting to separate the logic into individual responsibilities:

  • Die handles a single die, i.e. how many faces it has and the ability to roll it.
  • RollDice (which I would rename DiceRoller or DiceSet) handles the creation of a set of dice, and rolling them all (note: the individual roll logic depends on Die, DiceRoller only contains the "group" logic surrounding it)
  • Program (or some other class) would contain application logic such as choosing to create a new set of dice, or rolling the created set of dice again. When the user chooses an action, Program uses DiceRoller to perform those actions.

This is how you separate class responsibilities. Identify reusable patterns and create different classes, each to represent one of these patterns (commonly called "responsibilities").

\$\endgroup\$
3
  • \$\begingroup\$ I addressed the new Random() in my answer. .NET 5 treats new Random() differently (or rather, correctly) than .NET Framework. As per help: In .NET Framework, the default seed value is time-dependent. In .NET Core, the default seed value is produced by the thread-static, pseudo-random number generator. Link: docs.microsoft.com/en-us/dotnet/api/system.random?view=net-5.0 \$\endgroup\$
    – Rick Davin
    Apr 19, 2021 at 16:26
  • \$\begingroup\$ up-voting standouts: Creating a Die class, coupled with the collective reasoning on dice-rolling logic. @Dasein, use this as the foundation for object oriented dice (the plural) and all the goodness that implies, \$\endgroup\$
    – radarbob
    Apr 19, 2021 at 23:14
  • 1
    \$\begingroup\$ @RickDavin: Good to know that it becomes less of a problem from 5 onwards, but IMO it's still an unnecessary step to keep recreating it, and at the same time a good lesson on when to use class fields to your advantage. \$\endgroup\$
    – Flater
    Apr 20, 2021 at 8:22
1
\$\begingroup\$

Welcome to CR. While I do appreciate your enthusiasm and willingness to learn, your first attempt here is fairly lacking. To give that a more positive spin, let's say there are plenty of opportunities for improvement.

The good news is indentation is very nice. And you use { } for even one-line if statements.

The rest is not so good news.

Your namespace is poorly named.

Your comments are utterly useless as they tell us nothing that we do not already know from reading the code. Comments should tell us Why you are doing something, not How.

The comment of //Variables is not needed but also slightly incorrect. In C# terminology, these are called fields since they are defined to the class. More importantly, they should be defined as properties instead of fields. Property naming conventions is that they start with a capital letter.

I hope you are using .NET 5, where the use of Random() in your Roll() method would produce different values. If you are using .NET Framework, you should use a class level property for Random().

My 2 biggest issues with the code are not with specific statements but an overall design.

One, you have mixed the business logic (BL) of rolling dice, with the user interface (UI). A well designed application separates the two. In developer terms, we call this Separation of Concerns.

And Two, other than field totalRolled, which could be property TotalRolled, I see no advantage to your class being non-static. And it could easily be made static by keeping track of the total rolls outside of the class. If you insist on a non-static class, I would suggest:

A Dice class where it contains read-only properties of the dice, i.e. number of dice and number of sides. These properties would be set in a constructor, which your current code lacks. Some of your current fields should be constants, such as maxDice or minDice, since these would not change per any instance of Dice. What does change per instance of Dice is again the number of dice and the number of sides per die. The Roll method would belong to Dice. This class should not track total rolls.

A DiceGame class that is your UI. It prompts the user for input, creates a Dice instance based on number of dice and number of sides, and keeps track of total rolls.

I wish you luck and by all means keep up the willingness to learn.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.