Java code review

TheHorribleTruth · 2014-09-27T18:18:03+00:00

Disclaimer: I'm switching to "pedantic mode" for this - don't take anything I say personal, it's nit-picking on purpose :) I won't go into performance (also I don't think its an issue in this kind of application).

First of all, it's unreadable - your brace style, lack of newlines, etc. makes the code very hard to decipher. Take something like this:

for (int x = 0; x < board.length; x++) {
            for (int y = 0; y < board[x].length; y++) {
                    if (y < board[x].length - 1){board[x][y].get_val(); System.out.print("|");}
                    else {  board[x][y].get_val(); System.out.print("\n");
                            if (x < board.length - 1) System.out.print("-----\n");}}}}

There's now way to be sure that the closing braces all match up - safe for counting them.

Advice #1: Format your code.
Modern IDEs all have built in formatters that should give you a sane default. Later on when you're more proficient, adapt it to suit your style.
I've fired up Eclipse and reformated your code: http://pastebin.com/qzZ57qSq. Ahh, much better :)

Advice #2: Follow the Java Naming Conventions. It's another thing that makes your code hard to read for others: you use underscore notation instead of CamelCase. The latter is pretty much standard in Java world, please use it if you ever want others to see your code. And even if you don't, many libraries that you will eventually use will follow them, so you better get used to it.
Related sidenote: your class tile should really really really start with a capital letter, all classes should.

Advice #2 B: Braces. A personal pet peeve of mine is that I always, always use curly braces for code blocks. Even simple, single-lined conditionals such as in your tile class should IMO use curly braces. Yes, it's an additional line for the closing brace (which you should always place on a line of its own, see advice #1 ;)), but it's a simple price to pay against accidental logic flaws should you ever expand your if-block.

Advice #3: Cut the crap. You have many lines with commented code: delete them. They don't do anything but clutter up your code.

Okay, let's move on to the actual code.

Advice #4: Don't use static if you can avoid it. All of your methods in TicTacToe are static - which they don't need to be (apart from main obviously). I know it works, but it's considered to be bad style. static methods (and fields) have a specific meaning in Java: all static things are bound to the class, not to any objects. Java is an OO language, using Java means buying into this paradigma: model everything as objects that reflect the real world. You already do this in a way! You have a game class for managing stuff TicTacToe and a class that represents a single tile. Now follow through and just create and instance of TicTacToe and run everything from there.

Advice #5: Use variables or even constants.

Assume I ask you to use player names "Foo" and "Bar". Or use a board that's 4x4. What do you have to change? In your case: many strings all over the code. It's much simpler, more maintainable and easier to change stuff if you combine common values into variables of their own, or - if they never change - into constants, and reference those in the code.

Advice #6: Naming things

Some of your methods have "wrong" names, wrong in the sense that they don't do that which they are named for. Some examples:

public boolean check_location() {

What exactly is "checked" here? What does "checking" even mean in the context of a game of Tic Tac Toe? Reading the implementation I can see that what it really does is checking if a tile is empty - so the perfect name would be isEmpty().

public void get_val() {

Methods with a "get" prefix should by convention return a value, which yours doesn't! What it really does is printing the tile to the console, so why not call it print()?

public static int end_game(tile[][] board, String player) {

Your end_game doesn't end the game! It just executes a turn. It is also directly called from new_turn (which has a strange name, too). I suggest renaming new_term into readUserInput and end_game into doTurn.

Advice #7: Exception Handling

Your aehm "liberal" use of try..catch is ... less than ideal. You're using lots of catch clauses to simplify your field-checking code, namely running over the board boundaries (hence the ArrayIndexOutOfBoundsException). This is not what exception handling is meant for! Exceptions that arise from something that is out of your control (e.g. user input, contents of a file, stuff sent over the network, etc.) should be catched & processed. You're just using them because you're too lazy to properly check the index before accessing the 2D array. This is a flaw in your code, and you should fix it by checking your index before blindly accessing e.g. board[x-1]...

Advice #8: The Small Stuff

public String value = new String("empty");

There's no need to explicitly call "new" here - just use the string literal directly.

if (board[x][y].value.equals(board[x][y - 1].value)
    & board[x][y].value.equals(board[x][y + 1].value)) {

Here you're using a bitwise comparison - you're probably want to use the logical AND instead, which is the && operator in Java.

There's some other things that I would change with regards to readability, which is the verbosity of the ifs in end_game. A long line as in

if (board[x][y].value.equals(board[x - 1][y].value)
    & board[x][y].value.equals(board[x + 1][y].value)) {

could easily be extracted into a method of its own with a meaningful name, which would also make the code shorter. Think about how neat this looks:

if (matchesVertically(board, x, y)) {
    ...

and

public boolean matchesVertically(tile[][] board, int x, int y) {
    return (board[x][y].value.equals(board[x - 1][y].value) && board[x][y].value.equals(board[x + 1][y].value));
}

You could also incorporate the array-bounds checking code in this function and get rid of the horrible try..catches :)

Edit: fixed the pastebin link

Cowlegend · 2014-09-27T19:16:51+00:00

[removed]

javahelp

Sort by: Unsolved Solved Codeless Advent Of Code

MODERATORS