Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Here I've made a simple version of the game breakout for the Java course I'm taking. (This is a high school level course, I'm a freshman). Earlier this year we built Conway's Game of Life, and I used the same tools from that to build this game.

I've worked out most of the bugs, although the ball/block collision isn't working perfectly yet. Once you've pressed the start button, you can use keys 'A' and 'D' to control the slider. To launch the ball, hit 'O'.

Our teacher asks that we have an "expert external reviewer" take a look at our code and give us feedback. Anything along the lines of the efficiency of our code, the functionality of our game, etc... would be greatly appreciated!

Main:

import java.awt.GraphicsEnvironment;
import java.io.IOException;

import javax.swing.JFrame;


public class Main implements Runnable {

public JFrame frame;
final int DISPLAY_WIDTH = 1920;
final int DISPLAY_HEIGHT = 1000;

public Main(String[] args) throws IOException {

}

private void createAndShowGUI() {

    GraphicsEnvironment e = GraphicsEnvironment.getLocalGraphicsEnvironment();

    frame = new JFrame("Breakout");
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setMaximizedBounds(e.getMaximumWindowBounds());
   //frame.setExtendedState(frame.getExtendedState()|JFrame.MAXIMIZED_BOTH );
    frame.setSize(DISPLAY_WIDTH, DISPLAY_HEIGHT);

    Display display = new Display(DISPLAY_WIDTH, DISPLAY_HEIGHT);
    frame.setContentPane(display);

    // frame.pack();
    frame.setVisible(true);
}

public void run() {
    createAndShowGUI();
}

public static void main(String[] args) throws IOException {
   Main main = new Main(args);
    javax.swing.SwingUtilities.invokeLater(main);
}
}

Cell:

 import java.awt.Color;
 import java.awt.Graphics;
 import java.util.Random;

 public class Cell {
private int myX, myY; // x,y position on grid
private boolean myAlive; // alive (true) or dead (false)
private int myNeighbors; // count of neighbors with respect to x,y
private boolean myAliveNextTurn; // Used for state in next iteration
private boolean mySliderNextTurn; // Used for state in next iteration
private Color myColor; // Based on alive/dead rules
private final Color DEFAULT_ALIVE = Color.ORANGE;
private final Color DEFAULT_DEAD = Color.GRAY;
private boolean colorchoice = false;
private boolean side = false;
private boolean slider = false;
private boolean block = false;
private Color BLOCK_COLOR = Color.RED;
private boolean title = false;

public Cell(int x, int y) {
    this(x, y, false, Color.GRAY);
}

public Cell(int row, int col, boolean alive, Color color) {
    myAlive = alive;
    myColor = color;
    myX = col;
    myY = row;
}

public Cell(int row, int col, boolean alive, Color color, boolean slide, boolean s) {
    myAlive = alive;
    myColor = color;
    myX = col;
    myY = row;
    slider = slide;
    side = s;
}

public void setSideCell(boolean sides) {
    side = sides;
}

public boolean getSideCell() {
    return side;
}

public void setTitle(boolean bob) {
    if (bob) {
        title = true;
        setColor(Color.BLUE);
    } else {
        title = false;
        setColor(Color.GRAY);
    }
}


public void setSlider(boolean bob) {
    if (bob) {
        slider = true;
        setColor(Color.BLUE);
    } else {
        slider = false;
        setColor(Color.GRAY);
    }
}

public boolean getSlider() {
    return slider;
}

public boolean getXN(Cell[][] cell) {
    if (cell[myY][myX-1].getAlive() == true || cell[myY][myX+1].getAlive()) {
        return true;
    }
    return false;
}

public boolean getYN(Cell cell[][]) {
    if (cell[myY+1][myX].getAlive() == true || cell[myY-1][myX].getAlive()) {
        return true;
    }
    return false;
}

public boolean getAlive() {
    return myAlive;
}

public void setColorChoice(Boolean bool) {
    if (bool == true) {
        colorchoice = true;
    } else {
        colorchoice = false;
    }
}

public int getX() {
    return myX;
}

public int getY() {
    return myY;
}

public Color getColor() {
    return myColor;
}

public void setBlock(boolean bblock) {
    if (myY == 2) {
        BLOCK_COLOR = Color.ORANGE;
    }
    if (myY == 3) {
        BLOCK_COLOR = Color.YELLOW;
    }
    if (myY == 4) {
        BLOCK_COLOR = Color.GREEN;
    }
    if (myY == 5) {
        BLOCK_COLOR = Color.BLUE;
    }
    if (myY == 6) {
        BLOCK_COLOR = Color.MAGENTA;
    }
    if (bblock) {
        myColor = BLOCK_COLOR;
        block = true;
    } else {
        myColor = Color.GRAY;
        block = false;
    }
}

public boolean getBlock() {
    return block;
}

public void setAlive(boolean alive) {
    if (alive) {
        setAlive(true, myColor);
    }
}

public void setAlive(boolean alive, Color color) {
    if (alive == true && colorchoice == true) {
    Random rand = new Random();
    float r = rand.nextFloat();
    float g = rand.nextFloat();
    float b = rand.nextFloat();
    Color randomColor = new Color(r, g, b);
    myColor = randomColor;
    myAlive = alive;
    } else if (alive == true && colorchoice == false) {
        myColor = Color.yellow;
        myAlive = alive;
    } else {
        myColor = color;
        myAlive = alive;
    }
}

public void setAliveNextTurn(boolean alive) {
    myAliveNextTurn = alive;
}

public boolean getAliveNextTurn() {
    return myAliveNextTurn;
}

public void setSliderNextTurn(boolean alive) {
    mySliderNextTurn = alive;
}

public boolean getSliderNextTurn() {
    return mySliderNextTurn;
}

public void setColor(Color color) {
    myColor = color;
}

public int getNeighbors() {
    return myNeighbors;
}

//In this method, we used an array of x and y values that contained coordinates for the neighbors of a single cell. We then used a loop to check each cell and whether or not it was alive to count the neighbors.
public int calcNeighbors(Cell[][] cell) {
    int neighbors = 0;
    int[] xloc = {0, 1, 1, 1, 0, -1, -1, -1};
    int[] yloc = {-1, -1, 0, 1, 1, 1, 0, -1};
    for (int i = 0; i<8; i++) {
        try {
        if(cell[myY+xloc[i]][myX+yloc[i]].getAlive() == true) {
            neighbors++;
        }
        } 
        catch (ArrayIndexOutOfBoundsException e){
        neighbors+=0;   
        }

    }
    return neighbors;   
}

public void follow(Cell cell, Cell[][] carray) {
    if (cell.getY() > myY) {
        carray[myY][myX].setAlive(false);
        carray[myY+1][myX].setAliveNextTurn(true);
    }
    if (cell.getY() < myY) {
        carray[myY][myX].setAlive(false);
        carray[myY-1][myX].setAliveNextTurn(true);
    }
    if (cell.getX() > myX) {
        carray[myY][myX].setAlive(false);
        carray[myY][myX+1].setAliveNextTurn(true);
    }
    if (cell.getX() < myX) {
        carray[myY][myX].setAlive(false);
        carray[myY][myX-1].setAliveNextTurn(true);
    }
}

public void draw(int x_offset, int y_offset, int width, int height,
        Graphics g) {
    int xleft = x_offset + 1 + (myX * (width + 1));
    int xright = x_offset + width + (myX * (width + 1));
    int ytop = y_offset + 1 + (myY * (height + 1));
    int ybottom = y_offset + height + (myY * (height + 1));
    Color temp = g.getColor();

    g.setColor(myColor);
    g.fillRect(xleft, ytop, width, height);
}
 }

Display:

 import java.awt.Color;
 import java.awt.Graphics;
 import java.awt.Graphics2D;
 import java.awt.event.ActionEvent;
 import java.awt.event.ActionListener;
 import java.awt.event.KeyEvent;
 import java.awt.event.KeyListener;
 import java.awt.event.MouseEvent;
 import java.awt.event.MouseListener;
 import java.awt.event.MouseMotionListener;
 import java.awt.image.BufferedImage;
 import java.io.IOException;

 import javax.swing.JButton;
 import javax.swing.JComponent;
 import javax.swing.JLabel;

 // Note that the JComponent is set up to listen for mouse clicks
 // and mouse movement.  To achieve this, the MouseListener and
 // MousMotionListener interfaces are implemented and there is additional
 // code in init() to attach those interfaces to the JComponent.

public class Display extends JComponent implements MouseListener, MouseMotionListener {
public static int ROWS = 30;
public static int COLS = 40;
public static Cell[][] cell = new Cell[ROWS][COLS];
private final int X_GRID_OFFSET = 25; // 25 pixels from left
private final int Y_GRID_OFFSET = 40; // 40 pixels from top
private int CELL_WIDTH = 35;
private int CELL_HEIGHT = 20;
private final int DISPLAY_WIDTH;   
private final int DISPLAY_HEIGHT;
private boolean paintloop = true;
int TIME_BETWEEN_REPLOTS = 80;
int nextGen;
int sliderlength = 7;
int sliderLocation = ROWS-5;
public char lastKeyPressed = '/';
int balllength = 20;
int slideref = sliderlength/2 + ((COLS/2) - sliderlength);
double ballx = (CELL_WIDTH+1) * slideref + CELL_WIDTH+1;
double bally = 545;
double dirx = 3;
double diry = 3;
int ballcenterx = (int) ballx + (balllength/2);
int ballcentery = (int) bally + (balllength/2);
int[] returned = new int[2];
int[] ballcor = new int[2];
int lives = 3;
boolean ballMovement = false;
boolean tbm = true;
boolean draw = false;
int titleoffset = COLS;
private StartButton startStop;
private labels label;
int level = 1;
int blocksBroken = 0;
int imx = 10;
int imy = 10;

public Display(int width, int height) {
    DISPLAY_WIDTH = width;
    DISPLAY_HEIGHT = height;
    init();
}



public void init() { // This method initializes parts of the program.
    setSize(DISPLAY_WIDTH, DISPLAY_HEIGHT); // This initializes the height and width.
    getSides();
    initCells();
    //initGame();

    addMouseListener(this);
    addMouseMotionListener(this);
    this.addKeyListener(k);
    setFocusable(true);

    startStop = new StartButton();
    label = new labels();

    startStop.setBounds(X_GRID_OFFSET+5, Y_GRID_OFFSET+1+ROWS*(CELL_HEIGHT+1)+20, X_GRID_OFFSET+1+COLS*(CELL_WIDTH+1)-35, 50);
    label.setBounds(X_GRID_OFFSET, Y_GRID_OFFSET, label.WIDTH, label.HEIGHT);

    add(label);

    label.setVisible(true);

    paintloop = true;
    repaint();

}

public void initGame() {
    initSlider();
    drawBlocks(level);
    draw = true;
    TIME_BETWEEN_REPLOTS = 5;
}

void moveBall(Graphics g) {
    if (ballMovement == true) {
        ballx+=dirx;
        bally+=diry;
    }
    g.setColor(Color.BLUE);
    g.fillOval((int) ballx, (int) bally, balllength, balllength);
    g.setColor(Color.BLACK);
    g.drawOval((int) ballx, (int) bally, balllength, balllength);
}

void toggleBallMovement() {
    ballMovement = !ballMovement;
}


public void moveSlider(String dir) {
    for (int i = 0; i<COLS; i++) {
        cell[sliderLocation][i].setSliderNextTurn(false);
    }
    if (dir == "LEFT" && slideref > sliderlength/2) {
        slideref -=1;
        if (!ballMovement) {
            ballx-=CELL_WIDTH+1;
        }
    }
    if (dir == "RIGHT" && slideref < COLS-((sliderlength/2) + 1)) {
        slideref +=1;
        if (!ballMovement) {
            ballx+=CELL_WIDTH+1;
        }
    }
    for (int i = slideref-(sliderlength/2); i<slideref+((sliderlength/2) + 1); i++) {
        try {
            cell[sliderLocation][i].setSliderNextTurn(true);
        } catch (ArrayIndexOutOfBoundsException e) {

        }
    }
}

// This code sets up the grid, cells, and buttons.
public void paintComponent(Graphics g) {

    ballcenterx = (int) ballx + (balllength/2);
    ballcentery = (int) bally + (balllength/2);

    g.setColor(Color.GRAY);
    drawGrid(g);
    drawCells(g);
    drawButtons();
    if(draw) {
        drawBall(g);
    }

    if (checkBlocks() == 0 && draw) {
        level+=1;
        lives+=1;
        ballx = (CELL_WIDTH+1) * slideref + CELL_WIDTH+1;
        bally = 545;
        dirx = 3;
        diry = 3;
        toggleBallMovement();
        tbm = true;
        drawBlocks(level);
    }

    if (lives == 0) {
        clearBlocks(level);
        level = 1;
        drawBlocks(level);
        lives = 3;
        moveBall(g);
        moveSlider("");
        blocksBroken = 0;
    }
        if (titleoffset < -40) {
            add(startStop);
            startStop.setVisible(true);
            for (int i = 0; i<8; i++) {
                cell[i][0].setTitle(false);
            }
            if(draw) {
                startStop.setVisible(false);
            }
        }


    //setNextGen();

    if (paintloop) {

        try {
            Thread.sleep(TIME_BETWEEN_REPLOTS);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        if(!draw) {
            drawTitle(titleoffset);
            titleoffset-=1;
        }
        getSides();
        if(draw) {
            moveBall(g);
            titleoffset = COLS;
            /*
            try {
               image = ImageIO.read(new File("circle.jpg"));
            } catch (final IOException e) {
                e.printStackTrace();
            }    

            g.drawImage(image, imx, imy, this);
            */
        }
        checkCollisions();
        nextGeneration(); // This calls nextGeneration() at the end of the loop.
        repaint();
    }

    repaint();
}

public void drawBall(Graphics g) {
    if (paintloop == false) {
        g.setColor(Color.BLUE);
        g.fillOval((int) ballx, (int) bally, balllength, balllength);
        g.setColor(Color.BLACK);
        g.drawOval((int) ballx, (int) bally, balllength, balllength);
    }

    for (int i = 1; i<=lives; i++) {
        g.setColor(Color.BLUE);
        g.fillOval(i*50, (CELL_HEIGHT+1)*ROWS+X_GRID_OFFSET-23, balllength, balllength);
        g.setColor(Color.BLACK);
        g.drawOval(i*50, (CELL_HEIGHT+1)*ROWS+X_GRID_OFFSET-23, balllength, balllength);
    }
}

public void getSides() {
    int[] arg0 = new int[2];
    arg0[0] = ballcenterx;
    arg0[1] = ballcentery+balllength;
    returned = returnCell(arg0);
    if(returned == null) {
        return;
    } else {
        ballcor = returned;
    }
}

public void checkCollisions() {
    getSides();
    int[] arg0 = new int[2];
    //block collisions
    arg0[0] = ballcenterx + balllength;
    arg0[1] = ballcentery;
    int[] placeholder = returnCell(arg0); 
    if (placeholder != null && cell[placeholder[0]][placeholder[1]].getBlock() == true) {
        cell[placeholder[0]][placeholder[1]].setBlock(false);
        dirx*=-1;
        blocksBroken +=1;
    }
    arg0[0] = ballcenterx - balllength;
    placeholder = returnCell(arg0); 
    if (placeholder != null && cell[placeholder[0]][placeholder[1]].getBlock() == true) {
        cell[placeholder[0]][placeholder[1]].setBlock(false);
        dirx*=-1;
        blocksBroken +=1;
    }
    arg0[0] = ballcenterx;
    arg0[1] = ballcentery + balllength;
    placeholder = returnCell(arg0); 
    if (placeholder != null && cell[placeholder[0]][placeholder[1]].getBlock() == true) {
        cell[placeholder[0]][placeholder[1]].setBlock(false);
        diry*=-1;
        blocksBroken +=1;
    }
    arg0[1] = ballcentery - balllength;
    placeholder = returnCell(arg0); 
    if (placeholder != null && cell[placeholder[0]][placeholder[1]].getBlock() == true) {
        cell[placeholder[0]][placeholder[1]].setBlock(false);
        diry*=-1;
        blocksBroken +=1;
    }

    //slider and side collisions
    if (cell[ballcor[0]][ballcor[1]].getSlider() == true && ballMovement) {
        if (diry>0) {
            diry*=-1;
        }

        if (dirx<0) {
            if (lastKeyPressed == 'a') {
                dirx*=1.2;
            }

            if (lastKeyPressed == 'd') {
                dirx*=0.8;
            }
        } 

        if (dirx>0) {
            if (lastKeyPressed == 'd') {
                dirx*=1.2;
            }

            if (lastKeyPressed == 'a') {
                dirx*=0.8;
            }
        } 

    }
    /*
    arg0[0] = ballcenterx;
    arg0[1] = ballcentery;
    placeholder = returnCell(arg0); 
    if (placeholder != null && cell[placeholder[0]][placeholder[1]].getSlider() == true) {
    dirx*=-1;
    ballx+=CELL_WIDTH;
    }*/

    if (ballx+balllength>X_GRID_OFFSET+1+COLS*(CELL_WIDTH+1)) {  
        dirx*=-1;
    }
    if (ballx<X_GRID_OFFSET+1) {
        dirx*=-1;

    }
    if (bally+balllength>Y_GRID_OFFSET+1+ROWS*(CELL_HEIGHT+1)) {
        diry*=-1;
        lives-=1;
        ballx = (CELL_WIDTH+1) * slideref + CELL_WIDTH+1;
        bally = 545;
        dirx = 3;
        diry = 3;
        toggleBallMovement();
        tbm = true;
    }
    if (bally<Y_GRID_OFFSET+1) {
        diry*=-1;
    }

    if (Math.abs(dirx) > 16) {
        dirx *= 0.8;
    }

}

public void initCells() {

    // This code has a for loop inside a for loop.
    // The code below initializes each cell (alive or dead) so that the game is almost ready to begin.
    for (int row = 0; row < ROWS; row++) {
        for (int col = 0; col < COLS; col++) {
            cell[row][col] = new Cell(row, col);
        }
    }


}

public void initSlider() {
    for (int i = slideref-(sliderlength/2); i<slideref+((sliderlength/2) + 1); i++) {
        try {
            cell[sliderLocation][i].setSlider(true);
        } catch (ArrayIndexOutOfBoundsException e) {

        }
    }
}

public void drawBlocks(int level) {
    for (int i = 1; i<=level; i++) {
        for (int j = 1; j<COLS-1; j++) {
            cell[i][j].setBlock(true);
        }
    }
}

public void clearBlocks(int level) {
    for (int i = 1; i<=level; i++) {
        for (int j = 1; j<COLS-1; j++) {
            cell[i][j].setBlock(false);
        }
    }
}

public void drawTitle(int off) {
    String[] string = {
            "00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "01110011100111100110010010011001001011111000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "01001010010100001111010100100101001000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "01001010010100001001011000100101001000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "01110011100111001111011000100101001000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "01001011000100001001010100100101001000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "01001010100100001001010010100101001000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "01111010010111101001010010011000110000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
            "00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
    };
    convertBin(string, off);
}

public void convertBin(String[] string, int off) {
    for (int i = 0; i<string.length; i++) {
        for (int j = 0; j<string[i].length(); j++) {
            String arrstring = string[i];
            if (arrstring.charAt(j) == '1') {
                try {
                    cell[i][off+j].setTitle(true);
                    cell[i][off+j+1].setTitle(false);
                } catch(ArrayIndexOutOfBoundsException e) {

                }
            }
        }
    }
}

public void togglePaintLoop() {
    paintloop = !paintloop;
}


public void setPaintLoop(boolean value) {
    paintloop = value;
}

void drawSlide(Graphics g) {
    g.setColor(Color.BLUE);
    g.setColor(Color.GRAY);
}

// This draws the grid.
void drawGrid(Graphics g) {
    for (int row = 0; row <= ROWS; row++) {
        g.drawLine(X_GRID_OFFSET,
                Y_GRID_OFFSET + (row * (CELL_HEIGHT + 1)), X_GRID_OFFSET
                + COLS * (CELL_WIDTH + 1), Y_GRID_OFFSET
                + (row * (CELL_HEIGHT + 1)));
    }
    for (int col = 0; col <= COLS; col++) {
        g.drawLine(X_GRID_OFFSET + (col * (CELL_WIDTH + 1)), Y_GRID_OFFSET,
                X_GRID_OFFSET + (col * (CELL_WIDTH + 1)), Y_GRID_OFFSET
                + ROWS * (CELL_HEIGHT + 1));
    }
}


void drawCells(Graphics g) {
    // Have each cell draw itself
    for (int row = 0; row < ROWS; row++) {
        for (int col = 0; col < COLS; col++) {
            // The cell cannot know for certain the offsets nor the height
            // and width; it has been set up to know its own position, so
            // that need not be passed as an argument to the draw method
            cell[row][col].draw(X_GRID_OFFSET, Y_GRID_OFFSET, CELL_WIDTH,
                    CELL_HEIGHT, g);
        }
    }
}


// This draws the Buttons.
private void drawButtons() {
    startStop.repaint();
}

public KeyListener k = new KeyListener() {

    public void keyPressed(KeyEvent e) {
        int location = e.getKeyCode();
        if (location == KeyEvent.VK_A) {
            lastKeyPressed = 'a';
        }
        if (location == KeyEvent.VK_D) {
            lastKeyPressed = 'd';
        };
        if (location == KeyEvent.VK_P) {
            togglePaintLoop();
        };
        if (location == KeyEvent.VK_O) {
            if(tbm) {
                if(lastKeyPressed == 'a') {
                    dirx*=-1;
                    dirx*=1.2;
                }
                if(lastKeyPressed == 'd') {
                    dirx*=1.2;
                }
                toggleBallMovement();
                tbm = !tbm;
            }
        };
    }

    public void keyTyped(KeyEvent e) {
        int location = e.getKeyCode();
        if (location == KeyEvent.VK_A) {
        }
        if (location == KeyEvent.VK_D) {
        };
    }

    public void keyReleased(KeyEvent e) {
        int location = e.getKeyCode();
        if (location == KeyEvent.VK_A) {
            lastKeyPressed = '/';
        }
        if (location == KeyEvent.VK_D) {
            lastKeyPressed = '/';
        };


    }

};

public static void print(int x, int y) {
    System.out.println(x + "  " + y);
}

// This clears the grid and sets all the cells to dead.
private void clearGrid() {
    for (int i = 0; i<ROWS; i++) {
        for (int j = 0; j<COLS; j++) {
            cell[i][j].setAliveNextTurn(false);
            setNextGen();
        }
    }
}


// This code sets which cells are alive and which are dead in the next generation.
private void nextGeneration() {
    if (lastKeyPressed == 'a') {
        moveSlider("LEFT");
    }
    if (lastKeyPressed == 'd') {
        moveSlider("RIGHT");
    }
    if(lastKeyPressed == '/') {
        nextGen++;
        return;
    }
    setNextGen();
    nextGen++;
    //System.out.println(blocksBroken);
}

private int checkBlocks() {
    int c = 0;
    for(int i = 0; i<COLS-1; i++)
    for(int j = 0; j<=level; j++) {
        if(cell[j][i].getBlock() == true) {
            c+=1;
        }
    }
    return c;
}

private void setNextGen(){
    for (int a = 0; a<COLS; a++) {
        if(cell[sliderLocation][a].getSliderNextTurn() == false) {
            cell[sliderLocation][a].setSlider(false);
        }
        if(cell[sliderLocation][a].getSliderNextTurn()) {
            cell[sliderLocation][a].setSlider(true);
            //cell[sliderLocation][a].setSliderNextTurn(false);
        }
    }
}

public void mouseClicked(MouseEvent arg0) {

}



public void mouseEntered(MouseEvent arg0) {

}


public void mouseExited(MouseEvent arg0) {

}


// This code sets cells alive or dead when the mouse is pressed on them. 
public void mousePressed(MouseEvent arg0) {

}

// This returns what cells are pressed.
public int[] returnCell(int[] arg0) {
    int[] rc;
    boolean rrc = false;
    rc = new int[2];
    for (int j = 0; j<ROWS; j++) {
        for (int i = 0; i<COLS; i++) {
            if ((arg0[0] > CELL_WIDTH*(i)+i+X_GRID_OFFSET && arg0[0] < CELL_WIDTH*(i+1)+i+X_GRID_OFFSET)){
                if ((arg0[1] > CELL_HEIGHT*(j)+j+Y_GRID_OFFSET && arg0[1] < CELL_HEIGHT*(j+1)+j+Y_GRID_OFFSET)){
                    rc[0] = j;
                    rc[1] = i;
                    rrc = true;
                }
            }
        }
    }
    if(rrc == false) {
        return null;
    }
    return rc;
}



public void mouseReleased(MouseEvent arg0) {

}


// This code sets cells alive or dead when the mouse is dragged around.
public void mouseDragged(MouseEvent arg0) {

}



public void mouseMoved(MouseEvent arg0) {

}

// This is the Start/Stop Button. According to what the button says, it will Start/Stop the program when pressed.
private class StartButton extends JButton implements ActionListener {
    StartButton() {
        super("Start");
        addActionListener(this);
    }

    public void actionPerformed(ActionEvent arg0) {
        // nextGeneration(); // test the start button
        if (this.getText().equals("Start")) {
            initGame();
            startStop.setVisible(false);
        }
    }
}

private class labels extends JLabel {
    labels() {
        super("this is a label");
    }

}

}

share|improve this question

1 Answer

Hmmmm....

Main -

DISPLAY_HEIGHT and DISPLAY_WIDTH are both visible outside the given class. This may be confusing, especially as you seem to use similarly named variables inside Display. They should (likely) still be marked private. Also, given that they're initialized as part of their declaration, they should be marked static as well, making them true compile-time constants.
frame should be marked private and final. I seriously doubt that you're going to be switching windows in the middle of the game.
The constructor knows too much about the external (command line) representation of arguments. Personally, I'd prefer working with the 'correct' one (ie an actual boolean object instead of something that just says 'true'). Although, given that you don't yet have any arguments, it would be better to just use a no-arg constructor. If you do end up with a large number of arguments, something like a Factory Pattern, similarly to how most object parsers work.
Also, don't retain commented-out code, unless there are documentation reasons (perhaps as a documented reference to an inappropriate call). Rely on your source-code repository to retain history.

Use of SwingUtilities is good, and about all I'd do in a main(...) method too - just enough to tell a program to create itself, then duck.


Cell -

In general, this class feels like you're trying to do to much in it. I'm not necessarily talking about the inclusion of the drawing, although that might be the case with depending on architecture. More, it feels like you're making it try to fulfill to many backend roles.

You've prefixed everything with 'my' - don't, it's basically noise. If you need to differentiate between the 'current' reference and an incoming parameter, use the this reference.

public Cell(int x, int y) {
    this(x, y, false, Color.GRAY);
}

Here, you're using Color.GRAY, which you've also defined as DEFAULT_DEAD - are you just using the same color, and the fact that it's also the constant is coincidence, or should it have been that default? Also, the comment for myAlive indicates that it's for use in determining alive/dead colors - so why do you pass in both a boolean and the 'dead' color?

public void setSideCell(boolean sides) {
    side = sides;
}

You seem to have a confusion between singluar/plurals, here.

public boolean getSideCell() {
    return side;
}

Usually, boolean getters use 'is', 'has', etc. - so, isSideCell, hasMoreItems, etc.

public void setTitle(boolean bob) {
    if (bob) {
        title = true;
        setColor(Color.BLUE);
    } else {
        title = false;
        setColor(Color.GRAY);
    }
}

There's two problems here:

  1. What is bob? Don't use 'nonsense'/unrelated variable names. Always use explanatory names.
  2. The use of boolean arguments in general is considered problematic. You also have to watch out for internal variables having the same effect.

The setSlider(...) method has a similar issue. Depending on the relationship between them, I wonder if you could key them off the color?

public boolean getXN(Cell[][] cell) {
    if (cell[myY][myX-1].getAlive() == true || cell[myY][myX+1].getAlive()) {
        return true;
    }
    return false;
}

This method gets.... what? Whether the neighbors in the x-axis are alive? This (and the complementary getYN(...) method) should probably be simplified to:

public boolean getXN(Cell[][] cell) {
    return cell[myY][myX-1].getAlive() || cell[myY][myX+1].getAlive();
}

Among other things, comparing booleans to the true/false constants is considered bad practice, and the simple comparison is better. Among other things, it makes certain errors like the following impossible:

public void setColorChoice(Boolean bool) {
    if (bool = true) {
        colorchoice = true;
    } else {
        colorchoice = false;
    }
}

(Although I tweaked an example from your code, you don't actually have this bug. Note that if you do this deliberately (especially if you don't document it), future maintainers will have every justification to "Spindle, Fold, or Mutilate" you.)
The real version of above should be simplified to:

public void setColorChoice(Boolean bool) {
    colorchoice = bool;
}

The method setBlock(...) should use a switch statement - any time you have a bunch of exclusive options like this, consider that instead (or maybe a Command Pattern, depending). Something like:

    switch (myY) {
        case 2:
            myColor = Color.ORANGE;
            break;
        // you can fill in the other colors yourself
        default:
            myColor = Color.GRAY;

Note that break should be in every case (except the default), and the default should go last. Failure to do so results in weird behavior. You might want to also put it into another method, to make dealing with the way bblock makes the method behave.

The method setAlive(...) boils down to:

public void setAlive(boolean alive, Color color) {
    myAlive = alive;
    myColor = !alive ? color : (colorchoice ? getRandomColor() : Color.yellow);
}

(And yes, I recommend creating getRandomColor()).

The following:

public int getNeighbors() {
    return myNeighbors;
}

Makes it sound like you're actually getting other cells, but you're really getting the count of the neighbors. So.... call it getCountOfNeighbors() instead.

The method calcNeighbors(...) seems to be expecting exceptions. Throwing an 'exception' is a relatively expensive operation, and it can put program state into weird places, sometimes. You have the size of your array right - add the necessary bounds checking, and eliminate the try/catch block altogether. And don't forget to remove the comparison to the boolean constant.


Err, that's all I can do at the moment. I may be back later....

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.