Ensuring user input is an integer in a range

Question

I have isolated a little bit of code that was causing a small debate between myself and another user. I have taken some of the things that he said and meshed it with the code that was being reviewed in the first place here.

Original Code

while (!validInput)
{
    var playerChoice = -1;
    Console.WriteLine("Please choose your Gesture ");
    gameSetup(listOfGestures);
    try
    {
        playerChoice = Convert.ToInt32 (Console.ReadLine());
    }
    catch (FormatException e)
    {
        validInput = false;
    }

    if (playerChoice > 0 && playerChoice <= listOfGestures.Count)
    {
        playerGesture = listOfGestures[playerChoice - 1];
        validInput = true;
    }
    else
    {
        validInput = false;
    }
}

---

Here is what I came up with based on our conversation.

New Code

var validInput = false; //forgot this part
while (!validInput)
{
    var playerChoice = -1;
    Console.WriteLine("Please choose your Gesture ");
    gameSetup(listOfGestures);
    validInput = int.TryParse(Console.ReadLine(),out playerChoice);
    if (playerChoice > 0 && playerChoice <= listOfGestures.Count)
    {
        playerGesture = listOfGestures[playerChoice - 1];
        validInput = true;
    }
}

---

Do I need to create a new method to sort out user input and whether or not it is an integer inside the min and max bounds?

Answer 1

The second sample is definitely better. I would be inclined to continue to improve the code by separating mechanisms from policies. The policy is the code that actually expresses the meaning of the program; the mechanism is the code that expresses what specific operations implement the policy.

This idea comes from security design; you don't want the code that computes "is Bob allowed to open this door?" and the code that computes "is that really Bob's card key?" to be the same code. The first is a policy, the second is a mechanism. But it applies generally to all code.

By separating mechanism from policy we enable both to be understood more easily. The "main line" of your code should read like the specification. If you had to describe your code in English you'd probably say something like "present the user with a choice of gestures. If the user chooses an invalid gesture, keep trying until they choose a valid gesture." That's the meaning of your program, but that's not what your code looks like. Rather, your code looks like the most important things in the world are integer and bool variables, list counts, and so on.

Let's identify a mechanism: parsing an integer and testing whether it is in range is the mechanism behind the policy of "the user must choose a valid gesture". So let's isolate that mechanism into a purely mechanistic method, that knows nothing about your policy domain:

static class Extensions
{
    public static int? BoundedParse(this string str, int lower, int upper)
    {
        if (str == null) 
            return null;
        int result;
        bool success;
        success = int.TryParse(str, out result);
        if (!success) 
            return null;
        if (result < lower) 
            return null;
        if (result > upper) 
            return null;
        return result;
    }
}

Let's now rewrite your loop as a do-while instead of a while. Look how much ugly mechanism we removed, leaving your policy shining brightly through:

gameSetup(listOfGestures);
int? choice;
do
{
    Console.WriteLine("Please choose your Gesture ");
    choice = Console.ReadLine().BoundedParse(1, listofGestures.Count);
}
while(choice == null);
playerGesture = listOfGestures[choice.Value - 1];

And as a bonus, we now have a reusable method that we can apply to any task that requires that a string be parsed as a bounded integer.

We can take this even further. Isn't that loop a mechanism? Move it into a helper method:

static int PromptForNumber(string prompt, int lower, int upper)
{
    int? choice;
    do
    {
        Console.WriteLine(prompt);
        choice = Console.ReadLine().BoundedParse(lower, upper);
    }
    while(choice == null);
    return choice.Value;
}

And now your method is:

gameSetup(listOfGestures);
int choice = PromptForNumber("Please choose your gesture", 1, listofGestures.Count);
playerGesture = listOfGestures[choice - 1];

Now the code is super clear what is going on at the policy level because all the stuff you don't care about is now the responsibility of some mechanism somewhere. (And again we have a great mechanism for prompting for a number that we can re-use later.)

Even better: maybe there is a better way to display a prompt and get a number. If we decided that we wanted to write a GUI instead, we can change the mechanism code without changing the policy code. The policy code doesn't care how the valid gesture gets chosen, just that it does.

Answer 2

Random Nitpicks

• validInput should be named isValidInput to follow bool naming convention.
• listOfGestures shouldn't convey its type in the name (assuming it's a List<Gesture>); gestures would be a better name.
• if gestures were a Dictionary<int,Gesture>, your check-if-the-value-is-within-bounds condition would become much simpler: gestures.TryGetValue(playerChoice, out playerGesture);

Do I need to create a new method to sort out user input and whether or not it is an integer inside the min and max bounds?

If you went OOP-all-the-way and encapsulated that piece of logic into its own object, you could write a unit test that validates whether a valid input returns a Gesture:

interface IGestureInputValidator
{
    Gesture ValidateInput(string userInput);
}

The implementation of that interface could return null for any illegal userInput value, or a Gesture instance for any legal input. This would change your code a little:

Gesture playerGesture;
while (playerGesture == null)
{
    Console.WriteLine("Please choose your Gesture:");
    var input = Console.ReadLine();
    playerGesture = _validator.ValidateInput(input);
}

Notice _validator is an instance field; you can probably inject that instance in your constructor (or new it up there).

---

I... just noticed you have no Gesture object - a string could work just as well:

string playerGesture;
while (playerGesture == null)
{
    Console.WriteLine("Please choose your Gesture:");
    var input = Console.ReadLine();
    playerGesture = _validator.ValidateInput(input);
}

Given a small change in the validator interface:

interface IGestureInputValidator
{
    string ValidateInput(string userInput);
}

However I don't like this, because the intent isn't crystal-clear anymore; a string could be anything, and we're looking for a specific kind of string. I think gestures deserve their own class, or even better, an enum. Whatever you do, extracting the validation into its own object (or method, if you consider that responsibility as part of the same class that holds the other piece of logic) allows you to change that logic, without affecting the rest of your code.

I'd recommend to separate the concerns as much as possible, so to boldly answer your question, yes, you should separate it.

Answer 3

It would be best for the sake of modularity, and re-usability for this whole chunk of code to be placed in a separate method. If you are going to need more user input besides just what gesture they want to use you could also make it more generic to be re-used through the rest of your program for various situations.

Your code seems to be a bit illogical as you are using the playerChoice variable even when you know it was not parsed correctly. you set validInput, but then don't check it in the if statement.

Here is how I would re-write that chunk if you weren't going to just separate it all out into a generic method of its own.

Note: I also changed the default value of playerChoice to be 0 because that is obviously out of range in this case, as you say that valid input has to be over 0. This change is minor and unnecessary, but I just wanted you to be aware of it.

while (!validInput)
{
    var playerChoice = 0;
    Console.WriteLine("Please choose your Gesture ");
    gameSetup(listOfGestures);
    if (validInput = int.TryParse(Console.ReadLine(), out playerChoice) && playerChoice > 0 && playerChoice <= listOfGestures.Count)
        playerGesture = listOfGestures[playerChoice - 1];
}

---

Edit: My original response to the overall structure of this program included code that gave the user specific feedback to tell the user why their input was wrong (Point # 9 in my review). If you are not looking for such feedback in a method, here is a simple Method that gets Integer User input from the console

public static int GetIntInRange(int min = int.MinValue, int max = int.MaxValue, string prompt = "Please enter an Integer: ")
{
    int parsedValue;
    do { Console.Write(prompt); } 
    while (!(int.TryParse(Console.ReadLine(), out parsedValue) && parsedValue >= min && parsedValue <= max));
    return parsedValue;
}