开发者

Is there a neater way to do this? Java IF

开发者 https://www.devze.com 2023-02-10 12:57 出处:网络
public void moveRowItemToBottomIfAllowed(int r, int f) { int i = rows[r].peek().getType(); int j = 0; if (bottom[f].isEmpty()) {
public void moveRowItemToBottomIfAllowed(int r, int f) {
    int i = rows[r].peek().getType();
    int j = 0;
    if (bottom[f].isEmpty()) {
        for (int k = 0; k < 4; k++) {
            if ((k == f) || (bottom[k].isEmpty()) || (bottom[k].peek().getType() != i)) {
                continue;
            }
            j = 1;
        }
        if (j == 0) {
            bottom[f].push(rows[r].pop());
        }
    } else if ((!bottom[f].isEmpty()) && (rankTrueFalse(rows[r].peek(), bottom[f].peek())) &开发者_StackOverflow社区& (rows[r].peek().getType() == bottom[f].peek().getType())) {
        bottom[f].push(rows[r].pop());
    }
}

As I'm still learning java I've been putting together some rules for a game, I went through how to do it logically and came up with the above code which works correctly but it looks like a bit of a mess - is there any neater way or a more efficient way of writing this code? any pointers are much appreciated.


I would extract methods to make the code more readable. At first sight I would extract

  • the for loop, or probably the whole contents of the if block,
  • the expression from the 2nd long else if

Use descriptive names for your new methods (and for your variables too, for that matter). This makes a huge difference in readability.


I would recommend that you use more descriptive names for your variables. What is r? What is f? I'm guessing that f is some sort of numeric representation of the suit, since you compare it to k, which iterates over four values.

There might be more to say about the code overall, but the first step is to write the code in a self documenting manner.


There are bits of expressions which could be extracted into local variables: rows[r].peek() and bottom[f].peek() being the most obvious ones.


It looks like you are using j as a flag. Booleans are better for that, but you can return early instead of setting the condition that guards the rest of the processing, getting rid of j entirely.

You're double checking that bottom[f].isEmpty()) is false, and can use the already looked up i instead of repeating rows[r].peek().getType()

Both sides of your first condition, if they end up doing the processing, do the same processing, which you can write once:

public void moveRowItemToBottomIfAllowed(int r, int f) {
    int i = rows[r].peek().getType();

    if (bottom[f].isEmpty()) {
        for (int k = 0; k < 4; k++) {
            if (k == f) continue;
            if (bottom[k].isEmpty()) continue;
            if (bottom[k].peek().getType() == i) return;
        }
    } else {
        if (!rankTrueFalse(rows[r].peek(), bottom[f].peek())) return;
        if (bottom[f].peek().getType() != i) return;
    }

    bottom[f].push(rows[r].pop());
}

This code is then structured as a bunch of guards with early exits, followed by the processing.

The guards could then be extracted into their own method, leaving:

public void moveRowItemToBottomIfAllowed(int r, int f) {
    if (moveIsAllowed(r,f)) bottom[f].push(rows[r].pop());
}
0

精彩评论

暂无评论...
验证码 换一张
取 消