import java.util.Scanner;
public class ticTacToe {
public static Scanner input;
public static void main(String[] args) {
input = new Scanner(System.in);
char p1 = ' ';
char p2 = ' ';
boolean gameOver = false;
char[][] board = new char[3][3];
int pos1 = 0, pos2 = 0;
// Welcome the user to the game
System.out.println("----- Welcome to Tic-Tac-Toe! -----");
System.out.println("Would you like to play as x's or o's?");
p1 = input.next().charAt(0);
System.out.println("Would you like to go first or first?");
String first = input.next();
while (checkLetter(p1) == false) {
System.out.println("Invalid input. Please choose either x or o.");
p1 = input.next().charAt(0);
}
p2 = setCompLetter(p1);
// game loop
while (gameOver == false) {
// Player 1 turn
player1(p1, p2, board);
// Check P1 victory
if (checkP1Victory(board, p1) == true) {
gameOver = true;
System.out.println("Player 1 wins");
break;
}
// Player 2 turn
player2(p1, p2, board);
// Check p2 victory
if (checkP2Victory(board, p2) == true) {
gameOver = true;
System.out.println("Player 2 wins!");
break;
}
// Check for draw
if(checkDraw(p1, p2, board) == true){
System.out.println("It is a draw!");
break;
}
}
}
// Checks to see if the user input is valid when they select a letter
public static boolean checkLetter(char letter) {
if (letter != 'x' && letter != 'o') {
return false;
}
return true;
}
// Sets the computer letter based on the user input
public static char setCompLetter(char p1) {
if (p1 == 'x') {
return 'o';
} else {
return 'x';
}
}
// Prints the board to the console
public static void printBoard(char[][] board) {
for (int i = 0; i < board.length; i++) {
for (int j = 0; j < board[i].length; j++) {
System.out.print("[ " + board[i][j] + " ]");
}
System.out.println();
}
}
// Player 1 turn sequence
public static void player1(char p1, char p2, char[][] board) {
//System.out.println("Player 1 turn.");
System.out.println("Enter the position: ");
int pos1 = input.nextInt();
int pos2 = input.nextInt();
// Checks to see if the user entered a position that is in bounds
while ((pos1 >= 3) || (pos2 >= 3)) {
System.out.println("Out of bounds index, re-enter a position");
pos1 = input.nextInt();
pos2 = input.nextInt();
}
// Checks to see if there is an x or o in the position
while ((board[pos1][pos2] == p1) || (board[pos1][pos2] == p2)) {
System.out.println("Already something in that position, enter another.");
pos1 = input.nextInt();
pos2 = input.nextInt();
}
board[pos1][pos2] = p1;
printBoard(board);
}
// Player 2(CPU) sequence
public static void player2(char p1, char p2, char[][] board) {
int pos1 = (int) (Math.random() * 3);
int pos2 = (int) (Math.random() * 3);
while ((board[pos1][pos2] == p1) || (board[pos1][pos2] == p2)) {
pos1 = (int) (Math.random() * 3);
pos2 = (int) (Math.random() * 3);
}
board[pos1][pos2] = p2;
System.out.println("");
printBoard(board);
}
// Checks the p1 victory
public static boolean checkP1Victory(char[][] board, char p1) {
// Checks player 1 row victory
for (int i = 0; i < board.length; i++) {
if ((board[i][0] == p1) && (board[i][1] == p1) && (board[i][2] == p1)) {
return true;
}
}
// Checks Player 1 verticle victory
for (int j = 0; j < board.length; j++) {
if ((board[0][j] == p1) && (board[1][j] == p1) && (board[2][j] == p1)) {
return true;
}
}
// Checks Player 1 diagonal victory(left to right)
if ((board[0][0] == p1) && (board[1][1] == p1) && (board[2][2] == p1)) {
return true;
}
// Checks Player 1 diagonal victory(right to left)
if ((board[0][2] == p1) && (board[1][1] == p1) && (board[2][0] == p1)) {
return true;
} else {
return false;
}
}
public static boolean checkP2Victory(char[][] board, char p2) {
// Checks player 2 row victory
for (int i = 0; i < board.length; i++) {
if ((board[i][0] == p2) && (board[i][1] == p2) && (board[i][2] == p2)) {
return true;
}
}
// Checks Player 2 verticle victory
for (int j = 0; j < board.length; j++) {
if ((board[0][j] == p2) && (board[1][j] == p2) && (board[2][j] == p2)) {
return true;
}
}
// Checks Player 2 diagonal victory(left to right)
if ((board[0][0] == p2) && (board[1][1] == p2) && (board[2][2] == p2)) {
return true;
}
// Checks Player 2 diagonal victory(right to left)
if ((board[0][2] == p2) && (board[1][1] == p2) && (board[2][0] == p2)) {
return true;
} else {
return false;
}
}
public static boolean checkDraw(char p1, char p2, char[][] board){
int count = 0;
for(int i = 0; i < board.length; i++){
for(int j = 0; j < board[i].length; j++){
if(board[i][j] == 'x' || board[i][j] == 'o' ){
count++;
if(count == 9){
return true;
}
}
}
}
return false;
}
}
2 Answers
Use conventional names.
Class names in Java use the CamelCaps notation, i.e. the class name should be TicTacToe
, not ticTacToe
.
Encapsulate / hide fields, and consider making them final
.
Your field input
is public
, which means every other class could access it. It's not final
, which means it could be changed from anywhere in the code. You might consider using private static final Scanner input = new Scanner(System.in);
instead.
Encapsulate / hide methods
All of your methods are public
, but the way they are written, they should (apart from unit tests) never be accessed from anywhere else. In such a case, making them public
might not make sense.
Consider using OO
All your data is passed around between methods via arguments. Consider creating an instance of your TicTacToe
class and turning parameters which are passed around frequently, like the board
, into instance fields.
The input validation in player1()
is bogus
I can enter a position which is out-of-bounds by first entering a position at which there already is something, and then enter a position which is out-of-bounds.
I can also enter a position which is out-of-bounds and crashes the program by entering negative numbers.
Spelling
vertical
is neither a beer or body part ;) (change verticale
and verticle
to vertical
).
Redundant code
Did you notice that the contents of checkP1Victory()
and checkP2Victory()
are identical? You need only one of these two methods, and can make it generic for any player. This also seems more logical from the point of view of tic tac toe rules: The rules for winning are the same for player1 and player2.
Extract methods
In checkP1Victory()
/ checkP2Victory()
, I'd extract 3 methods.
I'd extract the first loop into checkHorizontalVictory()
, the second loop into checkVerticalVictory()
, and the two if
statements into checkDiagonalVictory()
.
This would simplify the code significantly, and you could get rid of comments.
Avoid check
in method names.
check
is a bad word to use in methods. There is a concept called CQS - Command-Query-Separation that we want to follow with methods. Methods should be either commands or queries but not both. Commands are void methods that don't return anything, but they have side-effects. Queries return something but usually do not have side-effects. The name of a method should tell whether it is a command or a query, and with check
that's problematic. It sounds like a command, but in your case it is a query.
How about hasPlayerWon
instead of checkPlayerVictory()
(resp. checkP1Victory()
/ checkP2Victory()
)?
Notice how nice that reads in if
-statements:
if (hasPlayerWon(p2)) { ... }
Food for thought: If you go for a little more OO and delegation, you could even end up with code which reads as nicely as:
if (player2.hasWon(game)) { ... }
Truth doesn't need to be compared
Instead of while (gameOver == false)
use while (!gameOver)
. Instead of if (xyz == true)
use if (xyz)
.
Unused variables and values.
In method main()
, the variables pos1 and pos2
are unused and can be removed.
In method main()
, the initial values ' '
of variables p1
and p2
are unused and can be removed.
In method main()
, the variable first
is unused, and because it's initialized from input.next()
, I believe it's a bug.
Method length
Consider splitting method main()
, it is very long. Good method lenghts in Java are 1-3 lines, sometimes a little bit longer, but hardly ever more than 6-7 lines.
Consider changing or even removing checkDraw
method.
This is a suggestion to the algorithm. Currently you determine whether the game is a draw by detecting that the all 9 fields of the board contain either 'x' or 'o'.
You could also find out the inverse faster, by finding out that one of the fields is not 'x' or 'o'.
Then you would not have to inspect the entire board, but could stop looking after you found the first ' '. It would also get rid of the count
variable.
Alternatively, you could also count the number of moves in the main()
method. If it reaches 9
, it's a draw.
Consider using constants.
Mistyping 'x'
as 'X'
or 'o'
as 'O'
or '0'
is all too easy and can lead to long long debugging sessions if you do not have unit tests (and it seems you do not have unit tests).
You could introduce constants
private static final char PLAYER1 = 'x';
private static final char PLAYER2 = 'o';
Then replace all other occurrences of 'x'
and 'o'
with PLAYER1
and PLAYER2
. This can make the code easier to read, and it also makes the code easier to maintain in case your requirements change, i.e. because your customer / client / teacher / whoever wants to have something else instead of 'x'
and 'o'
.
Consider using an enum
instead of char
for your board
.
Your board
fields can only ever have three valid states:
* ' '
* 'o'
* 'x'
However, the type char allows not 3 but 65536 different states, leaving each field potentially with 65533 invalid states. Parts of your code will inevitably be busy with preventing these 65533 invalid states from ever happening.
An enum
is a class
with a predefined set of instances. Here are two options:
- You could have an
enum
with two values, and represent the empty field withnull
. - You could have an
enum
with three values, one of them being the empty field.
Here's the enum
for the first option:
enum Player {
PLAYER1('x'),
PLAYER2('o');
private final char displayChar;
Player(char displayChar) {
this.displayChar = displayChar;
}
public char getChar() {
return displayChar;
}
}
Both approaches have pros and cons, and enum
might be new to you, so I'm not sure whether you really want to consider it already. But if you do, or even in the failed attempt of doing so, you might actually learn a lot, so it might be worth trying.
SRP violations
The SRP is the Single Responsibility Principle, and your methods violate it: They contain I/O and business logic at the same time. I/O and business logic are separate concerns and thus should be separated from each other. Imagine you want to create a graphical version, much of your code couldn't be reused without modification. And it turns out that reusable is a synonym to testable, and both are a subset of maintainable.
More Ideas for playing with the code further to learn more.
- Allow human vs human and pc vs pc games.
- Allow players to pick their symbols.
- Split your program into multiple classes.
Consider unit testing, consider TDD
Title says it all.
-
\$\begingroup\$ Wow thank you so much for taking the time out to review and give such extensive feedback of my code. Looks like I have a lot to learn so I will get crackin! \$\endgroup\$ Jan 5, 2016 at 5:24
It looks like you are well aware of your IDEs formatting feature - the code is very well formatted.
DRY
checkP1Victory and checkP2Victory can be condensed into a single function, checkVictory. The logic is exactly the same, the only difference is the player token being checked.
Conditionals
Favor using the not (!) operator over checking explicitly for false.
Printing
Favor building strings and only using System.out.println on the result of a method call. For example, printBoard should return a String. Use a StringBuilder.
Separate concerns
Try to extract the idea of a TicTacToe game out into a class. Don't have any I/O in the class itself (no prints!) - you should be able to create a Driver (main class) that uses this TicTacToe game. The driver could use plain console I/O, like it is now, or possibly use a GUI approach. It could even simulate a game between two computers. Separate the logic from the input method, and you'll have more robust code.
Enum
An enumeration would be a better choice for marking cells, instead of using a character. You would have X, O, and then Empty. There is a good argument to be made to use something like PlayerOne, PlayerTwo and Empty, since it is the driver which would control how things are displayed. Perhaps your client wants to use Dogs and Cats, not Xs and Os.
-
1\$\begingroup\$ Thank you for your input! I dont think I have ever encountered Enums before so I will have to look into that. I will start to revise! \$\endgroup\$ Jan 5, 2016 at 5:23
-
\$\begingroup\$ If you wish to receive more feedback, consider submitting another question with your new code. More pointed feedback can be given after you solve the bigger issues (sep. concerns, DRY) :) \$\endgroup\$ Jan 14, 2016 at 17:20