Skip to main content
Code Review

Return to Answer

added 1723 characters in body
Source Link
Maarten Bodewes
Maarten Bodewes
  • 5.8k
  • 18
  • 52

GUI code

There was some time required to get the GUI running. However, when I did the game ran, but there were some issues. For instance, you can scale the window containing puzzle, but that just mucks up the contents. The puzzle is not scaled well initially either, and a line seems to be missing on the right hand side.

I don't expect to interact with a Viewer. Maybe with a View, but a Viewer sounds rather passive.

There are dual calls to drawGrid that aren't explained at all. I don't see why those are necessary.

I presume the squares are 100 pixels in size or so, but there is a lot of repetition of the value 100. The value 100 should probably be a constant, but generally I'd already use a squareEdgeSize variable for that. At least you did make sure that the value is not repeated in the canvas code, so that's OK.

You are using a MouseListener with unimplemented methods. However, in that case you might want to have a look at a MouseAdapter instead that provides default implementations, cleaning up your listener interface.

When you look at the drawRectangle you can see that you specifically have to calculate x + 100, while the original method of drawing in Java uses a width. So you are calculating a second x using the width only to reverse that later.

Your methods are not symmetric. You have a drawGrid() that doesn't take any arguments, and many other draw methods that do. That kind of inconsistencies are also seen in the whitespace and such. It makes the code less clean and harder to read.

Both the constructor of the viewer and another method draw the grid, and make the same decision which one to draw. This goes against DRY principles.

GUI code

There was some time required to get the GUI running. However, when I did the game ran, but there were some issues. For instance, you can scale the window containing puzzle, but that just mucks up the contents. The puzzle is not scaled well initially either, and a line seems to be missing on the right hand side.

I don't expect to interact with a Viewer. Maybe with a View, but a Viewer sounds rather passive.

There are dual calls to drawGrid that aren't explained at all. I don't see why those are necessary.

I presume the squares are 100 pixels in size or so, but there is a lot of repetition of the value 100. The value 100 should probably be a constant, but generally I'd already use a squareEdgeSize variable for that. At least you did make sure that the value is not repeated in the canvas code, so that's OK.

You are using a MouseListener with unimplemented methods. However, in that case you might want to have a look at a MouseAdapter instead that provides default implementations, cleaning up your listener interface.

When you look at the drawRectangle you can see that you specifically have to calculate x + 100, while the original method of drawing in Java uses a width. So you are calculating a second x using the width only to reverse that later.

Your methods are not symmetric. You have a drawGrid() that doesn't take any arguments, and many other draw methods that do. That kind of inconsistencies are also seen in the whitespace and such. It makes the code less clean and harder to read.

Both the constructor of the viewer and another method draw the grid, and make the same decision which one to draw. This goes against DRY principles.

added 575 characters in body
Source Link
Maarten Bodewes
Maarten Bodewes
  • 5.8k
  • 18
  • 52

Design

Design: I'd create a separate class for Grid and Position at the very least. Especially when separating out the Grid the game code will be significantly reduced. Smaller classes should generally be preferred when they make sense.

With the removal of movecount (see below), you'd have just two fields: currentGrid and goalGrid, both of type Grid.

Code review

I'll handle the lines in order, skipping many, with the remarks below the code.

private int movecount = 1000;

The field movecount (or rather moveCount) should be a constant; it doesn't change within the game logic.

I'm a bit worried about the naming though, what about, say, edgeSize or dimension?

Never ever return a mutable reference to something that makes up the internal state of a class, violating encapsulation principles. Copy or clone the data instead, or provide read only access (iterator, lambda).

    return ( (x == xspace && (y == yspace - 1 || y == yspace + 1)) || (y == yspace && (x == xspace - 1 || x == xspace + 1)) );

Just returning or(or worse, just returning with null or 0 for methods that return data) is almost always a bad decision. In this case an exception should be thrown. 

If the calling code first wants to check if the move is valid, it can call legalClick itself. You may be calling legalClick twice that way, but the JIT compiler will probably notice if it starts to take too much time.

Design: I'd create a separate class for Grid and Position at the very least.

I'm a bit worried about the naming though, what about, say, edgeSize or dimension?

Never ever return a mutable reference to something that makes up the internal state. Copy or clone the data instead, or provide read only access (iterator, lambda).

    return ( (x == xspace && (y == yspace - 1 || y == yspace + 1)) || (y == yspace && (x == xspace - 1 || x == xspace + 1)) );

