2

My goal is to get specific inputs from the user (A, B, C, and D only).

for example: If i enter the letter A, the if statement will execute (its not supposed to). and the same with the do while. (Logic Error)

  char[] response = new char[20];
  Scanner k = new Scanner(System.in);

  //Prompts User to fill array with their responses
  for(int i=0; i<response.length; i++) {
       //Input validation
       do {
          System.out.println("Answer "+(i+1)+":");
          response[i] = k.nextLine().charAt(0); 

          if(response[i] != 'A' ||response[i] != 'B' || response[i] != 'C' ||response[i] != 'D')
              System.out.print("Try Again. ");
       }
       while(response[i]!= 'A' ||response[i] != 'B' ||response[i] != 'C' ||response[i] != 'D');
  }
Cito
  • 23
  • 5
  • 5
    You have a logical flaw, don´t use the or condition (it will allways be true logical) , rather use an and statement, reason beeing, if the response equals `A,B,C or D` then the other three conditions will still be true. – SomeJavaGuy Sep 04 '15 at 07:11
  • I'd guess that you're getting lowercase letters 'a', 'b', etc instead of uppercase 'A', 'B', etc? Try `System.out.println(response[i]);` to debug. – Luke Sep 04 '15 at 07:13

2 Answers2

7

This is how I would write it

Scanner in = new Scanner(System.in);
char[] response = new char[20];

//Prompts User to fill array with their responses
for(int i = 0; i < response.length; i++) {
   for (;;) {
       System.out.println("Answer " + (i + 1) + ":");
       response[i] = in.nextLine().charAt(0);
       //Input validation
       if ("ABCD".indexOf(response[i]) >= 0)
           break;
       System.out.print("Please try again, it must be A, B, C or D");
   }
}

What you were doing wrong is you needed to write

if (!(response[i] == 'A' || response[i] == 'B' || response[i] == 'C' || response[i] != 'D'))

OR

if (response[i] != 'A' && response[i] != 'B' && response[i] != 'C' && response[i] != 'D')
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
  • Why not use contains(...) instead of indexOf(...)? – L.Butz Sep 04 '15 at 07:16
  • @L.Butz You could write `"ABCD".contains("" + response[i])` The problem is there is no `String.contains(char)` in Java 8. – Peter Lawrey Sep 04 '15 at 07:18
  • Use .println() on the "Please try again" method, and check for "ABCDabcd" or call Character.toUpperCase(). Also, just a warning; the program crashes if the string returned by nextLine() is an empty string! – Luke Sep 04 '15 at 07:18
  • I think it would be more clean solution to make a Set of allowed characters instead of checking in a String. Set allowedCharacters = new HashSet<>(Arrays.asList('A','B','C','D')) and then check if this set contains the character. – PKuhn Sep 04 '15 at 07:37
0

You condition is backwards. When you get it A you will get

if (false || true || true || true) 

which will pass.

Change this to:

if(response[i] != 'A' && response[i] != 'B' && response[i] != 'C' && response[i] != 'D')
Nicholas Robinson
  • 1,359
  • 1
  • 9
  • 20
  • 4
    How can the response be A and B and C and D ? – Peter Lawrey Sep 04 '15 at 07:15
  • I think that Nicholas tried to apply Morgans law (https://en.wikipedia.org/wiki/De_Morgan's_laws) and repaired the code at the same time. But still it is not OK and the answer should be recoded in something less confusing and more simple. Try and fail could be a good lesson to understand the logic ;). – mcane Sep 04 '15 at 08:12
  • I apply De Morgans Law >_< I changed it and then remembered that there was another step to change `or` to `and` but didn't think that that's not what was asked.. Sorry – Nicholas Robinson Sep 04 '15 at 12:43