1
while (playerMove != "SCISSORS" || playerMove != "PAPER" && playerMove != "ROCK" || string.IsNullOrEmpty(playerMove))
   {
     Console.WriteLine("Please enter a valid command!");
     playerChoice = Console.ReadLine().ToUpper().Trim();
     continue;
   }

Can someone explain to a beginner programmer why this code doesn't work. The way I read this code myself is: While playerMove isn't "SCISSORS" or "PAPER" and isn't "ROCK" or Empty, do this...

How do I make this work?

Josef Ginerman
  • 1,460
  • 13
  • 24
PrebenL
  • 13
  • 3
  • check [this](https://kodify.net/csharp/if-else/short-circuit-if/) article. it may help you – Abbas Dehghan Mar 17 '19 at 11:42
  • In your case I'd recommend using an Enum to define Rock, Paper & Scissors. Then using a switch-case to determine what the user selected and what to do. – Josef Ginerman Mar 17 '19 at 11:44
  • Does this answer your question? [Why does non-equality check of one variable against many values always return true?](https://stackoverflow.com/questions/26337003/why-does-non-equality-check-of-one-variable-against-many-values-always-return-tr) – tripleee Sep 30 '22 at 05:29

2 Answers2

4

If you enter "PAPER", then playerMove != "SCISSORS" is true, so your while-condition is true, because only one part of a logical OR has to be true.

You want all of the !="XYZ" to be true, OR the string to be empty:

while((playerMove != "SCISSORS" && playerMove != "PAPER" && playerMove != "ROCK") || string.IsNullOrEmpty(playerMove))

That said, there are probably easier ways to check this.

You could define a list of acceptable strings, for instance, and check against that:

using System.Linq;
...
var acceptedString = new List<string> {"ROCK", "PAPER", "SCISSORS"};
...
while (!acceptedStrings.Contains(playermove)
{
   // error message
}

Actually checking for an empty string is useless, since an empty string never has an accepted value.

Another option, as mentioned in a comment, is to extract your condition to a method. This is almost always a good idea for complicated conditions:

while (!IsValid(playermove)){...}
...
private static bool IsValid(string move)
{
   return move == "ROCK"
       || move == "PAPER"
       || move == "SCISSORS";
}

Alternatively you can rewrite that to 

while (!IsValid(playermove)){...}
...
private static bool IsValid(string move) =>
    move == "ROCK" || move == "PAPER" || move == "SCISSORS";
oerkelens
  • 5,053
  • 1
  • 22
  • 29
  • Could you maybe give some advice, regarding the best practice for checking something like this. I'm soaking up any information you can throw at me. Any advice would help. – PrebenL Mar 17 '19 at 11:38
  • I feel like defining a list for only 3 choices decreases readability. I'd rather extract the logic in a method: `private bool IsValidChoice(string choice) => choice == "SCISSORS" || choice == "PAPER" || choice == "ROCK";` Then loop on it: `while (!IsValidChoice(playerMove))` – Kevin Gosse Mar 17 '19 at 11:45
  • But our two solutions converge on one point: mixing multiple negative statements is hard to read. It's better to make positive statements, then negate them as a whole – Kevin Gosse Mar 17 '19 at 11:47
  • @KevinGosse there are many different options, of course. And I agree that in general you should use a method for a complicated condition - I'll add that option as well :) – oerkelens Mar 17 '19 at 11:51
  • Thank you both for the information, looking into both answers right now, since you both agree a method is the best thing to do. i'll start with this. – PrebenL Mar 17 '19 at 11:56
0

I feel so dumb right now, i only just noticed i broke my head over this code for no reason. I noticed in the loop i asked to set a new value to "playerChoice", instead of playerMove. hence why it didnt run the way i wanted it to.

while ((playerMove != "SCISSORS" && playerMove != "PAPER" && playerMove != "ROCK") || string.IsNullOrEmpty(playerMove))
                {
                    Console.WriteLine("Please enter a valid command!");
                    playerMove = Console.ReadLine().ToUpper().Trim();
                    continue;
                }

Including answer into the conditions aswell.

PrebenL
  • 13
  • 3