JAVA Comparing two Strings isn't working

Question

So here's a snippet of code I'm working on:

String direction = s.readLine();
System.out.println(direction);
if (direction.equals("up") != true && direction.equals("down") != true &&
  direction.equals("left") != true && direction.equals("right") &&
  direction.equals(null) != true) {
    System.out.println("Invalid Solution file");
    System.exit(0);
}

What it is supposed to do is read a line from a text file (using a BufferedReader) and then if the line isn't either a valid direction or blank then it should print "Invalid Solution" and exit.

The problem is that no matter what the direction string is the if statement still runs. I put in a println to check whether the direction was being read correctly but it seems absolutely fine. So why isn't the code working as intended?

Also, != true looks weird. Use the ! ("Not") operator on each term, instead. — Edward Peters
– Edward Peters, Commented Aug 9, 2016 at 19:40
direction.equals(null) != true are you trying to compare with empty string? that should be direction.equals("") != true — tanjir
– tanjir, Commented Aug 9, 2016 at 19:41
@mmm That isn't correct based on his problem, he does indeed want && because he is trying to find string entries that match none of the valid directions — James Wierzba
– James Wierzba, Commented Aug 9, 2016 at 19:41

Bohemian · Accepted Answer · 2016-08-09 20:07:04Z

5

Part of your problem is readability. Fix that and your problem is 90% solved:

private static List<String> DIRECTIONS = Arrays.asList("up", "down", "left", "right");

then

if (!DIRECTIONS.contains(direction)) {
    System.out.println("Invalid Solution file");
    System.exit(0);
}

The other 10% was how to check for null, which is direction == null, but if you use this code you don't need to, because contains(null) will conveniently return false.

edited Aug 9, 2016 at 20:07

answered Aug 9, 2016 at 19:48

Bohemian♦

427k103 gold badges603 silver badges750 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

GhostCat · Accepted Answer · 2016-08-09 19:49:41Z

You code is much more complex than it is needs to.

Consider this instead:

Set<String> validDirections = new HashSet<>(Arrays.asList("up", "down", ...
if (validDirections.contain(direction.toLowerCase()) {
  // good ...
} else {
  // bad ..
}

You can make validDirections a global constant for example; so it could be used in other places as well.

What I am trying to explain here is: your code is low-level. Low level code is hard to write, read, maintain and extend. Programming is always about creating good abstractions. Or vice versa: if you don't use abstractions, you end up with pretty abstract code, like the one you are showing here!

For example: if you need another direction, you have to put into your already way too complicated if condition. In my solution, you just put it into the statement that builds that Set.

Finally: your error message, is saying nothing. So, that string is bad; but why is it? Wouldn't it be better to at least print the string that caused the error?!

davidxxx · Accepted Answer · 2016-08-09 19:49:41Z

1

Here && direction.equals("right") I think you have done a mistake since it is on contradiction with the rest :

direction.equals("up") != true && 
direction.equals("down") != true && 
direction.equals("left") != true

You test the negation in the most of conditions but direction.equals("right") tests the affirmation.

Try it , it's the same thing but less verbose and more readable :

if (direction !=null && !direction.equals("up") && 
                        !direction.equals("down") &&  
                        !direction.equals("left") && 
                        !direction.equals("right") ){
           System.out.println("Invalid Solution file");
           System.exit(0);
       }

edited Aug 9, 2016 at 19:49

answered Aug 9, 2016 at 19:43

davidxxx

132k23 gold badges231 silver badges228 bronze badges

Comments

nhouser9 · Accepted Answer · 2016-08-09 19:47:51Z

0

First, you should not use != true with a boolean statement, it is bad form. Rewrite like this:

direction !=null && !direction.equals("up") && !direction.equals("down") && !direction.equals("left") && !direction.equals("right")

Your error was that you did not include the != true part on one of your statements within the compound if. Replace with the above code to solve the issue.

answered Aug 9, 2016 at 19:47

nhouser9

6,7803 gold badges23 silver badges43 bronze badges

Comments

Poorna Chander · Accepted Answer · 2016-08-09 19:50:59Z

0

I'm confused why you are using !=true when your .equals method already returns a boolean. Try this.

String direction = s.readLine();
    System.out.println(direction);
    if ( direction!=null && !direction.equals("up") && !direction.equals("down")&& !direction.equals("left")&& direction.equals("right")){
        System.out.println("Invalid Solution file");
        System.exit(0);
    }

answered Aug 9, 2016 at 19:50

Poorna Chander

635 bronze badges

Comments

Patee Gutee · Accepted Answer · 2016-08-09 19:55:02Z

0

Try the following code:

boolean match = false;

if (direction.equals("up"))
{ match = true; }
if (direction.equals("down"))
{ match = true; }
if (direction.equals("left"))
{ match = true; }
if (direction.equals("right"))
{ match = true; }
if (direction.equals(null))
{ match = true; }
if (match == false){
    System.out.println("Invalid Solution file");
    System.exit(0);
}

You might also want to trim the direction string after reading from file.

answered Aug 9, 2016 at 19:55

Patee Gutee

3042 silver badges22 bronze badges

Comments

Lukas · Accepted Answer · 2016-08-09 20:09:02Z

The quals method returns a boolean so the result does not need to be compared with the true or false value. Also, I would start with null comparison - boolean expressions in Java are shortened so if this part will be fulfilled rest of the expression is not evaluated. The correct expression might look like this:

if (direction == null || (!direction.equals("up") && !direction.equals("down") && !direction.equals("left") && !direction.equals ("right "))) {
}

But this code is not readable. You could use enums or list of Strings like below

    List<String> directions = Arrays.asList("up", "down", "left", "right");
    String direction = "readValue"
    if (!directions.contains(direction)) {
        System.out.println("Invalid direction");
        System.exit(0)
    }

Collectives™ on Stack Overflow

JAVA Comparing two Strings isn't working

7 Answers 7

Comments

Comments

Comments

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

7 Answers 7

Comments

Comments

Comments

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related