c# do while unassigned local variable problem

Question

I am following a c# course and trying to upgrade my user input method to check if the entered console input is an integer. I have written myself into a do while loop that I know doesn't work, but I am struggling a bit with coming up with a method that can both check for value and if the entered variable is an integer.

So what I tried here was to do-while until the user entered input is an Integer and between the min and max values. But I get stuck on result is only set to a value in the 'if' block, not the 'else' block. It won't compile unless result is set to something, unassigned variable. And I understand why, because there is a branch where I end up with a variable without value, and that won't pass in my while greater-less comparisson. You can only compare numbers, not nulls or strings.

Should I abandon the do-while loop for something more clever? Right now my 'hack' is to set result = 0 in the case that TryParse is false. That is only useful as long as the user does not need to input 0, in which case the whole thing makes no sense any longer.

 static int readInt(string prompt, int low, int high) // METHOD readInt 
    {
        int result;
        bool succes;
        do
        {
            int temp;
            string intString = readString(prompt); 
            succes = Int32.TryParse(intString, out temp);
            if (succes)
            { Console.WriteLine("The string was a number within the limits, {0}.", intString);
                result = int.Parse(intString);
            }
            else
            {
                    Console.WriteLine("{0} is not a valid number between {1} and {2}", intString, low, high);
                result = 0;
            }

        } while (!succes && (result < low) || (result > high)); 
        return result;
    }

Answer 1

you can change your " result = int.Parse(intString);" in the "if" with "return temp";

you already have the number from your TryParse, so you do not need to parse it again; returning inside the "if" also removes the need to assign a value to 'result' inside the "else" in fact, you don't need "result" at all)

static int readInt(string prompt, int low, int high) // METHOD readInt 
{
    bool succes;
    do
    {
        string intString = readString(prompt); 
        succes = Int32.TryParse(intString, out int temp);
        if (succes)
        { Console.WriteLine("The string was a number within the limits, {0}.", intString);
            return temp;
        }
        else
        {
                Console.WriteLine("{0} is not a valid number between {1} and {2}", intString, low, high);
        }

    } while (!succes && (result < low) || (result > high)); 
}

Answer 2

It might be easier to just use a while (true) loop and return from inside the loop when you have a valid result. This is a structured construct known as a loop with one exit, so it's fine to use (if you're worried about structured programming).

For example:

static int readInt(string prompt, int low, int high) // METHOD readInt 
{
    while (true)
    {
        string intString = readString(prompt);

        if (Int32.TryParse(intString, out var result) && (result >= low) && (result <= high))
        {
            Console.WriteLine("The string was a number within the limits, {0}.", intString);
            return result;
        }
        else
        {
            Console.WriteLine("{0} is not a valid number between {1} and {2}", intString, low, high);
        }
    }
}

Note the use of a relatively recent C# feature, the ability to declare an out variable at the point of use using the var keyword - see the out var result inside the TryParse().

Answer 3

Or for a simpler pattern

int result;
string intString;

while (!int.TryParse(intString = Console.ReadLine(), out result) || result < low || result > high)
   Console.WriteLine($"{intString} is not a valid number between {low} and {high}");

Console.WriteLine("The string was a number within the limits, {result}.");

return result;

Answer 4

You had a couple of bugs in the code. This works:

    static int readInt(int low, int high)
    {
        int result;
        bool success;
        bool outOfLimits = false;
        do
        {
            Console.Write("Enter a number: ");
            string intString = Console.ReadLine();
            success = Int32.TryParse(intString, out result);
            if (!success)
            {
                Console.WriteLine("{0} is not a valid number.", intString, low, high);
                continue;
            }
            outOfLimits = result < low || result > high;
            if (outOfLimits)
                Console.WriteLine("The string was NOT a number between {1} and {2}.", intString, low, high);
            else
                Console.WriteLine("The string was a number within the limits, {0}.", intString);

        } while (!success || outOfLimits);
        return result;
    }

Answer 5

Try implementing the routine validating the conditions one after one (we have no need to make code too complex with !succes && (result < low) || (result > high) check):

1. If user input a valid integer (int.TryParse)?
2. If valid is it within the range?

If any validation fails keep asking user:

static int readInt(string prompt, int low, int high) 
{
    // Keep on looping until we return a valid result
    while (true) 
    {
        // Or 
        // Console.WriteLine(prompt);  
        // string input = Console.ReadLine();
        string input = readString(prompt); 

        int result = 0; // initialization: let the compiler be happy

        if (!int.TryParse(input, out result)) // Do we have an syntactically invalid input?
          Console.WriteLine($"{input} is not a valid integer number");
        else if (result < low || result > high) // If result is valid; is it out of range? 
          Console.WriteLine($"{input} is out of [{low}..{high}] range");
        else // result is valid integer and within the ranges: time to return it
          return result; 
    }
}

Answer 6

When code evolves like yours, it sometimes gets a bit unclear what it does as the complexity rises. Usually that's a sign that you should refactor it.

I would try to make the code clearer by moving all the checks for validity into a method by itself. Then you can do something like this.

   static int readInt(string prompt, int low, int high) // METHOD readInt 
    {
        bool valid = false;
        int result = 0;
        while (!valid)
        {
            var intString = readString(prompt);
            valid = checkIfValid(intString, low, high, out result);
        }
        return result;
    }

    static bool checkIfValid(string s, int low, int high, out int result)
    {
        if (!Int32.TryParse(s, out result))
        {
            Console.WriteLine(s + " isn't an integer");
            return false;
        }

        if (result < low)
        {
            Console.WriteLine("Number is too low");
            return false;
        }

        if (result > high)
        {
            Console.WriteLine("Number is too high");
            return false;
        }

        return true;
    }