Text-based RPG in Java

I'm writing a simple text-based RPG in Java. I think it's a good exercise to practice OO and think about how objects should best interact. I'd be interested in hearing any thoughts!

The Game class contains a single game:

public final class Game {

    private final Player player = Player.newInstance();

    public void play() throws IOException {
        System.out.println("You are " + player + " " + player.getDescription());
        Dungeon.newInstance().startQuest(player);
    }

    public static void main(String[] args) throws IOException {
        Game game = new Game();
        game.play();
    }

}

Here's a simple Dungeon class, a collection of Rooms laid out in the map. The player moves from room to room, encountering and battling monsters.

```
public final class Dungeon {

private final Map> map = new HashMap>();
private Room currentRoom;
private int currentX = 0;
private int currentY = 0;

private Dungeon() {
}

private void putRoom(int x, int y, Room room) {
if (!map.containsKey(x)) {
map.put(x, new HashMap());
}
map.get(x).put(y, room);
}

private Room getRoom(int x, int y) {
return map.get(x).get(y);
}

private boolean roomExists(int x, int y) {
if (!map.containsKey(x)) {
return false;
}
return map.get(x).containsKey(y);
}

private boolean isComplete() {
return currentRoom.isBossRoom() && currentRoom.isComplete();
}

public void movePlayer(Player player) throws IOException {
boolean northPossible = roomExists(currentX, currentY + 1);
boolean southPossible = roomExists(currentX, currentY - 1);
boolean eastPossible = roomExists(currentX + 1, currentY);
boolean westPossible = roomExists(currentX - 1, currentY);
System.out.print("Where would you like to go :");
if (northPossible) {
System.out.print(" North (n)");
}
if

private final Player player = Player.newInstance();

Function like newInstance are suspicious. I immeadieatly wonder why you didn't use new Player. In some cases you use newRandomInstance which I like better because it tells me what you are really up to.

public static void main(String[] args) throws IOException {

Having your main function throw an IOException is probably not the best idea. As it is you've got all kinds of functions that throw IOExceptions despite not really being IO related. Since there is really nothing you can do with the IOException I suggest you catch them when the happen and then rethrow them:

throw new RuntimeException(io_exception);

That you won't clutter the code with exceptions information you don't handle anyways.

private final Map> map = new HashMap>();

It seems to me that you'd be better off using a 2D array to hold the map rather then this. It would simplify your code in quite a few places.

System.out.print("Where would you like to go :");
    if (northPossible) {
        System.out.print(" North (n)");
    }

As Landei said, you are better off keeping your input/output in separate classes from the actual game logic.

private Room currentRoom;
private int currentX = 0;
private int currentY = 0;

It seems to me that these belong as part of the Player, not the dungeon.

public void startQuest(Player player) throws IOException {
    while (player.isAlive() && !isComplete()) {
        movePlayer(player);
    }

It's a little odd for a function named startQuest to continue on until the player dies or wins.

private final static Random random = new Random();
private final static Set monstersSeen = new HashSet();

I recommend avoiding static variables. (Constants are fine). You lose some flexibility when you use statics. In your case, I think you should really put that logic in a factory class. Also, you really shouldn't have a class-specific instance of Random. You want to share a single Random amongst all your objects.

if (roomsSeen.size() == NUM_ROOMS) {
        roomsSeen.clear();
    }
    int i;
    do {
        i = random.nextInt(NUM_ROOMS);
    } while (roomsSeen.contains(i));
    roomsSeen.add(i);

You do this basic thing multiple times, which suggests you should think about finding a way to write one class you can use in both cases.

if (monster.isAlive()) {
        new Battle(player, monster);
    }

Having action occour as a side of creating an object isn't a good idea. At least have the action occur as a result of calling a method.

The big thing here is to seperate the user interface (reading and writing to the console) from the game logic itself. The other things I point out could be improved, but that is where the biggest problems will arise.

StackExchange Code Review Q#9999, answer score: 14