14
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ You shouldn't be using System.out.println() directly - in fact, your back-end classes should have absolutely no clue that it's a text based system. Interestingly, doing so makes a change to a graphical interface much easier, later. \$\endgroup\$ – Clockwork-Muse Mar 13 '12 at 22:20
  • \$\begingroup\$ If you're looking for some inspiration, CoffeeMUD is a pretty cool open source MUD server. \$\endgroup\$ – mindvirus Mar 20 '12 at 12:29
14
\$\begingroup\$
    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.

\$\endgroup\$
  • \$\begingroup\$ Thanks! Great feedback. By the way, do you think its bad form to create factories for everything upfront? I was thinking this would make it easier later on if more complexity needs to be added to the factory, but perhaps this is overkill? \$\endgroup\$ – padawan Mar 14 '12 at 23:19
  • 1
    \$\begingroup\$ @padawan, I wouldn't say that you should have factories for everything. But I would say that an object shouldn't go through complex machinations to create itself. If you have to parse files, pick random numbers, etc. then I'd go for a factory. Note that this doesn't mean you should have lots of factories. In your case, I'd probably go for a single factory, a DungeonFactory. The DungeonFactory would build the Monsters/Rooms/etc and put them all together. \$\endgroup\$ – Winston Ewert Mar 15 '12 at 3:48
  • \$\begingroup\$ Thanks. I'm trying to understand Item 1: Consider static factory methods instead of constructors, in Effective Java. It gives some pretty solid reasons for using factory methods over constructors. So if I understand you, you're saying that a factory class is only needed for the Dungeon. What's your view on static factory methods within the class (e.g. within Monster or Room) as a substitute for constructors? Should these always be used, or only in certain cases? Thanks again! \$\endgroup\$ – padawan Mar 15 '12 at 17:07
  • 1
    \$\begingroup\$ @padawan, I've been trying to figure out exactly what my theory is on constructors vs static factory methods. I've come to the conclusion that I prefer very simple constructors. My constructors usually just copy the constructor parameters into fields. If I need something more complicated then I'd for for a factory or static method. \$\endgroup\$ – Winston Ewert Mar 17 '12 at 17:32
  • \$\begingroup\$ Random objects are generally static to reduce recreation of the object. \$\endgroup\$ – Eva Jan 17 '13 at 2:13
7
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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).

\$\endgroup\$
  • 7
    \$\begingroup\$ Composition is usually better. \$\endgroup\$ – Eva Jul 5 '12 at 11:42
  • \$\begingroup\$ @Eva Of course. But langages like Php and Pure C dont have multiple inheritance (and dont tell me about traits in Php...). That's why I said "pure OO". But you're 100% right for langages that have multiple inheritance/composition feature. \$\endgroup\$ – Olivier Pons Jan 31 '18 at 8:38

protected by Community Jan 9 '18 at 15:46

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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