9

Background: I am making a chess game. It works almost completely, just missing the check for checkmate, but I am refining some code for readability, etc.
Right now, I am recoding a method I had to check the path from any piece, to another location on the board. It returns true if there is a piece blocking the path and false if there is not one.
Note: I do not need to check the spot of the last location because my game will check to make sure that you do not occupy the spot you are trying to move to.

Also note that I have researched this question and I have found that the consensus, mainly on here, is that breaking is the correct solution. This is preferred over having a boolean variable initialized outside the loop and set true or false and breaking the loop on that value. However, I have two conditions in my loop that may make my code return true or return false so I can't do that completely.

Current Code 

public boolean isPathClear(Location l1, Location l2) {

    int atx = l1.getX(); 
    int aty = l1.getY(); 
    int xdiff = 0, ydiff = 0;
    int endx = l2.getX(), endy = l2.getY();

    if(l1.getX() > l2.getX()) {
        xdiff = 1;
    }
    if(l1.getX() < l2.getX()) {
        xdiff = -1;
    }
    if(l1.getY() > l2.getY()) {
        ydiff = 1;
    }
    if(l1.getY() < l2.getY()) {
        ydiff = -1;
    }

while(true) {
        atx += xdiff; 
        aty += ydiff;

        if(atx == endx && aty == endy) {
            return true 
        }

        if(board[atx][aty].getType() != ' ') {
            return false; 
        }
    }

Problem: Since breaking is the preferred method, that is what I planned to do. But, I came into a problem, if I use break in one of the if statements, it looks like I have to return in the other. Like:

while(true) {
        atx += xdiff; 
        aty += ydiff;

        if(atx == endx && aty == endy) {
            break;
        }

        if(board[atx][aty].getType() != ' ') {
            return false; 
        }
    }

    return true;

Question: This seems like a sort of mix that could be confusing. So with my situation, would this still be the preferred method over this code:

boolean clear;

    while(true) {
        atx += xdiff; 
        aty += ydiff;

        if(atx == endx && aty == endy) {
            clear = true;
            break;
        }

        if(board[atx][aty].getType() != ' ') {
            clear = false;
            break;
        }
    }

    return clear;
Matt C
  • 4,470
  • 5
  • 26
  • 44
  • 3
    I wouldn't use `while (true)` at all, but I think I'm in the minority on that. :-) – T.J. Crowder Dec 19 '13 at 17:37
  • Can you please show your whole method? What is the code supposed to do? – Behe Dec 19 '13 at 17:37
  • why do you say is breaking the preferred method? – bcorso Dec 19 '13 at 17:38
  • 2
    There is a good discussion on this entire topic here: – Brian Clapper Dec 19 '13 at 17:38
  • The question is why is `break`in Java anyway ? `break`is not mandatory for loops (but for `switch`), but it allows in some situation cleaner code by avoiding overloading of the continuing condition. Usage of `break`is valid if you dont overstress it. – PeterMmm Dec 19 '13 at 17:40
  • If `xdiff = Math.abs(endx-atx);`, and `atx = atx + xdiff` then won't `atx` *always* equal `endx` as long as `endx`>`atx` because of the absolute value (and same for y), therefore making the first if statement always true, really, if the piece is moving in the positive x/y direction? – asaini007 Dec 19 '13 at 17:41
  • @Behe, I explained what it is supposed to do. And I added the one line of code missing, the signature. – Matt C Dec 19 '13 at 17:41
  • @bcorso, mainly because of this thread http://stackoverflow.com/questions/3922599/is-it-a-bad-practice-to-use-break-in-a-for-loop – Matt C Dec 19 '13 at 17:42
  • Is this not even the whole method? `l2` and `team` aren't used – asaini007 Dec 19 '13 at 17:42
  • A possible problem with `return` from a loop (or anywhere else besides the end) is that if you decide to add some code later that's supposed to be executed just before your method returns, you might miss that it's returning early in other cases. That shouldn't be a problem for a small method, and if it's bigger, maybe it should be broken up anyway. Plus, if it's a case where some cleanup needs to be performed before returning, you can use `try`...`finally` or try-with-resources. – ajb Dec 19 '13 at 17:47
  • 1
    The thing to aim for is consistency I think. I would do the first or last example(first one, myself), but not the second. Either way, staying consistent is more reader-friendly. – DoubleDouble Dec 19 '13 at 17:50
  • @asaini007, I fixed the method. Looks like I grabbed some code from another method when I was copying/pasting. But it still doesn't change the context of the question at all. – Matt C Dec 19 '13 at 17:52
  • @ajb, a lot of my professors have advised against using try... unless you are specifically working with exceptions, and even to be very wary then. What's your standing behind using try...finally? – Matt C Dec 19 '13 at 17:55
  • 2
    For a mental challenging development, go for **shortest and best readable**. That would probably mean: returns. Such a discussion is a waste of time and energy for your _impetus_ to produce something nice and intellectual. – Joop Eggen Dec 19 '13 at 17:55
  • @PeterMmm, so which would you prefer in this situation? You see how much I am stressing it, I can't determine what you think of as "overstressing". – Matt C Dec 19 '13 at 18:05
  • @MatthewC Did they recommend against `try`...`finally`, or just `try`? Using `try`...`finally` has a very different purpose from `try`...`catch`, although you can combine the two. The latter is for handling exceptions; the former is for making sure needed cleanup gets done even if the method terminates unexpectedly. For example, if your method calls another method and that method throws a runtime exception, `finally` will make sure cleanup is done even if the exception is thrown over your method's head, so to speak. – ajb Dec 19 '13 at 19:27
  • @MatthewC P.S. whatever I said about `try`...`finally` applies to try-with-resources too. – ajb Dec 19 '13 at 19:34

