Pacman game implementation in Java

Question

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, 0, 0,  0, 0, 0, 0, 0 }, /*  5 */
            { 0,-4, 0, 0, 0,  4, 0, 4, 0, 0,  0,-4, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0, 0, 0 }, /*  6 */
            { 0, 0,-4, 0, 0,  0, 4, 0, 4, 0,  0, 0,-4, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0, 0, 0 }, /*  7 */
            { 0, 0, 0,-4, 0,  0, 0, 4, 0, 4,  0, 0, 0,-4, 0,  0, 0, 0, 0, 0,  0, 0, 0, 0, 0 }, /*  8 */
            { 0, 0, 0, 0,-4,  0, 0, 0, 4, 0,  0, 0, 0, 0,-4,  0, 0, 0, 0, 0,  0, 0, 0, 0, 0 }, /*  9 */

            { 0, 0, 0, 0, 0, -4, 0, 0, 0, 0,  0, 4, 0, 0, 0, -4, 0, 0, 0, 0,  0, 0, 0, 0, 0 }, /* 10 */
            { 0, 0, 0, 0, 0,  0,-4, 0, 0, 0,  4, 0, 4, 0, 0,  0,-4, 0, 0, 0,  0, 0, 0, 0, 0 }, /* 11 */
            { 0, 0, 0, 0, 0,  0, 0,-4, 0, 0,  0, 4, 0, 4, 0,  0, 0,-4, 0, 0,  0, 0, 0, 0, 0 }, /* 12 */
            { 0, 0, 0, 0, 0,  0, 0, 0,-4, 0,  0, 0, 4, 0, 4,  0, 0, 0,-4, 0,  0, 0, 0, 0, 0 }, /* 13 */
            { 0, 0, 0, 0, 0,  0, 0, 0, 0,-4,  0, 0, 0, 4, 0,  0, 0, 0, 0,-4,  0, 0, 0, 0, 0 }, /* 14 */

            { 0, 0, 0, 0, 0,  0, 0, 0, 0, 0, -4, 0, 0, 0, 0,  0, 4, 0, 0, 0, -4, 0, 0, 0, 0 }, /* 15 */
            { 0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0,-4, 0, 0, 0,  4, 0, 4, 0, 0,  0,-4, 0, 0, 0 }, /* 16 */
            { 0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0,-4, 0, 0,  0, 4, 0, 4, 0,  0, 0,-4, 0, 0 }, /* 17 */
            { 0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0,-4, 0,  0, 0, 4, 0, 4,  0, 0, 0,-4, 0 }, /* 18 */
            { 0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0, 0,-4,  0, 0, 0, 4, 0,  0, 0, 0, 0,-4 }, /* 19 */

            { 0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0, 0, 0, -4, 0, 0, 0, 0,  0, 4, 0, 0, 0 }, /* 20 */
            { 0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0,-4, 0, 0, 0,  4, 0, 4, 0, 0 }, /* 21 */
            { 0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0,-4, 0, 0,  0, 4, 0, 4, 0 }, /* 22 */
            { 0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0,-4, 0,  0, 0, 4, 0, 4 }, /* 23 */
            { 0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0, 0, 0,  0, 0, 0, 0,-4,  0, 0, 0, 4, 0 }  /* 24 */
            //    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 
    };

    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;

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

    public void displayGraph() {    //this method will just print out the vertices of the graph if I don't call RenderGraph, but this method so far isn't called, because renderGraph is used instead. 
        for (int i=0; i<25; ++i) {
            for (int j=0; j<25; ++j) {
                if (graph[i][j] > 0) System.out.print("+");
                else if (graph[i][j] == 0) System.out.print(" ");
                System.out.print( ""+graph[i][j] );
                if (j < 24) System.out.print(" ");
            }
            System.out.println();
        }
    }

    public void renderGraph() {
        // this is really the output image object
        char[][] renderPlane = new char[17][33]; //17 tall by 33 wide

        // clear image
        for (int i=0; i<17; ++i) {
            for (int j=0; j<33; ++j) {
                renderPlane[i][j] = ' ';
            }
        }

        // draw the bare graph 
        //   - this should be a separate utility method
        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] = '-';
        }
        /*
       0       8       16      24      32
     0 ---------------------------------



     4 ---------------------------------



     8 ---------------------------------



    12 ---------------------------------



    16 ---------------------------------
         */

        for (int j : cols) {
            for (int i=0; i<17; ++i) renderPlane[i][j] = '|';
        } 
        /*
       0       8       16      24      32
     0 |-------|-------|-------|-------| 
       |       |       |       |       |
       |       |       |       |       |
       |       |       |       |       |
     4 |-------|-------|-------|-------|
       |       |       |       |       |
       |       |       |       |       |
       |       |       |       |       |
     8 |-------|-------|-------|-------|
       |       |       |       |       |
       |       |       |       |       |
       |       |       |       |       |
    12 |-------|-------|-------|-------|
       |       |       |       |       |
       |       |       |       |       |
       |       |       |       |       |
    16 |-------|-------|-------|-------|
         */

        for (int i : rows) {
            for (int j : cols) renderPlane[i][j] = '+';
        }
        /*
       0       8       16      24      32
     0 +-------+-------+-------+-------+ 
       |       |       |       |       |
       |       |       |       |       |
       |       |       |       |       |
     4 +-------+-------+-------+-------+
       |       |       |       |       |
       |       |       |       |       |
       |       |       |       |       |
     8 +-------+-------+-------+-------+
       |       |       |       |       |
       |       |       |       |       |
       |       |       |       |       |
    12 +-------+-------+-------+-------+
       |       |       |       |       |
       |       |       |       |       |
       |       |       |       |       |
    16 +-------+-------+-------+-------+
         */

        // draw PacMan (and ghosts, and energy blobs)
        int from  = pacManState[FROM]; //12
        int to    = pacManState[TO];   //13
        int steps = pacManState[STEPS];  //0

        // translate from abstract graph position to physical render                plane position
        /* 
          0       1       2       3       4
          0       8       16      24      32
      0 0 0-------1-------2-------3-------4 
          |       |       |       |       |
          |       |       |       |       |
          |       |       |       |       |
      1 4 5-------6-------7-------8-------9
          |       |       |       |       |
          |       |       |       |       |
          |       |       |       |       |
      2 8 10------11------12------13------14
          |       |       |       |       |
          |       |       |       |       |
          |       |       |       |       |
     3 12 15------16------17------18------19
          |       |       |       |       |
          |       |       |       |       |
          |       |       |       |       |
     4 16 20------21------22------23------24
         */
        //changes the vertice's number to a position which can be       recognized by the renderGraph() method. 

        int fromRow = (from / 5);  //= 2
        int fromCol = (from % 5); // = 2
        int toRow   = (  to / 5); // = 2
        int toCol   = (  to % 5); // = 3

        /* Example: 
             from(12)-->to(13),
             fromRow = 2, fromCol = 2
             toRow   = 2, toCol   = 3

             from(16)-->to(11),
             fromRow = 3, fromCol = 1
             toRow   = 2, toCol   = 1
         */

        if (fromRow == toRow) {
            if (fromCol < toCol) {            // move right
                renderPlane[4*fromRow][8*fromCol + steps] = 'Q';
            }
            else if (fromCol > toCol) {    // move left
                renderPlane[4*fromRow][8*fromCol - steps] = 'Q';
            }
            else {                                            // no move
                renderPlane[4*fromRow][8*fromCol] = 'Q';
            }
        }
        else if (fromCol == toCol) {
            if (fromRow < toRow) {            // move down
                renderPlane[4*fromRow + steps][8*fromCol] = 'Q';
            }
            else if (fromRow > toRow) {    // move up
                renderPlane[4*fromRow - steps][8*fromCol] = 'Q';
            }
            else {                                            // no move
            }
        }
        else {
            System.out.println( "Illegal move." );
            return;
        }

        // paint image on screen (this is really image.repaint() )
        for (int i=0; i<17; ++i) {
            for (int j=0; j<33; ++j) {
                System.out.print( renderPlane[i][j] );
            }
            System.out.println();
        }
        System.out.println();

    }

    public void run() {
        int n = 0;
        while (n++ < 1000) {

            int from    = pacManState[FROM];
            int to      = pacManState[TO];
            int steps   = pacManState[STEPS];


            System.out.println("(from,to,steps) = ("+from+","+to+","+steps+")");
            System.out.println();



            renderGraph();



            try { Thread.sleep(100); }
            catch (InterruptedException e) { }

            // update PacMan state with random motion
            int edgeLen = Math.abs(graph[from][to]); //gets you the length of the edge between the two vertices of where Pacman is and wants to go. 

            if (steps < edgeLen) {
                ++pacManState[STEPS];    //if Pacman hasn't gotten to the other side, his steps go up by one. 
            }

            else if (steps == edgeLen) {
                int randomizer = 0;
                for (int j=0; j<25; j=j+1) {
                    if (!(0 == graph[to][j]))  {
                        randomizer=randomizer+1;  
                    }
                }

                int choice = (int)(randomizer * Math.random());//sets any vertex in the same row or column to the from vertex. 

                for (int j=0; j<25; j=j+1) {   
                    if (!(0 == graph[to][j])) {   
                        if (0 == (choice)) {
                            from  = to;
                            to    = j;
                            steps = 0;
                        }
                        choice=choice-1;
                    }
                }

                pacManState[FROM]  = from;
                pacManState[TO]    = to;
                pacManState[STEPS] = steps;
            }
        }  
    }
}

