Pacman game implementation in Java

I assume you've all played Pacman, I mean, most people have.

I am a 10th grader, and I am working on building Pacman for my intro to Java class in school.

However, the project I'm working on demands that someone review my code.

About the code: I have not started the graphics yet. I have just done a little work using a two-dimenstional array to store the state of the Pacman, and at the bottom there is a method which randomizes the motion of the Pacman.

The large array in the beginning represents a 5 x 5 grid with vertices in which the Pacman can move through. If you copy the code into a compiler, you can see what I mean. When Pacman is at a vertex , he will make a decision and can thus move to another one. If the value is negative, then there is a vertical distance in which the Pacman can move through, but if the value is positive, then this distance is horizontal.

Please give me any suggestions you have on what my next steps should be and how I should clean up the code. The more specific, the better. But basically, anything you say will be fine.

```
public class PacmanRoughDraft {

public static void main(String[] args) {

PacmanRoughDraft pacMan = new PacmanRoughDraft();
pacMan.run();
}
//To is on the y-axis, from is on the x-axis.
int[][] graph = {
// 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24
{ 0, 4, 0, 0, 0, -4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, / 0 /
{ 4, 0, 4, 0, 0, 0,-4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, / 1 /
{ 0, 4, 0, 4, 0, 0, 0,-4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, / 2 /
{ 0, 0, 4, 0, 4, 0, 0, 0,-4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, / 3 /
{ 0, 0, 0, 4, 0, 0, 0, 0, 0,-4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, / 4 /

{-4, 0, 0, 0, 0, 0, 4, 0, 0, 0, -4, 0, 0, 0, 0, 0, 0, 0

This is a great code review opportunity because your program 1) works, and 2) has tons of room for improvement. Almost every line of your code could be refactored, but don't take that negatively. Every day I come out of code reviews with changes suggested to my awesome code. It's just how the business works and we should always enjoy the opportunity to learn and write even better code in the future.

With that said, in your main method, you can accomplish the same with the following:

public static void main(String[] args) {
    new PacmanRoughDraft().run();
}

You define the graph in your code, which is okay for early development and quick debugging, but you should really load it from a file. Google will lead you to some Stack Overflow answers on how to do this, and I highly reccommend it. It's quite easy, and it's never to early to have some experience with file IO.

But was does -4 in your graph mean? As you'll see, it's a bad idea to use these Magic Numbers.

int[] pacManState = new int[3];   // [0] = from
// [1] = to
// [2] = steps
static final int FROM  = 0;
static final int TO    = 1;
static final int STEPS = 2;

I'm curious why you are using an array to hold the state here. This could simply be:

private int from;
private int to;
private int steps;

That way you don't have to worry about the problems that can come along with arrays. Notice also that I declared them private. Without a modifier, they have default access, which means any other class in the same package as it can modify it's values. Rarely a good idea.

public PacmanRoughDraft() {
    pacManState[FROM]  = 12;
    pacManState[TO]    = 13;
    pacManState[STEPS] =  0;
}

12 and 13 here are magic numbers. You should replace them with constants declared near the top of the class so you can easily modify. And with the previous suggestion, this could be rewritten as

// near the top of the class
private static final int INITIAL_FROM = 12;
private static final int INITIAL_TO = 13;
private static final int INITIAL_STEPS = 0;

// later...
public PacmanRoughDraft() {
    from  = INITIAL_FROM;
    to    = INITIAL_TO;
    steps = INITIAL_STEPS;
}

I'll keep bringing up magic numbers because they are evil. Or, if not evil, unnecessary, as the following code shows:

public void displayGraph() {  
    for (int i=0; i 0) System.out.print("+"); // wait, now I know what -4 means... I think
            else if (graph[i][j] == 0) System.out.print(" ");
            System.out.print( ""+graph[i][j] );
            if (j < 24) System.out.print(" ");
        }
        System.out.println();
    }
}

This can be rewritten like so:

public void displayGraph() { 
    for (int i = 0; i  0) {
              System.out.print("+");
            }
            else if (graph[i][j] == 0) {
              System.out.print(" ");
            }

            System.out.print(graph[i][j]);

            if (j < graph[i].length - 1) {
              System.out.print(" ");
            }
        }
        System.out.println();
    }
}

There's a lot going on here that I'll point one:

-
Whitespace between things. for (int i=0; i

-
Getting length of the array means you can change the size of your grid without having to change everything. Seriously, anywhere you have 25 in your code should be replaced with grid.length or grid[i].length.

-
Avoid (read, never) use an if-statement without curly brackets. It'll only cause bugs when you come back and change things. For example, what if I intended for System.out.print( ""+graph[i][j] ); to only be called when graph[i][j] == 0.

-
Speaking of, the traditional way to output an array is to use Arrays.toString(graph[i][j]), not ""+graph[i][j].

-
This got me thinking, why is graph an int array anyways? Can you not make it a char array and just print that out, instead of doing all these calculations?

renderGraph()

I got confused here. What is renderPlane and how is it different from graph? That being said, you can do some neat improvements to this method.

char[][] renderPlane = new char[17][33];

Again, magic numbers. Replace with constants so you can change easily. And is there any relationship from the size of this array to the graph array?

int[] rows = new int[]{  0,  4,  8, 12, 16 };
    int[] cols = new int[]{  0,  8, 16, 24, 32 };

    for (int i : rows) {
        for (int j=0; j<33; ++j) renderPlane[i][j] = '-';
    }

I see what you're doing here, and it works but is not extensible. What if you want to change the size of you renderPlane? You can do away with both rows and cols, and add constants for ROW_SIZE and COL_SIZE. That way you can replace the above code with

``
private static final int ROW_SIZE = 4;
private static final int COL_SIZE = 4;

// later...
for (int i = 0; i < renderPlane.length; i += ROW_SIZE) {
for (int j = 0; j < renderPlane[i].length; ++j) {
// Same thing about curly braces

StackExchange Code Review Q#48728, answer score: 26