Reorganize a maze game from one large class into smaller classes

Question

I'm new to Java and I really don't understand how to make more classes work like one. So if anybody could explain and help me divide my large code into smaller classes (like Player.java, Main.java, Move.java, etc.). I want to check if the player hits the wall, and the easiest way is to make player object a blocks object and then check if they intersect. But I don't know how to make objects from what I have.

import java.awt.Color;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.Image;
import java.awt.Toolkit;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;
import java.awt.image.BufferedImage;
import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.io.InputStreamReader;

import javax.swing.Timer;
import javax.imageio.ImageIO;
import javax.swing.JFrame;

import org.omg.CORBA.portable.InputStream;

import java.io.Reader;

public class NewTest extends JFrame implements KeyListener {
/**
 * 
 */
private static final long serialVersionUID = 1L;
private Image TestImage;
private BufferedImage bf;
private BufferedImage bufferedImage;
private int cordX = 100;
private int cordY = 100;
public int mapy=25;
public int mapx=mapy;
 public int size= 20;
private boolean down, up, left, right;
private Image wall = Toolkit.getDefaultToolkit().getImage("image/Koala.jpg");
private Image no = Toolkit.getDefaultToolkit().getImage("image/house.jpg");

public static int[][] 
        map =               {{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0},
                             {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0},
                            {0, 10, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 11},
                            {0, 2, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0, 3, 0, 4, 0, 5, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 1, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 5, 0, 10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
                            {0, 12, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 13}};

public NewTest() throws IOException {
    setTitle("Testing....");
    setSize(mapy*size+50,mapx*size+50);
    imageLoader();
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    setVisible(true);
}

public void imageLoader() throws IOException {
    TestImage = Toolkit.getDefaultToolkit().getImage("image/Koala.jpg");

    addKeyListener(this);
}

public void update(Graphics g){
       paint(g);
}


public void paint(Graphics g){

    bf = new BufferedImage( this.getWidth(),this.getHeight(), BufferedImage.TYPE_INT_RGB);

try{
animation(bf.getGraphics());
g.drawImage(bf,0,0,null);
}catch(Exception ex){

}


}

public void animation(Graphics g) {
    super.paint(g);
    Graphics2D g2d = (Graphics2D)g;
    for (int y = 0; y <= mapy; y++){
        for (int x = 0; x <= mapx; x++){

            int L = x * size;
            int U = y * size;
            int R = size;
            int D = size;
               /// g2d.rotate(Math.toRadians(-90));

            if (map[y][x] == 1){
                //no black wall
                g2d.setColor(Color.green);
                g2d.fillRect(L, U, R, D);


            }else if (map[y][x] == 2){
                //on left
                g2d.setColor(Color.green);
                g2d.fillRect(L, U, R, D);
                g2d.setColor(Color.black);
                g2d.fillRect(L-size/2 +2, U, 8, size);

            }else if (map[y][x] == 3){
                //on right
                g2d.setColor(Color.green);
                g2d.fillRect(L, U, R, D);
                g2d.setColor(Color.black);
                g2d.fillRect(L+size, U, 8, size);

            }else if (map[y][x] == 4){
                //on top
                g2d.setColor(Color.green);
                g2d.fillRect(L, U, R, D);
                g2d.setColor(Color.black);
                g2d.fillRect(L, U-size/2 + 2, size, 8);

            }else if (map[y][x] == 5){
                //on bottom
                g2d.setColor(Color.green);
                g2d.fillRect(L, U, R, D);
                g2d.setColor(Color.black);
                g2d.fillRect(L, U+size, size, 8);
            }else if (map[y][x] == 10){
                //on bottom
                g2d.setColor(Color.green);
                g2d.fillRect(L, U, R, D);
                g2d.setColor(Color.black);
                g2d.fillRect(L, U-size/2 + 2, size, 8);
                g2d.fillRect(L-size/2 +2, U, 8, size );
            }else if (map[y][x] == 11){
                //on bottom
                g2d.setColor(Color.green);
                g2d.fillRect(L, U, R, D);
                g2d.setColor(Color.black);
                g2d.fillRect(L, U+size, size, 8);
            }else if (map[y][x] == 12){
                //on bottom
                g2d.setColor(Color.green);
                g2d.fillRect(L, U, R, D);
                g2d.setColor(Color.black);
                g2d.fillRect(L, U+size, size, 8);
            }else if (map[y][x] == 13){
                //on bottom
                g2d.setColor(Color.green);
                g2d.fillRect(L, U, R, D);
                g2d.setColor(Color.black);
                g2d.fillRect(L, U+size, size, 8);
            }

        }

    }
    g.drawImage(TestImage, cordX, cordY,20 , 20, this);

   }


public static void main(String[] args) {
    try {
        new NewTest();
    } catch (IOException e) {

        e.printStackTrace();
    }
}

public void keyPressed(KeyEvent ke) {
    switch (ke.getKeyCode()) {
        case KeyEvent.VK_RIGHT:
            right = true;
            break;
        case KeyEvent.VK_LEFT:
            left = true;
            break;
        case KeyEvent.VK_DOWN:
            down = true;
            break;
        case KeyEvent.VK_UP:
            up = true;
            break;
    }
    updateState();
}

public void keyReleased(KeyEvent ke) {
    switch (ke.getKeyCode()) {
        case KeyEvent.VK_RIGHT:
            right = false;
            break;
        case KeyEvent.VK_LEFT:
            left = false;
            break;
        case KeyEvent.VK_DOWN:
            down = false;
            break;
        case KeyEvent.VK_UP:
            up = false;
            break;
    }
    updateState();
}

protected void updateState() {
    if (right) {
        cordX += 5;
    } else if (left) {
        cordX -= 5;

    }
    if (down) {
        cordY += 5;
    } else if (up) {
        cordY -= 3;
    }
    if (down && up) {
        cordY += 0;
        cordX += 0;
    }
    if (right && left) {
        cordX += 0;
        cordY += 0;
    }

    System.out.printf("X:");System.out.println(cordX);
    System.out.printf("Y:");System.out.println(cordY);
    repaint();
}

public void keyTyped(KeyEvent ke) {
}

}

Answer 1

Splitting your classes

Rename NewTest to something more meaningful. What is the main responsibility of this class? Think about that, and rename it accordingly.

As a first step, the main class should not implement KeyListener. A class should have only one responsibility, and a main class already has one: being the main class. Create an inner class that implements KeyListener and trigger the appropriate state updates.

Once your class has a proper name corresponding to its main responsibility, the main method should start to look odd... You see, launching the class is yet another responsibility. So move the launching functionality out to a different class, for example KoalaGameRunner. This might seem a bit overkill at first, but it's not. You can envision in the future adding command line parsing functionality in KoalaGameRunner.

Keep looking at the main class that seems to do too many things with a critical eye, keeping in mind the single responsibility principle. Check every feature if's part of the main responsibility or if it can be extracted to a class of its own. Repeat until you cannot find any more features to extract, when everything seem to fit well, and the class looks like a good cohesive unit.

The single responsibility principle

Apply the single responsibility principle to methods even more strictly than classes. For example this is strange:

public void imageLoader() throws IOException {
    TestImage = Toolkit.getDefaultToolkit().getImage("image/Koala.jpg");

    addKeyListener(this);
}

This method loads an image, and then... adds a listener to this? That second step doesn't make any sense. And the first step was actually really 2 steps: load an image and assign it to a field. So that already violated the single responsibility principle.

This method would be better this way:

Image imageLoader(String path) throws IOException {
    return Toolkit.getDefaultToolkit().getImage(path);
}

Now it does only one thing: it loads an image. Even better, it can load any image specified by a path. Even better, it doesn't mess with the internal state of the main class. And at this point I hope you see that this method could be a good candidate to extract to a utility class.

Bugs

The updateState method seems to have a bug in the logic:

if (right) {
    cordX += 5;
} else if (left) {
    cordX -= 5;
}
if (right && left) {
    cordX += 0;
    cordY += 0;
}

If right and left are pressed at the same time, then you increment cordX by 5, and then you have the right && left condition, to do nothing. You probably meant this way:

if (right && left) {
    // do nothing
} else if (right) {
    cordX += 5;
} else if (left) {
    cordX -= 5;
}

The same goes for up and down.

Exception handling

This seems pointless:

public static void main(String[] args) {
    try {
        new NewTest();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

Without the try-catch, effectively the same thing will happen: the program will crash, and the stack trace will be printed on the console. Even better, its exit code will be a failure, instead of success. Just let it crash:

public static void main(String[] args) throws IOException {
    new NewTest();
}

Answer 2

Unused Imports

About half of your imports are unused. It's best to remove these, so it's easier to see which imports you actually do use.

Coding Style

Your indentations are off quite often. This makes the code hard to read. You can use any IDE to fix this easily.

Overridable Method Call from Constructor

It's not a good idea to call a method which might be overridden by a subclass from a constructor, because it can lead to bugs.

You can fix this by making imageLoader private.

@Override annotations

It's always good to use @Override for overriding methods (like update, paint, keyPressed, etc).

Single responsibility methods

Ideally, methods should only do one thing. You imageLoader for example does two things: it loads an image, and it adds the keylistener. If I read imageLoader, I would definitely not expect the second functionality.

Image loading

You are loading some images in the field declaration, and some in imageLoader. I would handle this uniformly (in the imageLoader method).

Variable Names

Variable names should be written in camelCase. So L should for example be l and mapy should be mapY.

But I would prefer more descriptive names. L, U, R, and D don't really tell me what those variables do.

Magic Numbers

You should avoid hardcoding random numbers in your code. Define them in a static final variable instead. If you give the variable a good name, it might also express why down leads to + 5 while up leads to - 3.

Comments

You don't have to overdo it, but some comments on what the code does would be nice (like a class comment, a comment on cordX and cordY, mapy and mapx, etc.

OOP

I didn't comment on the general structure of the code because you already know that it should be separated in different classes. See also my comment for clarification.