Just returning or worse, just returning with null is almost always a bad decision. In this case an exception should be thrown. If the calling code first wants to check if the move is valid, it can call legalClick itself. You may be calling legalClick twice that way, but the JIT compiler will probably notice if it starts to take too much time.

Design

I'd create a separate class for Grid and Position at the very least. Especially when separating out the Grid the game code will be significantly reduced. Smaller classes should generally be preferred when they make sense.

With the removal of movecount (see below), you'd have just two fields: currentGrid and goalGrid, both of type Grid.

Code review

I'll handle the lines in order, skipping many, with the remarks below the code.

private int movecount = 1000;

The field movecount (or rather moveCount) should be a constant; it doesn't change within the game logic.

I'm a bit worried about the naming though, what about, say, edgeSize or dimension?

Never ever return a mutable reference to something that makes up the internal state of a class, violating encapsulation principles. Copy or clone the data instead, or provide read only access (iterator, lambda).

return ( (x == xspace && (y == yspace - 1 || y == yspace + 1)) || (y == yspace && (x == xspace - 1 || x == xspace + 1)) );

Just returning (or worse, just returning null or 0 for methods that return data) is almost always a bad decision. In this case an exception should be thrown. 

If the calling code first wants to check if the move is valid, it can call legalClick itself. You may be calling legalClick twice that way, but the JIT compiler will probably notice if it starts to take too much time.

Source Link
Maarten Bodewes
Maarten Bodewes
  • 5.8k
  • 18
  • 52

Design: I'd create a separate class for Grid and Position at the very least.

import java.util.Arrays;
public class FifteenPuzzle
{

Please use at least a white line between import statements and the class declaration.

The class should probably be made final unless you create it for extension.

Using { on a new line instead of "Egyptian braces" is often acceptable, but it is not very much used for Java. If they are used, then use them consistently (which is not the case here). Most IDE's contain code formatting nowadays, use that if you don't like doing it yourself.

private int xspace;   // xspace,yspace are the current coordinates of the space
private int yspace;

What space? The empty space? Please be precise. If two variables are needed for one thing, try and aggregate them together, e.g. into a Position class (immutable with equals and hashCode please).

private final int size; // the number of tiles across and down

Not required, grid.length will do that. Generally, try to minimize the amount of fields. However, for readability reasons I guess this is kinda OK.

I'm a bit worried about the naming though, what about, say, edgeSize or dimension?

grid = initialGrid;

Why isn't the size of the grid checked here? It is not symmetric with the constructor that just takes the grid size. Furthermore, there is no check that the width and height are identical.

public void scramble() {

Hey, is the user allowed to call scramble? If not, then why is the method public instead of private? Same for other methods.

done = true;

Use while (true) or for(;;) and break / continue instead. May look weird initially, but it removes one variable from the equation, and you can remove redundant checks.

for (int a = 1; a <= size; a++) {

No, no, no. Always use zero based indexing:

for (int a = 0; a < size; a++) {

unless there are specific reasons not to. It seems OK for the move counter (as you're doing move 1 initially, not move 0 I suppose).

And what about that naming, why a and b for coordinates? x and y is used correctly in the rest of the code.

public int[][] setGoal() {

A "setter" is a specific function that sets a field to a given value. It takes a single argument and returns void. createGoal is probably a better name.

public int[][] getGrid()

This is a correct "getter" declaration. No arguments, single return of a field.

return grid;

Never ever return a mutable reference to something that makes up the internal state. Copy or clone the data instead, or provide read only access (iterator, lambda).

if (x < 0 || y < 0 || x > size-1 || y > size-1) {

This I can read somewhat.

    return ( (x == xspace && (y == yspace - 1 || y == yspace + 1)) || (y == yspace && (x == xspace - 1 || x == xspace + 1)) );

This I cannot. The line size is too large as well. Compare to reading this:

return insideGrid(x, y) && nextToEmpty(x, y);

and then implementing those functions. With a bit of help of the IDE that's not even that much work.

if (!legalClick(x, y)) {
    return;
}

Just returning or worse, just returning with null is almost always a bad decision. In this case an exception should be thrown. If the calling code first wants to check if the move is valid, it can call legalClick itself. You may be calling legalClick twice that way, but the JIT compiler will probably notice if it starts to take too much time.