Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
1  
You initialize playerChoice to -1, but that value is never used. –  CodesInChaos Mar 11 at 13:24
    
validInput = int.TryParse(Console.ReadLine(),out playerChoice); @CodesInChaos –  Malachi Mar 11 at 13:33
    
That line overwrites the initial value of player choice and doesn't read it (it's an out parameter), so you didn't need to initialize it to -1. I'd instead declare it as int playerChoice; without initialization. –  CodesInChaos Mar 11 at 13:34
1  
Unless you REALLY need to use an untyped variable, strongly typing generally is the better way to go, because it can help catch stupid errors down the line. It's an added line of defence against bugs. –  eidylon Mar 11 at 15:59
1  
This is C#, there are no untyped variables. All variables are strongly statically typed. –  Jörg W Mittag Mar 11 at 23:52

3 Answers 3

up vote 47 down vote accepted

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.

share|improve this answer
9  
Very well explained, puts the ideas of encapsulation and separation of responsibility into more concrete terms that others can more readily understand. –  UpAndAdam Mar 10 at 22:39
    
I generally don't like do while loops I rather like while loops. I guess that I could still write it as a while loop. I will post the code to the game when I am finished with my changes. –  Malachi Mar 11 at 2:41
4  
I know leaving "+1" and "thanks" comments are generally discouraged, but I want to ignore that here just to say thanks, Eric, for all of your answers across Stack Exchange. I regularly browse through the latest answers on your profile (mainly on SO), because the quality of the information that you provide is always so good. –  John H Mar 11 at 12:47
5  
@JohnH: That discouragement is in my opinion counterproductive. Politeness costs very little and goes a long way towards making people feel good about both asking for help and providing help to others. You're very welcome; it's my pleasure. Thanks for the kind words. –  Eric Lippert Mar 11 at 13:14
4  
@EricLippert "Thanks — good point about X" comments are fine; just "Thanks" would be noise. Votes are the currency of thanks that makes Stack Exchange sites work, so we prefer voting — it's the most rewarding form of thanks possible. I notice that you have only voted twice so far. Code Review would benefit greatly from having experts like you recognize good questions and answers from other members by voting. –  200_success May 7 at 4:30

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.

share|improve this answer
    
In one case, you're telling him to use a naming convention for bool that indicates the type, in another case you're advising him not to indicate in the name that his variable is a list. Isn't that inconsistent? –  Nate C-K Mar 10 at 20:53
    
@NateC-K a bool is a simple value type with true/false values; "IsXxxxxx" is an established naming convention for them. A list, however, is a reference type that implements an interface; by sticking "List" to a variable's name, you are potentially calling a cat as a dog, ...or a lion. Many types implement IList<T>, not just List<T>; calling an enumerable a xxxxList is a lie. –  Mat's Mug Mar 10 at 21:56
    
The "is..." naming convention is part of Hungarian notation, which MS says you shouldn't bother with anymore. Anything that implements IList is necessarily an IList (not to mention a variable of this type can only be used to call methods that implement IList), so I don't see how you can say that it is "a lie" to call it a list. You program only cares about the typing of the variable at the current level of abstraction; you seem to suggest that I should care about the details underlying the abstraction even when they are irrelevant. –  Nate C-K Mar 10 at 22:09
2  
Variable naming is for the Developer only, the compiler will compile the code no matter what you name it. standards are for people maintaining the code. the naming schemes are there as guidelines. –  Malachi Mar 10 at 22:29
4  
@NateC-K From Microsoft's Names of Type Members (4.5): "1. DO name collection properties with a plural phrase describing the items in the collection instead of using a singular phrase followed by "List" or "Collection." 2. DO name Boolean properties with an affirmative phrase (CanSeek instead of CantSeek). Optionally, you can also prefix Boolean properties with "Is," "Can," or "Has," but only where it adds value." –  Pakman Mar 11 at 15:18

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;
}
share|improve this answer
    
Good, but your code has the bug that inputting 0 or a number higher than the number of gestures will still end the loop. Which is, admittedly, a bug in the original code too, but it was more obvious there. –  Bobson Mar 10 at 18:42
    
@Bobson I'm not sure I see the problem, if the user enters 0 it will fail playerChoice > 0 which will set validInput to false, which will cause the main loop to go again. –  BenVlodgi Mar 10 at 18:45
2  
Ooh, no, you're right. I misplaced the ()'s when I was reading it such that validInput was only set to the result of TryParse() and not the rest of the conditions. –  Bobson Mar 10 at 19:15
1  
@Malachi yes, everything happens sequentially, so the comparisons will only be ran if the TryParse didn't fail. the TryParsehas to finish to be able to determine this, which means it has sent out the playerChoice before continuing. –  BenVlodgi Mar 10 at 19:26
1  
I think it makes the code harder to read if you cram so much of the logic into the test part of the do-while block. I think it's generally better practice to put things like reading input inside the body of the loop, assuming that doing so gives you the same result. –  Nate C-K Mar 10 at 20:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.