Answer 1

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<25; ++i) { // what is 25? the graph width? 
        for (int j=0; j<25; ++j) { // or is it the height?
            if (graph[i][j] > 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 < graph.length; ++i) {
        for (int j = 0; j < graph[i].length; ++j) {
            if (graph[i][j] > 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<25; ++i) { is needlessly hard on the eyes.

• 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 applies to for and while statements!
       renderPlane[i][j] = '-';
    }
}

You can apply similar logic to the rest of the method.

I won't go onto the run method, as you can refactor that yourself using some of the comments above. Overall, this is good code. I'm looking forward to when you post your Swing version!

Answer 2

This was a fun project to review. In the end, I decided that there were enough problems with the design to justify starting afresh.

The main problems I saw were…

Drawing the grid

The renderGraph() method would be better named renderGrid().

The design is rigid. You have magic numbers everywhere within renderGraph(): 17, 33, 0, 4, 8, 12, 16, 0, 8, 16, 24, 32, 4, 8, 17, 33. If you ever want to extend the grid or expand the spacing, you would have to make changes in many places.

Your graphical comments helped tremendously. Thank you for writing them.

Considering the rigidity of the design and the amount of code to achieve the effect, you would have been better off just hard-coding the entire result directly:

private static final String[] GRID = new String[] {
    "+-------+-------+-------+-------+",
    "|       |       |       |       |",
    "|       |       |       |       |",
    "|       |       |       |       |",
    "+-------+-------+-------+-------+",
    "|       |       |       |       |",
    "|       |       |       |       |",
    "|       |       |       |       |",
    "+-------+-------+-------+-------+",
    "|       |       |       |       |",
    "|       |       |       |       |",
    "|       |       |       |       |",
    "+-------+-------+-------+-------+",
    "|       |       |       |       |",
    "|       |       |       |       |",
    "|       |       |       |       |",
    "+-------+-------+-------+-------+"
};

Calling System.out.print() to print a character at a time is inefficient. That results in a write() kernel call each time. Compose the string you want to print, and print it all at once.

The node adjacency table

I had a fun time puzzling out the int[][] graph enigma. You've numbered the intersection nodes from 0 to 24, in row-major order. The graph matrix indicates the number and direction of the steps to move between a pair of nodes, or 0 if two nodes are not adjacent.

It was probably a lot of work for you to construct the matrix. It would be a lot of work again if you ever needed to expand the grid.

Furthermore, it's a sparse matrix. When you use it to pick the next destination node, you have at best a \$\frac{4}{25}\$ chance of picking a valid adjacent node on each attempt (\$\frac{3}{25}\$ if on an edge, and only \$\frac{2}{25}\$ if at a corner). You would easily expend a dozen random numbers just to get out of a corner.

Miscellaneous observations

• displayGraph() and run() are both long functions. You could easily break out subroutines, such as the rendering of the 'Q' and the selection of the next destination.

• In the end, you'll probably want more than one character on the screen. You've attempted to encapsulate the character's movement within a pacManState array, which is a good start, but you really ought to have a proper object. Making a separate class would also help to organize your code.

Recommendations

• Draw the grid by tesselating small squares.
• Get rid of the node-numbering scheme.
• Create a PacmanCharacter class to represent the animated objects.
• Select a destination by picking one of the four cardinal directions, and checking that the destination is in bounds.

Suggested Implementation

PacmanDraft

public class PacmanDraft {
    public static final int ROWS = 4, COLS = 4;

    private static final String[] SQUARE = {
        "+-------",
        "|       ",
        "|       ",
        "|       "
    };

    // SQUARE is 8 chars wide vs. 4 rows high, so ASPECT_RATIO = 2
    public static final int ASPECT_RATIO = 2;

    public static final int SQUARE_HEIGHT = SQUARE.length,
                            SQUARE_WIDTH  = SQUARE[0].length();

    public static final int STEPS = SQUARE_HEIGHT;

    private char[][] grid;

    private PacmanCharacter[] characters;

    public PacmanDraft() {
        this.characters = new PacmanCharacter[] {
            new PacmanCharacter('Q', ROWS / 2, COLS / 2)
        };
    }

    private static char[][] makeGrid() {
        // +1 so that there is a bottom edge
        char[][] grid = new char[ROWS * SQUARE_HEIGHT + 1][];
        for (int r = 0; r < grid.length; r++) {
            // +1 so that that there is a right edge
            grid[r] = new char[COLS * SQUARE_WIDTH + 1];
            for (int c = 0; c < grid[0].length; c++) {
                grid[r][c] = SQUARE[r % SQUARE_HEIGHT].charAt(c % SQUARE_WIDTH);
            }
        }
        return grid;
    }

    private void placeCharacters() {
        int[] coords = new int[2];
        for (PacmanCharacter c : this.characters) {
            c.getScreenCoords(coords);
            this.grid[coords[0]][coords[1]] = c.getSymbol();
            c.step();
        }
    }

    public String toString() {
        StringBuilder sb = new StringBuilder(this.grid.length * (this.grid[0].length + 1));
        for (char[] row : this.grid) {
            sb.append(row).append('\n');
        }
        return sb.toString();
    }

    public void run(int steps) throws InterruptedException {
        for (int i = steps; i > 0; i--) {
            this.grid = makeGrid();
            this.placeCharacters();
            System.out.println(this);

            Thread.sleep(100);
        }
    }

    public static void main(String[] args) throws InterruptedException {
        new PacmanDraft().run(1000);
    }
}

PacmanCharacter

import java.util.Random;

public class PacmanCharacter {
    private final char symbol;

    private int srcRow, srcCol,
                dstRow, dstCol;
    private int steps;

    private Random random;

    public PacmanCharacter(char symbol, int initRow, int initCol) {
        this.symbol = symbol;
        this.srcRow = this.dstRow = initRow;
        this.srcCol = this.dstCol = initCol;
        this.steps = PacmanDraft.STEPS;
        this.random = new Random();
    }

    public char getSymbol() {
        return this.symbol;
    }

    public void getScreenCoords(int[] coords) {
        int row = coords[0] = PacmanDraft.SQUARE_HEIGHT * this.srcRow +
                              (this.dstRow - this.srcRow) * this.steps;
        int col = coords[1] = PacmanDraft.SQUARE_WIDTH * this.srcCol +
                              PacmanDraft.ASPECT_RATIO * (this.dstCol - this.srcCol) * this.steps;
        System.out.printf("(%d, %d) -> (%d, %d) step %d = [%d, %d]\n",
                          this.srcRow, this.srcCol, this.dstRow, this.dstCol,
                          this.steps,
                          row, col);
    }

    public void step() {
        if (this.steps >= PacmanDraft.STEPS) {
            this.nextDestination();
        }
        this.steps++;
    }

    private void nextDestination() {
        this.steps = 0;
        this.srcRow = this.dstRow;
        this.srcCol = this.dstCol;
        do {
            switch (this.random.nextInt(4)) {
              case 0:
                this.dstRow = this.srcRow - 1;
                this.dstCol = this.srcCol;
                break;
              case 1:
                this.dstRow = this.srcRow;
                this.dstCol = this.srcCol + 1;
                break;
              case 2:
                this.dstRow = this.srcRow + 1;
                this.dstCol = this.srcCol;
                break;
              case 3:
                this.dstRow = this.srcRow;
                this.dstCol = this.srcCol - 1;
                break;
            }
        } while (this.dstRow < 0 || this.dstRow > PacmanDraft.ROWS ||
                 this.dstCol < 0 || this.dstCol > PacmanDraft.COLS);
    }
}