Resolved Code review request - integer issues

[Bobert]

Hello!
I just started learning C# via online youtube videos and am trying to apply some learnings to make a small dice game with the below code but am having issues getting the dice roll to return as an integer and keep getting the result as 1. Was hoping for some help and feedback on where I'm going wrong, please!

using System;
using System.Collections.Generic;
namespace Dice_game
{
    class Dice_game
    {
        static void Main(string[] args)
        {       
            Random numberGen = new Random();
           
            Console.WriteLine("Hello. Let's play a game. You will roll 2 dice by pressing enter. \nGuess how many rolls it will take to get the sum of those numbers that you also choose.");
            Console.WriteLine("What's the sum you're aiming for?");
            int sumGuess = Convert.ToInt16(Console.ReadLine());
            Console.WriteLine("How many rolls do you think it will take to hit?");
            int rollCount = Convert.ToInt16(Console.ReadLine());
            int dice1 = 0;
            int dice2 = 1;
            int attempt = 0;
            int sum = (dice1+dice2);
            Console.WriteLine("Press enter to roll the dice.");
            while (sum != sumGuess) {
                Console.ReadKey();
                dice1 = numberGen.Next(1,7);
                dice2 = numberGen.Next(1,7);
                Console.WriteLine ("You rolled " + dice1 + " and " + dice2 + " for a total of " + sum);
                attempt++;
            }
            Console.WriteLine("You guessed it would take " + rollCount + " attempts to roll your estiamted sum of " + sumGuess);
            Console.WriteLine("It took you " + attempt + " attempts to roll " + sum);
            // Wait before closing
            Console.ReadKey();
        }
    }
}

 

[Skydiver]

That's because you only compute sum on line 19. You need to compute it after line 24.

 

[jmcilhinney]

A lot of people seem to think that being new to programming means that they should abandon the logic they have been using their whole life. It does not. Code is not magic. It is simply an implementation of logic. That is the same logic you have been using your whole life. If you were playing a game IRL, would you try to add the values on your dice before rolling them? Of course you wouldn't, so why would you do it in your code? People try to write code without a clear idea of what it is supposed to do and it ends up doing nothing useful. You need to work out your logic first and then you should write it down. Formalise it into an algorithm and then test that algorithm manually. Once you know it works, then you can write code to explicitly implement that algorithm. You can then always refer back to the algorithm to make sure that the code is doing what it is supposed do. If your algorithm says to roll the dice first and then add up the values but your code adds the values first and then rolls the dice then the issue is obvious. If you don't know what the logic is that the code is supposed to implement though, you can't see when the code is doing illogical things.

 

[Skydiver]

If you were playing a game IRL, would you try to add the values on your dice before rolling them? Of course you wouldn't, so why would you do it in your code?

It's a side effect of the way math/algebra was taught to some of us. We are taught that an equation will always be true. You define an equation once, and it will always be set even if you manipulate the variables in the equation around.

And then when people start learning how to program a few years later, poor tutorials or poor teachers will say that a variable assignment is just like an equation from algebra. Unfortunately, as we know, that is not true in the programming world. A variable assignment is not an equation even though it may look like it. In programming that assignment only happens at that instant in time while the line of code is being executed. Later when the variables that were on the righthand side are updated, the variable on the lefthand side won't be automatically updated. I'm guessing that the OP was expecting the lefthand side to get updated based on the way his code above was written.

 

[Bobert]

It's a side effect of the way math/algebra was taught to some of us. We are taught that an equation will always be true. You define an equation once, and it will always be set even if you manipulate the variables in the equation around.

And then when people start learning how to program a few years later, poor tutorials or poor teachers will say that a variable assignment is just like an equation from algebra. Unfortunately, as we know, that is not true in the programming world. A variable assignment is not an equation even though it may look like it. In programming that assignment only happens at that instant in time while the line of code is being executed. Later when the variables that were on the righthand side are updated, the variable on the lefthand side won't be automatically updated. I'm guessing that the OP was expecting the lefthand side to get updated based on the way his code above was written.

Thank you both for the feedback! It helps to have the concepts explained more thoroughly, so I appreciate that.

