Receiving user input from do while statement

Question

I'm at a total loss here... The logic seems to be setup correctly but the "response" in the while statement says it doesn't exist in the current context. I searched on here and quite seem to find the same issue in this context. Is the issue the conver to method?

    do
        {
            Console.WriteLine("enter a number between 1 and 5");
            int x = Convert.ToInt32(Console.ReadLine());

            Random r = new Random();
            int rr = r.Next(1, 5);
            Console.WriteLine("Do you want to continue?  Please select yes or no.");
            string response = Convert.ToString(Console.ReadLine());
        } while (response == "yes");

You declared `response` inside the loop, so that is where it exists Generally a new level of indentation creates a new block scope.. You also probably want to create one random instance for the entire loop, not for each iteration — Ňɏssa Pøngjǣrdenlarp, Dec 31 '16 at 18:18
If you define `response` inside the loop then how is it supposed to check it outside of it? You have to define `string response` before the loop — UnholySheep, Dec 31 '16 at 18:18
Response is indeed not visible out of {} scope, declare it before do — Oleg Bogdanov, Dec 31 '16 at 18:18
You should also read [ask] and take the [tour]...just because you havent yet — Ňɏssa Pøngjǣrdenlarp, Dec 31 '16 at 18:20
Nitpick: `Console.ReadLine` already gives a `string` result. There is no need to Convert to string. — Hans Keﬆing, Dec 31 '16 at 18:27
**also** the max param to `Random.Next(min, max)` is exclusive so the code will only ever generate a value from 1 to 4 — Ňɏssa Pøngjǣrdenlarp, Dec 31 '16 at 18:43

Jonathon Reinhart · Answer 1 · 2016-12-31T18:24:14.657

Variables declared in one scope (generally a set of braces { ... }) are not accessible outside of that scope. You've declared response inside the loop. You need to declare response outside of the loop.

You also want to trim whitespace from the string before comparing it, using String.Trim(). Otherwise there will be a newline character (\n) at the end, causing your comparison to fail.

string response;

do {
    //...

    response = Console.ReadLine().Trim();
} while (response == "yes");

Thomas Fitzgerald · Answer 2 · 2016-12-31T18:27:41.133

1

Your response variable is not in the context of the loop. Simply move the variable declaration outside the loop as below:

        string response = String.Empty;

        do
        {
            Console.WriteLine("enter a number between 1 and 5");
            int x = Convert.ToInt32(Console.ReadLine());

            Random r = new Random();
            int rr = r.Next(1, 5);
            Console.WriteLine("Do you want to continue?  Please select yes or no.");
            response = Convert.ToString(Console.ReadLine());
        } while (response == "yes");

edited Dec 31 '16 at 18:27

answered Dec 31 '16 at 18:24

Thomas Fitzgerald

103
1
10

1

No need to create a new random for each iteration; its a bad habit. No need to Convert.ToString either – Ňɏssa Pøngjǣrdenlarp Dec 31 '16 at 18:30
1

@Plutonix - you are correct, but local variables used only inside the braces should be declared inside the braces, to limit scope. The compiler is smart enough to make this as efficient as possible. – Joe Dec 31 '16 at 19:11
1

The compiler is not going to move the Random creation to outside the loop. There are literally hundreds of questions here where the OP is getting the same number over and over because they create a new one in a loop. Also, `r.Next(1, 5)` also is not going to generate a number from 1-5 – Ňɏssa Pøngjǣrdenlarp Dec 31 '16 at 19:30
2

**[Random number generator only generating one random number](http://stackoverflow.com/q/767999/1070452)** and another: **[Multiple random numbers are the same](http://stackoverflow.com/q/14673876/1070452)** and also **[Random number generator only generating one random number](http://stackoverflow.com/q/767999/1070452)** – Ňɏssa Pøngjǣrdenlarp Dec 31 '16 at 19:41
@Joe The compiler will detect the attempted use of an uninitialized local variable. It is incorrect to initialize the variable here because its initial value will never be used. Some tools like ReSharper might even automatically suggest you remove the initialization. Someone once said "hey you should initialize local variables" and people turned it into a hard rule, instead of general advice... just like the "omg never use goto" fanatics. – Jonathon Reinhart Jan 01 '17 at 14:25

David Cardinale · Answer 3 · 2017-01-02T01:25:20.997

0

Might help to encapsulate this a bit. How about:

static void Main(string[] args)
    {
        Random rand = new Random();
        do
        {
            Write("enter a number between 1 and 5");
            string response = Console.ReadLine();
            int x = 5;
            if (Validate(response, "1-5")) int.TryParse(response, out x);                
            Write(rand.Next(0,x));
            Write("Do you want to continue?  Please select yes or no.");                
        } while (Validate(Console.ReadLine().ToLower(), "yes"));
    }
    static void Write(string s) => Console.WriteLine(s);
    static bool Validate(string s, string Pattern) => Regex.Match(s, Pattern).Success;

edited Jan 02 '17 at 01:25

answered Dec 31 '16 at 20:14

David Cardinale

153
7

Creating a new Random object every time you want a random number is incorrect. – Jonathon Reinhart Jan 01 '17 at 14:21
I hope you down voted all the others as I am only working with the code they provided. I'll correct it anyway, even though this has no effect on processing time or memory as GC should collect this as it falls out of scope each call and system idles on the wait for user response and as needed... – David Cardinale Jan 02 '17 at 01:23
I actually believe you are incorrect, even though I believe I met your recommendation. Following proper OOP it would be better to encapsulate the randomization into a method as I did. The loop should be kept as clean as possible as its purpose is to gather user input, the processing of the input should be handled elsewhere. It is perfectly okay to create a new Random for each call to a method, but not within the loop itself as this would prevent GC from collecting as the references would never be released. However in its own method they would be released with each call and return value. – David Cardinale Jan 02 '17 at 01:52
I hope you intend on responding or removing your down vote as this code works just fine, and I believe its prior form was more correct than the one you recommended. – David Cardinale Jan 02 '17 at 01:56
Please see this comment above: http://stackoverflow.com/questions/41410716/receiving-user-input-from-do-while-statement/41411421?noredirect=1#comment70025945_41410770 – Jonathon Reinhart Jan 02 '17 at 02:04
Again you are not entirely incorrect, however this is not a tight loop. This is the MSDN statement on it: "Initializing two random number generators in a tight loop or in rapid succession creates two random number generators that can produce identical sequences of random numbers. In most cases, this is not the developer's intent and can lead to performance issues, because instantiating and initializing a random number generator is a relatively expensive process." https://msdn.microsoft.com/en-us/library/system.random(v=vs.110).aspx#Multiple – David Cardinale Jan 02 '17 at 03:01

Receiving user input from do while statement

3 Answers3