5 Answers5

2

Instead of while(true), you can try to handle your actual conditions inside the while loop and you don't need additional break statements:

public boolean isPathClear(Location l1, Location l2) {
    int atx =  l1.getX(), aty =  l1.getY();
    int endx = l2.getX(), enxy = l2.getY();

    int xdiff = Integer.signum(endx - atx);
    int ydiff = Integer.signum(endy - aty);

    do{
        atx += xdiff;
        aty += ydiff;
    } while(!(atx == endx && aty == endy) && board[atx][aty].getType() == ' ');

    return atx == endx && aty == endy;
}
bcorso
  • 45,608
  • 10
  • 63
  • 75
  • I can't use the ? operator in this case, although that code does look very nice. The reason it won't work is because xdiff and ydiff might be 0. If movement is along the x-axis or y-axis and not diagonally, one of them will be 0. The shortest code you could make for that case would be two more statements like `int xdiff = atx > 0 ? 0 : atx` – Matt C Dec 19 '13 at 18:11
  • Also, you can do int xdiff = atx == endx ? 0: (atx > endx ? 1:-1); in a single statement, but it does start to look convoluted. – bcorso Dec 19 '13 at 18:23
  • Also, I've adjusted the while loop to handle both conditions inside while(). – bcorso Dec 19 '13 at 18:29
  • Also, I suspect that no matter how you implement this, you'll want to change your increment step to 'atx += atx == endx ? 0:xdiff' (and similarly for y) to handle the cases where x and y don't stop at the same time. – bcorso Dec 19 '13 at 18:46
  • Could you elaborate on the change to the incrementation? – Matt C Dec 19 '13 at 18:47
  • Suppose atx=1, endx=2, aty=1, endy=4. Then the while loop will loop 3 times until aty == endy, and (the way it is coded now) atx will also be incremented 3 times even though it should only be incremented once. – bcorso Dec 19 '13 at 18:50
  • You don't have to worry about that. All movements that are passed to this method will either be straight or diagonal. The only piece that can move in a line that is not straight or diagonal is the Knight, and I do not call this method when moving the Knight. – Matt C Dec 19 '13 at 19:01
  • Ah I see, well in any case, the while loop I have now removes the needs for breaks completely and also has a single exit point. – bcorso Dec 19 '13 at 19:03
  • @MatthewC if you can't use ? operator, can you use Integer.signnum ? I think that is easier to understand than the ? operator so I've edited the code. – bcorso Dec 19 '13 at 19:10
1

Mostly I'd say it's a subjective thing.

One objective advantage to your last code sample is that it gives you a single point of exit from the method, which is frequently useful for debugging. A guy I used to work with who did military software previously said it was a requirement they had to work to: A method must only have a single exit point. His claim was that it was a robustness/reliability/maintainability thing. I just like having that easy "here's where it leaves" thing.

T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875
  • Just because a requirement is for for military software, doesn't make that requirement a good one. Or that it even makes sense. – Dodd10x Dec 19 '13 at 17:57
  • @Dodd10x: No argument there. In this case, I'd say there were good reasons for it, though. – T.J. Crowder Dec 19 '13 at 18:13
  • @Dodd10x, Nobody said it was a good requirement. The poster simply pointed out that one style might have one advantage in this case. – Matt C Dec 19 '13 at 18:17
  • @MatthewC - TJ has since expanded on his answer, but my point was simply saying the military requires it doesn't make it correct. In fact, many military contracts require the waterfall development model still - and I think most people on stackoverflow would argue against that. – Dodd10x Dec 19 '13 at 18:42
1

Use return statements. The idea is to keep it readable and maintainable. Just don't make different method calls inside those if statements and you will be fine.

Read this post from Bruce Eckel: http://onthethought.blogspot.com/2004/12/multiple-return-statements.html

Another benefit of using return statements is that another programmer is less likely to come along and modify your local variable, causing an incorrect value to be returned.

Dodd10x
  • 3,344
  • 1
  • 18
  • 27
1

If you want to do it using break - it doesn't really matter - but I prefer this:

boolean notObstructed = true;
while(notObstructed) {
    atx += xdiff; 
    aty += ydiff;
    if(atx == endx && aty == endy)
        break;
    if(board[atx][aty].getType() != ' ')
        notObstructed = false;
}
return notObstructed;

This way there's only one return statement whose value is configured based on the while loop and if statements.

asaini007
  • 858
  • 5
  • 19
0

I prefer using return in these kinds of loops as i think breaks would just be a waste. Keep a return statement at the end of your method, and use 2 regular returns in your if statements that will either return true or false.

Also last I remember coding something similar to this, since the return type of my methods were void, i could simply use:

return;

and it would exit the loop and the method.

ThaBomb
  • 702
  • 6
  • 11