My understanding is that int dice1 = 0 is storing the integer of the rolled number between 1-6, and that the sum that I put after would simply add those rolled values together once the random numbers are generated. When I run the code, it returns random values each time as it should, but doesn't store those rolled values as integers because it keeps adding up to 1 (per screenshot). If I move the sum to after line 24, then I get an error because sum doesn't exist for line 25 to refer to, and I'm unable to declare it a second time. So I'm confused by why the dice rolls are working and returning the random values, but then those shown values are not the ones being added together.

 

[Skydiver]

Try this instead:

using System;
using System.Collections.Generic;
namespace Dice_game
{
    class Dice_game
    {
        static void Main(string[] args)
        {      
            Random numberGen = new Random();
         
            Console.WriteLine("Hello. Let's play a game. You will roll 2 dice by pressing enter. \nGuess how many rolls it will take to get the sum of those numbers that you also choose.");
            Console.WriteLine("What's the sum you're aiming for?");
            int sumGuess = Convert.ToInt16(Console.ReadLine());
            Console.WriteLine("How many rolls do you think it will take to hit?");
            int rollCount = Convert.ToInt16(Console.ReadLine());
            int dice1 = 0;
            int dice2 = 0;
            int attempt = 0;
            int sum = 0;
            Console.WriteLine("Press enter to roll the dice.");
            while (sum != sumGuess) {
                Console.ReadKey();
                dice1 = numberGen.Next(1,7);
                dice2 = numberGen.Next(1,7);
                sum = dice1 + dice2;
                Console.WriteLine ("You rolled " + dice1 + " and " + dice2 + " for a total of " + sum);
                attempt++;
            }
            Console.WriteLine("You guessed it would take " + rollCount + " attempts to roll your estiamted sum of " + sumGuess);
            Console.WriteLine("It took you " + attempt + " attempts to roll " + sum);
            // Wait before closing
            Console.ReadKey();
        }
    }
}

Notice the minimal changes on lines 17 and 19, and the new line 25.

 

[Skydiver]

but then those shown values are not the ones being added together.

It's because you are not adding them together into sum.

 

[Bobert]

Try this instead:

using System;
using System.Collections.Generic;
namespace Dice_game
{
    class Dice_game
    {
        static void Main(string[] args)
        {     
            Random numberGen = new Random();
        
            Console.WriteLine("Hello. Let's play a game. You will roll 2 dice by pressing enter. \nGuess how many rolls it will take to get the sum of those numbers that you also choose.");
            Console.WriteLine("What's the sum you're aiming for?");
            int sumGuess = Convert.ToInt16(Console.ReadLine());
            Console.WriteLine("How many rolls do you think it will take to hit?");
            int rollCount = Convert.ToInt16(Console.ReadLine());
            int dice1 = 0;
            int dice2 = 0;
            int attempt = 0;
            int sum = 0;
            Console.WriteLine("Press enter to roll the dice.");
            while (sum != sumGuess) {
                Console.ReadKey();
                dice1 = numberGen.Next(1,7);
                dice2 = numberGen.Next(1,7);
                sum = dice1 + dice2;
                Console.WriteLine ("You rolled " + dice1 + " and " + dice2 + " for a total of " + sum);
                attempt++;
            }
            Console.WriteLine("You guessed it would take " + rollCount + " attempts to roll your estiamted sum of " + sumGuess);
            Console.WriteLine("It took you " + attempt + " attempts to roll " + sum);
            // Wait before closing
            Console.ReadKey();
        }
    }
}

Notice the minimal changes on lines 17 and 19, and the new line 25.

Thank you so much for your help with that! I didn't realize the sum would also have to first be stored as 0 and then redeclared again. Thank you

 

[Skydiver]

Line 25 is not a redeclaration. It is an assignment statement.

Line 19 is a declaration with assignment or initialization. It could have been two lines as:

int sum;    // declaration
sum = 0;    // assignment statement

The reason why we initialize things is other than this being a best practice is that it will prevent a C# compiler error on line 21 where the compiler will complain that you are using a variable that has not been initialized.

 

[Skydiver]

Okay, code review time:

