Can method be readable and shorter ?

I wrote this method:

private int maxSequence (char player , Cell c)
{
    int row = c.getRow();
    int col = c.getCol();
    int maxVert = 0;
    int maxHor = 0;
    int maxDiag = 0;

    if (player == 'O')
    { 

        for (int j = 0; j < _board[0].length; j++)
        {
            if ( (_board[col][row+j] == 'O') || (_board[col][row-j] == 'O') )
            {
                maxVert++;
            }

            if ( (_board[col+j][row] == 'O') || (_board[col-j][row] == 'O') )
            {
                maxHor++;
            }

            if ( (_board[col+j][row+j] == 'O') || (_board[col-j][row-j] == 'O') )
            {
                maxDiag++;
            }
        }
    }

    if (player == 'X')
    {
        for (int j = 0; j < _board[0].length; j++)
        {
            if ( (_board[col][row+j] == 'O') || (_board[col][row-j] == 'X') )
            {
                maxVert++;
            }

            if ( (_board[col+j][row] == 'O') || (_board[col-j][row] == 'X') )
            {
                maxHor++;
            }

            if ( (_board[col+j][row+j] == 'O') || (_board[col-j][row-j] == 'X') )
            {
                maxDiag++;
            }
        }
    }

    if ( (maxDiag >= maxVert) && (maxDiag >= maxHor) )
    {
        retur开发者_如何转开发n maxDiag;
    }

    else if ( (maxVert >= maxDiag) && (maxVert >= maxHor) )
    {
        return maxVert;
    }

    else
    {
        return maxHor;
    }
}

I wonder if there is a way to improve the method to be readable and \ or shorter ?

---

Using O as a variable name is a bad idea. O can easily misread or mistyped as 0 or vice versa, as you appear to have done in a couple of places.

In this case, I think you should use something like O_PLAYER and O_MARKER instead of O, for the reason above, and also to distinguish the two different meanings of the constant in your code. (In fact, depending on the larger context, I might have created a couple of enum types for these two cases.)

Putting the opening curly braces on the previous line saves space and is (IMO) just as readable; e.g.

if (cond) {
   // blah
} else {
   // blah
}

versus

if (cond) 
{
   // blah
} 
else 
{
   // blah
}

Prefixing instance variables with _ violates the Java naming standard.

Finally, the last 12 lines could be rewritten as:

return Math.max(maxHor, Math.max(maxVert, maxDiag));

---

First of all, you may want to follow java coding convention especially for namings (meaningful names and no underscores...) and also you can remove the extra parenthesis around the boolean evaluations like below.

Also you can merge the for loops into one as they are doing real similar job

if (board[col][row + j] == O || board[col][row - j] == O) 


        if (maxDiag >= maxVert && maxDiag >= maxHor) {
            return maxDiag;
        }else if (maxVert >= maxDiag && maxVert >= maxHor) {
            return maxVert;
        }else {
            return maxHor;
        }

---

Aside from the fact that this won't compile (unless X and O are instance variables) ...

How does the code for player 'O' differ from player 'X'? The answer is the second expression in each if statement ... replace it by a variable, and you've cut the code in half.

However, a better approach is to not have those conditionals at all. But that requires some thought as to how you might do a Tic-Tac-Toe AI for any arbitrary place on the board.

---

private int maxSequence (char player , Cell c)
{
    int row = c.getRow();
    int col = c.getCol();
    int maxVert = 0;
    int maxHor = 0;
    int maxDiag = 0;

    for (int j = 0; j < _board[0].length; j++)
    {
        if ( (_board[col][row+j] == O) || (_board[col][row-j] == player) )
        {
            maxVert++;
        }

        if ( (_board[col+j][row] == O) || (_board[col-j][row] == player) )
        {
            maxHor++;
        }

        if ( (_board[col+j][row+j] == 0) || (_board[col-j][row-j] == player) )
        {
            maxDiag++;
        }
    }

    return Math.max(maxDiag, Math.max(maxVert, maxHor));
}

---

To build complex one-liners is in most cases a bad idea. However - in some cases like here, you make it easy for the eyes to see, what is common over multiple rows, and what is different:

private int maxSequence (char player , Cell c)
    {
        int row = c.getRow();
        int col = c.getCol();
        int maxVert = 0;
        int maxHor = 0;
        int maxDiag = 0;

        if (player == O)
        {
            for (int j = 0; j < _board[0].length; j++)
            {
                if ((_board [col]  [row+j] == O) || (_board [col]  [row-j] == O))  maxVert++;
                if ((_board [col+j][row]   == O) || (_board [col-j][row]   == O))  maxHor++;
                if ((_board [col+j][row+j] == 0) || (_board [col-j][row-j] == O))  maxDiag++;
            }
        }
        if (player == X)
        {
            for (int j = 0; j < _board[0].length; j++)
            {
                if ((_board [col]  [row+j] == O) || (_board [col]  [row-j] == X))   maxVert++;
                if ((_board [col+j][row]   == O) || (_board [col-j][row]   == X))   maxHor++;
                if ((_board [col+j][row+j] == 0) || (_board [col-j][row-j] == X))   maxDiag++;
            }
        }
        if ((maxDiag >= maxVert) && (maxDiag >= maxHor))    return maxDiag;
        if ((maxVert >= maxDiag) && (maxVert >= maxHor))    return maxVert;
        return maxHor;
    }

After resorting the lines, I observed that in the first block, you test for OO,OO,OO, and in the second block for OX,OX,OX, and I doubt, you would like to test for XX,XX,XX.

Without compilation, in a second view I see, that there is a OO0, where I would expect an OOO (wasn't it XXX, like mentioned above). :) - Ah, but that pattern is used in the pure O-Block too.

I like to append, that I like Stehpen C's idea to express max(a, b, c), that the first two blocks should be replaced by an parametrized block, and that an bigger picture of the whole program could lead to a complete different design, more object oriented.

Combining the suggestions, we intermediately stop here:

private int maxSequence (char player, Cell c)
{
    int row = c.getRow();
    int col = c.getCol();
    int maxVert = 0;
    int maxHor = 0;
    int maxDiag = 0;

    for (int j = 0; j < _board[0].length; j++)
    {
        if ((board [col]  [row+j] == player) || (board [col]  [row-j] == player))  maxVert++;
        if ((board [col+j][row]   == player) || (board [col-j][row]   == player))  maxHor++;
        if ((board [col+j][row+j] == player) || (board [col-j][row-j] == player))  maxDiag++;
    }
    return Math.max (maxHor, Math.max (maxVert, maxDiag));
}

Intermediately, because we might do a completely different design - I'm smelling a Board-class and a Player-class at least.

---

Not a full answer here, sorry, drinking coffee and sort of lazy this morning (also this kind of looks like an school problem). I'll therefore keep it high level.

1. Don't be afraid of methods, they can improve clarity a lot. Your for loops can be encapsulated in a method where the name explains what's going on.
2. Avoid using magic values, replace the 'X' chars with a constant.
3. Even better wrap the char with a player class. Player.for('x') returns a player class. Delegate the behavior in your loops to an XPlayer, YPlayer classes.
4. Like other posters have pointed out use Math.max and other built in functions where possible