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").