1. Line 2: Unneeded using.
2. Lines 2-3: Missing vertical whitespace between line 2 and 3 to separate using declarations from namespace block.
3. Line 3: Bad namespace name. Does not following .NET naming conventions. Do not use underscores. Use Pascal casing. Namespace must be of the form CompanyName.Product .
4. Line 5: Bad class name. Does not following .NET naming conventions. Do not use underscores. Use Pascal casing.
5. Line 7: Unused args should be removed.
6. Line 9: Move numberGen declaration and initialization closer to the code that uses it.
7. Line 11: In C#, we the convention is not to embed "\n" in strings. The convention is to use multiple WriteLine()'s (or AppendLine() if using a StringBuilder if possible. If you'll embed it use "\r\n" because C# is Windows/DOS/NT centric. If you want to be more universal, then use Environment.NewLine which is a constant value appropriate for the target environment.
8. Line 13,15: Use int.TryParse() instead of Convert.To*() to be able to prevent exceptions from being thrown due to invalid input.
9. Line 13: Input validatione: What happens if the user values greater than 12?
10. Lines 13,15: Input validation: What happens if the user enters negative values or zero?
11. Lines 20,22: You are telling the user to press Enter, but almost any other key press will also roll the dice.
12. Lines 23-24: Don't use magic numbers.
13. Lines 26, 29, 30: Prefer to use string formatting or string interpolation. Ex. Console.WriteLine("You rolled {0} and {1} for a total of {2}", dice1, dice2, sum)[icode] or [icode]Console.WriteLine($"You rolled {dice1} and {dice2} for a total of {sum}")
14. Line 31-32: Prefer to create methods with meaningful names instead of commenting. Comments go out of date. Method names receive more developer attention.
15. Lines 11-32: Breakup lines of code to denote the phases of operation or groups of related functionality. Ideally blocks of lines that are of related functionality should be moved into their own methods.

Here's an example of an re-write:

using System;

namespace AdventureWorks.DiceGame
{
    class DiceGame
    {
        const int MinSum = 1;
        const int MaxSum = 12;
        const int MinRoll = 1;
        const int MaxRoll = int.MaxValue;

        static int ReadInteger(string prompt, int min, int max)
        {
            int value = 0;

            do
            {
                Console.WriteLine(prompt);
                var input = Console.ReadLine();

                if (!int.TryParse(input, out value) || value < min || value > max)
                {
                    Console.Error.WriteLine($"That is not a valid integer in the range {min} to {max}. Please try again");
                }
            } while (value <= 0);

            return value;
        }

        static ConsoleKeyInfo ReadKeyWithoutEcho()
        {
            return Console.ReadKey(true);
        }

        static void WaitForEnterKey()
        {
            Console.WriteLine("Press Enter key to roll the dice.");
            while(ReadKeyWithoutEcho().Key != ConsoleKey.Enter)
            {
                // do nothing
            }
        }

        static void WaitForAnyKeyBeforeClosing()
        {
            ReadKeyWithoutEcho();
        }

        static void Main()
        {
            Console.WriteLine("Hello. Let's play a game. You will roll 2 dice by pressing enter.");
            Console.WriteLine("Guess how many rolls it will take to get the sum of those numbers that you choose.");

            var sumGuess = ReadInteger("What's the sum you're aiming for?", MinSum, MaxSum);
            var rollGuess = ReadInteger("How many rolls do you think it will take to hit?", MinRoll, MaxRoll);

            var numberGen = new Random();
            int rollCount = 0;
            int sum;
            do
            {
                WaitForEnterKey();

                int dice1 = numberGen.Next(1, 7);
                int dice2 = numberGen.Next(1, 7);
                sum = dice1 + dice2;
                rollCount++;

                Console.WriteLine($"You rolled {dice1} and {dice2} for a total of {sum}.");
            } while (sum != sumGuess);

            Console.WriteLine($"You guessed it would take {rollGuess} attempts to roll your estimated sum of {sumGuess}.");
            Console.WriteLine($"It took you {rollCount} attempt(s) to roll {sum}.");

            WaitForAnyKeyBeforeClosing();
        }
    }
}

The code above can still be improved. For example Lines 11-70 should really be moved into its own method. Some of the methods in the class should be broken off into helper utility classes because they don't really have anything directly to do with the dice game.