Problem
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<Integer, Map<Integer, Room>> map = new HashMap<Integer, Map<Integer, Room>>();
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<Integer, Room>());
}
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 (eastPossible) {
System.out.print(" East (e)");
}
if (southPossible) {
System.out.print(" South (s)");
}
if (westPossible) {
System.out.print(" West (w)");
}
System.out.print(" ? ");
BufferedReader in = new BufferedReader(new InputStreamReader(System.in));
String direction = in.readLine();
if (direction.equals("n") && northPossible) {
currentY++;
} else if (direction.equals("s") && southPossible) {
currentY--;
} else if (direction.equals("e") && eastPossible) {
currentX++;
} else if (direction.equals("w") && westPossible) {
currentX--;
}
currentRoom = getRoom(currentX, currentY);
currentRoom.enter(player);
}
public void startQuest(Player player) throws IOException {
while (player.isAlive() && !isComplete()) {
movePlayer(player);
}
if (player.isAlive()) {
System.out.println(Art.CROWN);
} else {
System.out.println(Art.REAPER);
}
}
public static Dungeon newInstance() {
Dungeon dungeon = new Dungeon();
dungeon.putRoom(0, 0, Room.newRegularInstance());
dungeon.putRoom(-1, 1, Room.newRegularInstance());
dungeon.putRoom(0, 1, Room.newRegularInstance());
dungeon.putRoom(1, 1, Room.newRegularInstance());
dungeon.putRoom(-1, 2, Room.newRegularInstance());
dungeon.putRoom(1, 2, Room.newRegularInstance());
dungeon.putRoom(-1, 3, Room.newRegularInstance());
dungeon.putRoom(0, 3, Room.newRegularInstance());
dungeon.putRoom(1, 3, Room.newRegularInstance());
dungeon.putRoom(0, 4, Room.newBossInstance());
dungeon.currentRoom = dungeon.getRoom(0, 0);
return dungeon;
}
}
Here’s the Monster class:
public final class Monster {
private final String name;
private final String description;
private int hitPoints;
private final int minDamage;
private final int maxDamage;
private final static Random random = new Random();
private final static Set<Integer> monstersSeen = new HashSet<Integer>();
private final static int NUM_MONSTERS = 3;
public static Monster newRandomInstance() {
if (monstersSeen.size() == NUM_MONSTERS) {
monstersSeen.clear();
}
int i;
do {
i = random.nextInt(NUM_MONSTERS);
} while (monstersSeen.contains(i));
monstersSeen.add(i);
if (i == 0) {
return new Monster("Harpy", Art.HARPY, 40, 8, 12);
} else if (i == 1) {
return new Monster("Gargoyle", Art.GARGOYLE, 26, 4, 6);
} else {
return new Monster("Hobgoblin", Art.HOBGOBLIN, 18, 1, 2);
}
}
public static Monster newBossInstance() {
return new Monster("Dragon", Art.DRAGON, 60, 10, 20);
}
private Monster(String name, String description, int hitPoints, int minDamage, int maxDamage) {
this.name = name;
this.description = description;
this.minDamage = minDamage;
this.maxDamage = maxDamage;
this.hitPoints = hitPoints;
}
@Override
public String toString() {
return name;
}
public String getDescription() {
return description;
}
public String getStatus() {
return "Monster HP: " + hitPoints;
}
public int attack() {
return random.nextInt(maxDamage - minDamage + 1) + minDamage;
}
public void defend(Player player) {
int attackStrength = player.attack();
hitPoints = (hitPoints > attackStrength) ? hitPoints - attackStrength : 0;
System.out.printf(" %s hits %s for %d HP of damage (%s)n", player, name, attackStrength,
getStatus());
if (hitPoints == 0) {
System.out.println(" " + player + " transforms the skull of " + name
+ " into a red pancake with his stone hammer");
}
}
public boolean isAlive() {
return hitPoints > 0;
}
}
Battle class:
public final class Battle {
public Battle(Player player, Monster monster) throws IOException {
System.out.println("You encounter " + monster + ": " + monster.getDescription() + "n");
System.out.println("Battle with " + monster + " starts (" + player.getStatus() + " / "
+ monster.getStatus() + ")");
BufferedReader in = new BufferedReader(new InputStreamReader(System.in));
while (player.isAlive() && monster.isAlive()) {
System.out.print("Attack (a) or heal (h)? ");
String action = in.readLine();
if (action.equals("h")) {
player.heal();
} else {
monster.defend(player);
}
if (monster.isAlive()) {
player.defend(monster);
}
}
}
}
Room class:
public final class Room {
private final String description;
private final Monster monster;
private final Boolean isBossRoom;
private final static Random random = new Random();
private final static Set<Integer> roomsSeen = new HashSet<Integer>();
private final static int NUM_ROOMS = 7;
private Room(String description, Monster monster, Boolean isBossRoom) {
this.description = description;
this.monster = monster;
this.isBossRoom = isBossRoom;
}
public static Room newRegularInstance() {
if (roomsSeen.size() == NUM_ROOMS) {
roomsSeen.clear();
}
int i;
do {
i = random.nextInt(NUM_ROOMS);
} while (roomsSeen.contains(i));
roomsSeen.add(i);
String roomDescription = null;
if (i == 0) {
roomDescription = "a fetid, dank room teeming with foul beasts";
} else if (i == 1) {
roomDescription = "an endless mountain range where eagles soar looking for prey";
} else if (i == 2) {
roomDescription = "a murky swamp with a foul smelling odour";
} else if (i == 3) {
roomDescription = "a volcano with rivers of lava at all sides";
} else if (i == 4) {
roomDescription =
"a thick forest where strange voices call out from the trees high above";
} else if (i == 5) {
roomDescription =
"an old abandoned sailing ship, littered with the remains of some unlucky sailors";
} else if (i == 6) {
roomDescription = "a cafe filled with hipster baristas who refuse to use encapsulation";
} else {
}
return new Room(roomDescription, Monster.newRandomInstance(), false);
}
public static Room newBossInstance() {
return new Room("a huge cavern thick with the smell of sulfur", Monster.newBossInstance(),
true);
}
public boolean isBossRoom() {
return isBossRoom;
}
public boolean isComplete() {
return !monster.isAlive();
}
@Override
public String toString() {
return description;
}
public void enter(Player player) throws IOException {
System.out.println("You are in " + description);
if (monster.isAlive()) {
new Battle(player, monster);
}
}
}
And the Player class:
public final class Player {
private final String name;
private final String description;
private final int maxHitPoints;
private int hitPoints;
private int numPotions;
private final int minDamage;
private final int maxDamage;
private final Random random = new Random();
private Player(String name, String description, int maxHitPoints, int minDamage, int maxDamage,
int numPotions) {
this.name = name;
this.description = description;
this.maxHitPoints = maxHitPoints;
this.minDamage = minDamage;
this.maxDamage = maxDamage;
this.numPotions = numPotions;
this.hitPoints = maxHitPoints;
}
public int attack() {
return random.nextInt(maxDamage - minDamage + 1) + minDamage;
}
public void defend(Monster monster) {
int attackStrength = monster.attack();
hitPoints = (hitPoints > attackStrength) ? hitPoints - attackStrength : 0;
System.out.printf(" " + name + " is hit for %d HP of damage (%s)n", attackStrength,
getStatus());
if (hitPoints == 0) {
System.out.println(" " + name + " has been defeated");
}
}
public void heal() {
if (numPotions > 0) {
hitPoints = Math.min(maxHitPoints, hitPoints + 20);
System.out.printf(" %s drinks healing potion (%s, %d potions left)n", name,
getStatus(), --numPotions);
} else {
System.out.println(" You've exhausted your potion supply!");
}
}
public boolean isAlive() {
return hitPoints > 0;
}
public String getStatus() {
return "Player HP: " + hitPoints;
}
@Override
public String toString() {
return name;
}
public String getDescription() {
return description;
}
public static Player newInstance() {
return new Player("Mighty Thor",
"a musclebound hulk intent on crushing all evil in his way", 40, 6, 20, 10);
}
}
Solution
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<Integer, Map<Integer, Room>> map = new HashMap<Integer, Map<Integer, Room>>();
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<Integer> monstersSeen = new HashSet<Integer>();
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.
IO: You use everywhere System.out.println and System.in (BTW, Scanner is much more convenient than a Reader). Even if you want to switch from console output to a simple Swing application with little more than a text area, you have to change everything. Same story if you want to provide a translation for your game.
So follow the the Single Responsibility Principle: Your model classes like Player and Monster should care about the state of the game and its transitions, and not about IO. Even if this is not a perfect separation, it’s still better to send Strings to an IO class, and ask it for input, than doing everything locally. Then the IO class is in charge how to present the data. Later you might want to send just messages like PlayerDied() or HealthDownTo(42) to IO, which gets the real output from a text file or so.
Room: if (i == 0) {... cascades are written better using switch. In your case an array with all strings would be even better, you need just roomDescriptions[i] to get the right one.
There’s nothing about Java, but pure OO principle.
Why Player and Monster are totally different classes?
There should be the class “Objet” then its descendant “Creature” (or whatever) then, from it, Player and Monster.
Object
|
|
|
Creature
|
/
/
/
/
| |
Player Monster
Creature can move, carry stuff, have tons of properties that Player and Monster share (attack, defense, health, and so on).