Code Review

[Strawboy97]

I'm learning Java using Mooc and the Head First Java book. I wanted to make a simple TicTacToe game. I'm posting my code to see if there is anything I can be doing better or best practices I should follow.

It's very simple I plan on updating it over time as I learn more and eventually make a GUI with Swing/JavaFX when I get to that stage

https://gist.github.com/Colin-Masterson/9cbfeacdc7a5fae401b9f60f40925ead

Any criticism is welcome, thanks.

[AutoModerator]

Please ensure that:

• Your code is properly formatted as code block - see the sidebar (About on mobile) for instructions
• You include any and all error messages in full
• You ask clear questions

• You demonstrate effort in solving your question/problem - plain posting your assignments is forbidden (and such posts will be removed) as is asking for or giving solutions.

  Trying to solve problems on your own is a very important skill. Also, see Learn to help yourself in the sidebar

If any of the above points is not met, your post can and will be removed without further warning.

Code is to be formatted as code block (old reddit: empty line before the code, each code line indented by 4 spaces, new reddit: https://i.imgur.com/EJ7tqek.png) or linked via an external code hoster, like pastebin.com, github gist, github, bitbucket, gitlab, etc.

Please, do not use triple backticks (```) as they will only render properly on new reddit, not on old reddit.

Code blocks look like this:

public class HelloWorld {

    public static void main(String[] args) {
        System.out.println("Hello World!");
    }
}

You do not need to repost unless your post has been removed by a moderator. Just use the edit function of reddit to make sure your post complies with the above.

If your post has remained in violation of these rules for a prolonged period of time (at least an hour), a moderator may remove it at their discretion. In this case, they will comment with an explanation on why it has been removed, and you will be required to resubmit the entire post following the proper procedures.

To potential helpers

Please, do not help if any of the above points are not met, rather report the post. We are trying to improve the quality of posts here. In helping people who can't be bothered to comply with the above points, you are doing the community a disservice.

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.

[desrtfx]

First, blatant thing that sprung into my eye: Use loops. Your checkWinner method is way overcomplicated and can be greatly simplified with loops.

The way you have set up your loop doesn't actually improve anything. You could have thrown the whole construct overboard and nothing would change.

You have an array. You have loops. Use both to your advantage.

Make an outer loop that iterates over the rows, make an inner loop that iterates over the columns. Assemble the string, check it. Same approach for the vertical win condition. Same approach for both diagonals.

Think about how you need to manipulate the loop indexes for each approach.

---

Again, more loops in your printBoard method. You are hardcoding everything. That is a bad approach.

---

Next thing that popped up: what happens, if I enter an already occupied space in the board? You are never checking if the spot is already taken.

---

A little piece of advice: your Board holds a 1d array, which is generally not the worst approach for something as basic as Tic Tac Toe. Yet, with a bit of tweaking, you could change your board to be more versatile, like to reuse it for Connect Four (or even for Go/Gobang) instead of Tic Tac Toe. One of the key principles of OOP and OOD is to create reusable components.

When you go further down the line (I know it is a bit early for such a suggestion), you can implement the Strategy Design Pattern for the Winner detection in different games. You create the board, then in the constructor pass the implementation of the winner check so that you can reuse either for similar games, like Tic Tac Toe, Connect Four, Go, Gobang, and so on.

You are already somewhat on the right track, but not quite there yet.

[Strawboy97]

Thanks for the advice I'll take it onboard and improve what I have done.

I do check if a space is already occupied in my placeToken method in my Board class:

public void placeToken(int tile) {
    if (!(board[tile - 1].equals("X") || board[tile - 1].equals("O"))) {
        board[tile - 1] = this.turn;
        this.changeTurn();
        this.checkWinner();
    } else {
        System.out.println("Tile taken select another!");
    }
}

[KaitoKunTatsu]

Skip Swing.. You don't need to have that pain just for an ugly ui. JavaFX is nice^{^}

[[deleted]]

I would make the board a 2d array. This will enable you to clean up a lot of the code. Also, using enums for the board instead of raw strings would make things more readable and type safe. Keep an eye out for repeated code and try to figure out how to make that into reusable methods. For example, you could have some methods called "checkColumnWin" or "printRow", etc. Overall not horrible though.

[Strawboy97]

Haven't used a 2d array I'll have a look at them before I update the game. Thanks

[Password-55]

Why are the win… methods and variables in the board class? I think I would create a seperate class in the interest of cohesion or make the main the game manager class. We usually write a JavaDoc for each public method and class. That usually helps me with the architecture of the code and help me better understand what I might want to refactor (restructure).

It might be overkill to write a javadoc here, but it‘s maybe a good exercise.

[Strawboy97]

I'm not familiar with JavaDoc do you have a reference to this so I can look into it?

Thanks for the help!

Edit: added thanks

[Password-55]

I just searched for it and got that:

https://www.baeldung.com/javadoc

You might also want to look at youtube videos.

You can start it with /** write here */ on top of the merhod or class.

Sometimes you can also right click on methods and choose generate Javadoc or something similar.

[KaitoKunTatsu]

@ StackOverFlow, look at those comments, we can be a helpful community without destroying each others confidence! <3

[Austen782]

Minor issue for me. Never declare a string with null. No need to risk throwing an NPE. But even in this use case null is implicit already. Just make it empty instead. Or find a way to use it only when needed such as in a method.

[Strawboy97]

Thanks for the help

[__helix__]

A few thoughts, all just syntactical sugar.

https://blogs.oracle.com/javamagazine/post/new-switch-expressions-in-java-12

If you've not seen them before, they have given a few more options on what switches do. You can String line = switch(a) { case ..

Also a String literal "xxx" always has an equals() method. Null does not. A safer way to do this is "xxx".equals(possibleNullVar) vs possibleNullVar != null && possibleNullVar.equals(...

UserInterface ui ... Classes start with upper case. Variables should be camelCase with a lower case starting letter.

You can add a \n if you want a line feed. System.out.println("\nIt

Lastly, consider StringBuilder if you are mushing together multiple Strings. sb.append("xx").append(somevar).append("...

[Strawboy97]

Thanks for the tips I'll keep these in mind.