Your formatting seems to be inconsistent, use an automatic code formatter (your IDE most likely has one).
public static Scanner scan = new Scanner(System.in);
The usage of the scanner (or streams in general) should most likely be scoped. Streams are rather easily associated with native resources which must be destroyed explicitly to be freed.
public static char[][] table = {
{'1', '2', '3'},
{'4', '5', '6'},
{'7', '8', '9'}
};
Why is the table prefilled with values?
public static ArrayList<Character> picksTillNow = new ArrayList<Character>();
Try to always use the most super-class you can use and get away with, makes it easier to make sure that you're coupling classes through methods of the actual implementation instead of the interface. In this case, declare the variable with List
.
public static char playerOne = ' ';
public static char playerTwo = ' ';
You're preparing these, but you only use them in a very limited scope, you should declare them there.
for(char i = '1'; i <= '9'; i++){
picksTillNow.add(i);
}
Autoboxing.
while (true){
You could also have a boolean
like running
or playing
or gameRunning
and set it depending on the return value of `endGame().
Or, as the number of turns is fixed, you might do a mixture of both:
int turn = 0;
while (turn++ < 9 && nobodyHasWonYet) {
// Logic.
}
endGame();
That's a bad name for the method, as it does not always end the game.
for (int i = 0; i < table.length; i++) {
for (int j = 0; j < table[i].length; j++) {
I'm a persistent advocate that you're only allowed to use single-letter variable names when dealing with dimensions ("x", "y", "z"), and that excludes using i
and j
). In this case actually, using x
and y
as variable names would improve the readability of the code. Even better would be using row
and column
.
if (table[i][2] != 9) {
System.out.println();
}
But the default value of this field is overwritten at some point, isn't it?
}else{
playerOneChoice();
}
You're recursing here, so keeping to hit Return without entering something should at some point crash your game because of a stackoverflow.
public static void endGame(){
for(int i = 0; i < table.length; i++){
if(table[i][0] == table[i][1] && table[i][1] == table[i][2] && table[0][i] != 'О'){
System.out.println("Победа!");
System.exit(0);
}
}
for(int j = 0; j < table.length; j++){
if(table[0][j] == table[1][j] && table[1][j] == table[2][j] && table[j][0] != 'O'){
System.out.println("Победа!");
System.exit(0);
}
}
if(table[0][0] == table[1][1] && table[1][1] == table[2][2] && table[0][0] != 'O'){
System.out.println("Победа!");
System.exit(0);
}
if(table[0][2] == table[1][1] && table[1][1] == table[2][0] && table[0][2] != 'O'){
System.out.println("Победа!");
System.exit(0);
}
if(picksTillNow.isEmpty()) System.exit(0);
}
If I don't completely suck at math (and Tic Tac Toe), there should only be 8 winning positions, and even then there are only 4 and the other 4 are mirrored or rotated. So it might be more interesting and easier to maintain if these are hardcoded.
System.exit(0);
Something to keep in mind is that System.exit
is not "exit the application" but "kill the JVM process". When invoking this not even finally
block may run. In this case it does not matter, but something to keep in mind.
If I read this right, you have the playing field with the numbers in it, which then get replaced with the player choices. Whether it is valid choice is validated through the remaining fields in your list. Instead I suggest you keep the state limited to only the playing field. Assuming your field definition, you can check whether a field is still free by calculating the position of the input and then checking whether there is an O
or X
placed, like this:
int fieldChoice = getChoice();
int row = fieldChoice % 3;
int column = fieldChoice / 3; // Might be other way around...never can remember.
char selectedField = playingField[row][column];
if (selectedField != 'X' && selectedField != 'O') {
// Set it.
} else {
// No dice.
}
playerOneChoice
and playerTwoChoice
can be rolled into one, by passing the wanted field player as parameter. Using my above example:
public static void playerChoice(char playerCharacter) {
int fieldChoice = getChoice();
int row = fieldChoice % 3;
int column = fieldChoice / 3; // Might be other way around...never can remember.
char selectedField = playingField[row][column];
if (selectedField != 'X' && selectedField != 'O') {
playingField[row][column] = playerCharacter;
} else {
// No dice.
}
}
Having said that, your print function would then boil down to this:
for (int row = 0; row < 3; row++) {
for (int column = 0; column < 3; column++) {
System.out.println(playingField[row][column] + " ");
}
System.out.println();
}
Hardcoding the size here is not a bad thing, as long as you assume a Tic Tac Toe game with a 3x3 playing field. If you want to support larger playing fields, you'll have to adjust the rest of your logic anyway.
Coming around to checking whether somebody has won, the winning conditions would boil down to:
// Rows
playingField[0][0] == playingField[0][1] && playingField[0][1] == playingField[0][2];
playingField[1][0] == playingField[1][1] && playingField[1][1] == playingField[1][2];
playingField[2][0] == playingField[2][1] && playingField[2][1] == playingField[2][2];
// Columns
playingField[0][0] == playingField[1][0] && playingField[1][0] == playingField[2][0];
playingField[0][1] == playingField[1][1] && playingField[1][1] == playingField[2][1];
playingField[0][2] == playingField[1][2] && playingField[1][2] == playingField[2][2];
// Diagonal
playingField[0][0] == playingField[1][1] && playingField[1][1] == playingField[2][2];
playingField[0][2] == playingField[1][1] && playingField[1][1] == playingField[2][0];
You will notice, that is just as short as using your loops, and we can chain them with ||
directly for a return. However, that will not tell us who one. For that need some additional logic, namely an if
on every line. But we could also assume that the winner is the player that last played, which is a fair assumption, actually. To make it more readable we can add us three helper functions:
public static boolean isWinningRow(int rowIndex) {
return playingField[row][0] == playingField[row][1] && playingField[0][1] == playingField[row][2];
}
public static boolean isWinningColumn(int column) {
return playingField[0][column] == playingField[1][column] && playingField[1][column] == playingField[2][column];
}
public static boolean isLeftRightDiagonalWin() {
return playingField[0][0] == playingField[1][1] && playingField[1][1] == playingField[2][2];
}
public static boolean isRightLeftDiagonalWin() {
return playingField[0][2] == playingField[1][1] && playingField[1][1] == playingField[2][0];
}
This does not simplify the overall complexity, but does improve the readability:
return isWinningRow(0)
|| isWinningRow(1)
|| isWinningRow(2)
|| isWinningColumn(0)
|| isWinningColumn(1)
|| isWinningColumn(2)
|| isLeftRightDiagonalWin()
|| isRightLeftDiagonalWin();
So you could rewrite your logic to something like this:
int turn = 0;
boolean running = true;
while (turn < 9 && running) {
int currentPlayer = (turn % 2) + 1;
playerChoice(currentPlayer);
printPlayingField();
if (hasWon()) {
System.out.println("Winner: " + Integer.toString(currentPlayer);
running = false;
}
turn++;
}
I'm a little bit unhappy with the code to choose the player, but it should work. And overall should give you a good idea where to start.