Object Oriented Ascii dungeon game

Question

I have solved an assignment (Exercise 33) from the MOOC Object-Oriented programming with Java, part II, but I'm not enrolled in said course.

Assignment Summary

In this exercise, you implement a dungeon game. In the game, the player is in a dungeon full of vampires. The player has to destroy the vampires before his lamp runs out of battery and the vampires can suck his blood in the darkness. The player can see the vampires with a blinking of their lamp, after which they have to move blind before the following blinking. With one move, the player can walk as many steps as they want. The game situation, i.e. the dungeon, the player and the vampires are shown in text form. The first line in the print output tells how many moves the player has left (that is to say, how much battery the lamp has). After that, the print output shows player and vampire positions, which in turn are followed by the game map.
The coordinates are calculated starting from the high left corner of the game board. The user can move by giving a sequence of commands and pressing Enter. The commands are:

• w go up
• s go down
• a go left
• d go right

When the user commands are executed (the user can give many commands at once), a new game situation is drawn. If the lamp charge reac hes 0, the game ends and the text YOU LOSE is printed on the board.

The vampires move randomly in the game, and they take one step for each step the player takes. If the player and a vampire run into each other (even momentarily) the vampire is destroyed. If a vampire tries to step outside the board, or into a place already occupied by another vampire, the move is not executed. When all the vampires are destroyed, the game ends and it prints YOU WIN. The player starts the game in the position 0,0 Player and vampires can not move out of the dungeon and two vampires cannot step into the same place

Question:
How do you refactor this code so that it follows OOP, reads better, is manageable? How can I write method names and classes better? How do you know which entities are to be made separate classes, and how to use classes efficiently?

Feels like there is a lot of bloated classes and repeated code. Also coding took a long time, spent a lot of time in refactoring and debugging hell. How do you plan and design your code? How do you produce workable, clean code in less time? What do you focus on?

Movement class - translates keyboard key into direction

package dungeon;

import useful.BiMap;
import useful.Randomizer;


public class MovementKey {
    private BiMap<Direction, String> movementKeys;
    private Randomizer randomizer;

    enum Direction {
        UP,
        LEFT,
        DOWN,
        RIGHT
    }

    public MovementKey() {
        this.movementKeys = new BiMap<>();
        String[] keys = {"w", "a", "s", "d"};
        for (int i = 0; i < keys.length; i++) {
            this.movementKeys.put(Direction.values()[i], keys[i]);
        }
        this.randomizer = new Randomizer();
    }

    public static int noOfKeys() {
        return Direction.values().length;
    }

    public Direction getRandomDirection() {
        int randomNumber = randomizer.random(noOfKeys());
        return Direction.values()[randomNumber];
    }

    public String getRandomKey() {
        return movementKeys.get(getRandomDirection());
    }

    public Direction getDirection(String ascii) {
        return movementKeys.getKey(ascii);
    }
}

Character class - controls the player and the individual vampires

package dungeon;

import java.util.Objects;

public class Character {
    private Point point;
    private Point previousPoint;
    private String symbol;
    private MovementKey movementKey;

    public Character(String symbol) {
        this.symbol = symbol;
        this.point = new Point();
        this.previousPoint = new Point();
        this.movementKey = new MovementKey();
    }

    public String symbol() {
        return symbol;
    }

    public Point point() {
        return point;
    }

    public MovementKey movementKey() {
        return movementKey;
    }

    public boolean hasMoved() {
        return !point.equals(previousPoint);
    }

    public boolean isAtThisPosition(Point other) {
        return point.equals(other);
    }

    public void rewindMove() {
        point = previousPoint;
    }

    public void setPoint(Point newPoint) {
        point = newPoint;
    }

    public void move(String command, Dungeon dungeon)
            throws IllegalArgumentException {
        previousPoint = new Point(point);
        try {
            switch (movementKey.getDirection(command)) {
                case UP:
                    point.setRelativeY(-1);
                    break;
                case LEFT:
                    point.setRelativeX(-1);
                    break;
                case DOWN:
                    point.setRelativeY(1);
                    break;
                case RIGHT:
                    point.setRelativeX(1);
                    break;
                default:
                    System.out.println("Invalid command!");
            }
            if (!dungeon.isInBoundary(point)) {
                rewindMove();
            }
        }
        catch (NullPointerException e) {
            throw new IllegalArgumentException("Illegal Command : " + command);
        }
    }
    public String coordinates() {
        return String.format("%s %d %d", symbol, point.x(), point.y());
    }

    @Override
    public String toString() {
        return symbol;
    }

    @Override
    public boolean equals(Object object) {
        if (this == object) {
            return true;
        }
        if (object == null) {
            return false;
        }
        if (getClass() != object.getClass()) {
            return false;
        }
        Character other = (Character) object;

        return Objects.equals(symbol, other.symbol)
                && point.equals(other.point);
    }
}

Vampire Group : controls the actions of the vampires as a group (composition over inheritance)

package dungeon;


import java.util.*;

public class VampireGroup {
    private List<Character> vampires;
    private boolean doesMove;

    public VampireGroup(int count, boolean doesMove) {
        this.vampires = new ArrayList<>();
        for (int i = 0; i < count; i++) {
            this.vampires.add(new Character("v"));
        }
        this.doesMove = doesMove;
    }

    public boolean areAshes() {
        return vampires.isEmpty();
    }

    public String symbol() {
        return vampires.get(0).symbol();
    }

    public Set<Point> getPoints() {
        Set<Point> points = new HashSet<>();
        for (Character vampire : this.vampires) {
            points.add(vampire.point());
        }
        return points;
    }

    public Character vampireAtPosition(Point point) {
        for (Character vampire : vampires) {
            if (vampire.isAtThisPosition(point)) {
                return vampire;
            }
        }
        return null;
    }

    public void removeAshes(Point playerPoint) {
        // do nothing if no vampires are found
        vampires.remove(vampireAtPosition(playerPoint));
    }

    public void setInitialCoordinates(int length, int height, 
                                      Set<Point> usedPoints) {
        for (Character vampire : vampires) {
            usedPoints.remove(vampire.point());
            setToRandomCoordinate(length, height, vampire, getPoints());
            usedPoints.add(vampire.point());
        }
    }

    public void setToRandomCoordinate(int length, 
                                      int height, 
                                      Character vampire, 
                                      Set<Point> usedPoints) {
        Point newPoint;
        do {
            newPoint = Point.randomPoint(length, height);
        }
        while (usedPoints.contains(newPoint));
        vampire.setPoint(newPoint);
    }

    public void moveRandomly(Dungeon dungeon, Character vampire, 
                             Set<Point> usedPoints) {
        String key = vampire.movementKey().getRandomKey();
        vampire.move(key, dungeon);
        if (usedPoints.contains(vampire.point())) {
            vampire.rewindMove();
            moveRandomly(dungeon, vampire, usedPoints);
        }
    }

    public void move(Dungeon dungeon) {
        if (!doesMove) {
            return;
        }
        for (Character vampire : vampires) {
            do {
                moveRandomly(dungeon, vampire, dungeon.usedPoints());
            }
            while (!vampire.hasMoved());
        }
    }

    public String coordinates() {
        StringBuilder stringBuilder = new StringBuilder();
        for (Character vampire : this.vampires) {
            stringBuilder.append(String.format("%s\n", vampire.coordinates()));
        }
        return stringBuilder.toString();
    }
}

Dungeon class - draws grid, controls entities in the dungeon (player and vampires)

package dungeon;


import java.util.HashSet;
import java.util.Set;

public class Dungeon {
    private int length;
    private int height;
    private int lampCount;
    private Character player;
    private VampireGroup vampires;

    public Dungeon(int length, int height, int lampCount) {
        this.length = length;
        this.height = height;
        this.lampCount = lampCount;
        this.player = new Character("@");
        this.vampires = new VampireGroup(2, true);
        this.vampires.setInitialCoordinates(length, height, usedPoints());
    }

    public void printResults() {
        if (vampires.areAshes()) {
            System.out.println("YOU WIN");
        }
        else {
            System.out.println("YOU LOSE");
        }
    }

    public boolean isNotClearedWhenPlayerIsAlive() {
        return lampCount > 0 && !vampires.areAshes();
    }


    public Set<Point> usedPoints() {
        Set<Point> usedPoints = new HashSet<>();
        usedPoints.add(player.point());
        usedPoints.addAll(vampires.getPoints());
        return usedPoints;
    }

    public void printStats() {
        System.out.println(lampCount);
        System.out.println();

        printCoordinates();
        System.out.println();

        printGrid(player, vampires, usedPoints());
        System.out.println();
    }

    public void playersTurn(String key) throws IllegalArgumentException {
        player.move(key, this);
    }

    public void vampiresTurn() {
        vampires.removeAshes(player.point());
        vampires.move(this);
    }

    public void printCoordinates() {
        System.out.println(player.coordinates());
        System.out.println(vampires.coordinates());
    }

    public boolean isInBoundary(Point point) {
        return  isInRange(point.x(), length)
                && isInRange(point.y(), height);
    }

    public void decrementLampCount() {
        if (player.hasMoved()) {
            lampCount--;
        }
    }

    public boolean isInRange(int value, int min, int max) {
        return value >= min && value < max;
    }

    public boolean isInRange(int value, int max) {
        return isInRange(value, 0, max);
    }

    public void printGrid(Character player, VampireGroup vampires, Set<Point> usedPoints) {
        for (int i = 0; i < height; i++) {
            for (int j = 0; j < length; j++) {
                Point currentPoint = new Point(j, i);
                if (usedPoints.contains(currentPoint)) {
                    if (player.isAtThisPosition(currentPoint)) {
                        System.out.print(player);
                    }
                    else {
                        System.out.print(vampires.symbol());
                    }
                }
                else {
                    System.out.print('.');
                }
            }
            System.out.println();
        }
    }
}

Point class - since every character in the dungeon is a point in a 2D grid

package dungeon;

import useful.Randomizer;

import java.util.Objects;

public class Point {
    private static Randomizer randomizer;
    private int x;
    private int y;

    public Point(int x, int y) {
        setX(x);
        setY(y);
        randomizer = new Randomizer();
    }

    public Point() {
        this(0, 0);
    }

    public Point(Point other) {
        this(other.x, other.y);
    }

    public static Point randomPoint(int xMin, int xMax, int yMin, int yMax) {
        return new Point(randomizer.random(xMin, xMax),
                randomizer.random(yMin, yMax));
    }

    public static Point randomPoint(int xMax, int yMax) {
        return randomPoint(0, xMax - 1, 0, yMax - 1);
    }

    public int y() {
        return y;
    }

    public int x() {
        return x;
    }

    public void setX(int dx) {
        x = dx;
    }

    public void setY(int dy) {
        y = dy;
    }

    public void setRelativeX(int dx) { setX(x + dx);
    }

    public void setRelativeY(int dy) {
        setY(y + dy);
    }

    public boolean equals(Object object) {
        if (this == object) {
            return true;
        }
        if (object == null) {
            return false;
        }
        if (getClass() != object.getClass()) {
            return false;
        }
        Point other = (Point) object;
        return x == other.x
                && y == other.y;
    }

    public int hashCode() {
        return Objects.hash(x, y);
    }

    public String toString() {
        return String.format("(%d, %d)", x, y);
    }
}

Randomizing class - Helper class to simplify Random class

package useful;

import java.util.Random;

public class Randomizer extends Random {
    //inclusive
    public int random(int min, int max) {
        Random random = new Random();
        return random.nextInt(max - min + 1) + min;
    }

    public double randomDouble(double min, int max) {
        Random random = new Random();
        return random.nextDouble()*(max - min) + min;
    }

    //exclusive
    public int random(int n) {
        return random(0, n - 1);
    }
}

BiMap class - Helper class used to create |Direction <---> Key| relation.

package useful;


import java.util.HashMap;
import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;

public class BiMap<K, V> extends HashMap<K, V> {
    private Map<V, K> inversedMap = new HashMap<>();

    public K getKey(V value) {
        return inversedMap.get(value);
    }

    @Override
    public V put(K key, V value) {
        inversedMap.put(value, key);
        return super.put(key, value);
    }

    @Override
    public void putAll(Map<? extends K, ? extends V> m) {
        super.putAll(m);
        for (K key : m.keySet()) {
            inversedMap.put(super.get(key), key);
        }
    }

    @Override
    public V remove(Object key) {
        inversedMap.remove(super.get(key));
        return super.remove(key);
    }

    @Override
    public void clear() {
        super.clear();
        inversedMap.clear();
    }

    @Override
    public V putIfAbsent(K key, V value) {
        inversedMap.putIfAbsent(value, key);
        return super.putIfAbsent(key, value);
    }

    @Override
    public boolean remove(Object key, Object value) {
        inversedMap.remove(value, key);
        return super.remove(key, value);
    }

    @Override
    public boolean replace(K key, V oldValue, V newValue) {
        inversedMap.remove(oldValue);
        inversedMap.put(newValue, key);
        return super.replace(key, oldValue, newValue);
    }

    @Override
    public V replace(K key, V value) {
        if (super.containsKey(key)) {
            put(key, value);
        }
        return value;
    }

    @Override
    public V computeIfAbsent(K key,
                             Function<? super K, ? extends V> mappingFunction) {
        V value = super.computeIfAbsent(key, mappingFunction);
        inversedMap.put(value, key);
        return value;
    }

    @Override
    public V computeIfPresent(K key,
                              BiFunction<? super K,? super V,? extends V> remappingFunction) {
        V value = super.computeIfPresent(key, remappingFunction);
        inversedMap.put(value, key);
        return value;
    }

    @Override
    public V compute(K key,
                     BiFunction<? super K,? super V,? extends V> remappingFunction) {
        V value = super.compute(key, remappingFunction);
        inversedMap.put(value, key);
        return value;
    }

    @Override
    public V merge(K key,
                   V value,
                   BiFunction<? super V,? super V,? extends V> remappingFunction) {
        V replacedValue = super.merge(key, value, remappingFunction);
        inversedMap.remove(value);
        inversedMap.put(replacedValue, key);
        return replacedValue;
    }

    @Override
    public void replaceAll(BiFunction<? super K,? super V,? extends V> function) {
        inversedMap.clear();
        super.replaceAll(function);
        for (K key : super.keySet()) {
            inversedMap.put(super.get(key), key);
        }
    }
}

UserInterface class - deals with the commands given by user and executes them

package dungeon;

import java.util.*;

public class UserInterface implements AutoCloseable {
    private Scanner scanner;
    private Dungeon dungeon;

    public UserInterface() {
        this.scanner = new Scanner(System.in);
        this.dungeon = new Dungeon(5, 5, 14);
    }

    public void executeCommands(String multipleCommands)
            throws IllegalArgumentException {
        for (char command : multipleCommands.toCharArray()) {
            dungeon.playersTurn(String.valueOf(command));
        }
        dungeon.vampiresTurn();
        dungeon.decrementLampCount();
    }

    public void run() {
        while (dungeon.isNotClearedWhenPlayerIsAlive()) {
            dungeon.printStats();
            if (scanner.hasNext()) {
                String commandSequence = scanner.next();
                try {
                    executeCommands(commandSequence);
                }
                catch (IllegalArgumentException e) {
                    System.out.println(e);
                }
            }
            System.out.println("--------------------------");
        }
        dungeon.printResults();
    }

    @Override
    public void close() {
        scanner.close();
    }
}

Main class

import dungeon.UserInterface;

public class Main {
    public static void main(String[] args) {
        try (UserInterface ui = new UserInterface()) {
            ui.run();
        }
    }
}

Expected Output

14

@ 0 0
v 1 2
v 7 8
v 7 5
v 8 0
v 2 9

@.......v.
..........
.v........
..........
..........
.......v..
..........
..........
.......v..
..v.......

ssd
--------------------------
13

@ 1 2
v 8 8
v 7 4
v 8 3
v 1 8

..........
..........
.@........
........v.
.......v..
..........
..........
..........
.v......v.
..........

ssss
--------------------------
12

@ 1 6
v 6 9
v 6 5
v 8 3

..........
..........
..........
........v.
..........
......v...
.@........
..........
..........
......v...

dd
--------------------------
11

@ 3 6
v 5 9
v 6 7
v 8 1

..........
........v.
..........
..........
..........
..........
...@......
......v...
..........
.....v....

ddds
--------------------------
10

@ 6 7
v 6 6
v 5 0

.....v....
..........
..........
..........
..........
..........
......v...
......@...
..........
..........

w
--------------------------
9

@ 6 6
v 4 0

....v.....
..........
..........
..........
..........
..........
......@...
..........
..........
..........

www
--------------------------
8

@ 6 3
v 4 0

....v.....
..........
..........
......@...
..........
..........
..........
..........
..........
..........

aa
--------------------------
7

@ 4 3
v 4 2

..........
..........
....v.....
....@.....
..........
..........
..........
..........
..........
..........

w
YOU WIN

Answer 1

Here are my comments:

inheritance

composition over inheritance is usually a good principle to follow. However, inheritance has the advantage that the compiler is aware of it and can be used to alert in case of errors. specifically, I saw that in VampireGroup, you declared private List<Character> vampires;. So theoretically, a player character can be placed in this list. This is where it would be beneficial to have Player and Vampire classes that inherit from Character. with this you could declare private List<Vampire> vampires; which is more clear and also makes the compiler guard from accidental addition of Player character. the other benefit is that you can further customize player and vampires separately (perhaps they can define their own symbol? more on that later).

Direction enum

this was already mentioned by @glissi. I will add further: in Character.move(), you have a switch statement with case for every movement. the key binding itself is defined in the constructor of MovementKey. however, all these pieces of logic and data are tied together. What if you decide to expand your game to allow for diagonal (or other) movements? you will need to make code change in all the above places (possibly more that I didn't see). in CS, this is called Single Responsibility Principle. One place that is responsible for all logic and data pertaining to an isolated functionality.

If you wrap your head on how to have the character movement in the enum: you can have a Function argument to every enum value. The Function interface defines a method that takes one argument and returns a value. in your case, it will receive a Point of current position and return a new Point (can be same instance) in new position. Alternatively, you can have a Consumer that returns void (more an that later). the actual body of the functions can be specified as lambda expressions

enum Direction {
    UP((point -> {point.setRelativeY(-1); return point;})),
    LEFT((point -> {point.setRelativeX(-1); return point;})),
    ...
    public Function<Point, Point> move;
    Direction(Function<Point, Point> move) {
        this.move = move;
    }
}

Immutability

Point class is mutable. it has setter methods that allow callers to alter the state (modify instance variables' values). In contrast, if you remove the setter methods of Point. it becomes immutable. if you want to move a character, you don't alter its point. instead, you assign a new point. This is similar to the immutable java.lang.String. There are several advantages to immutable objects: they can be better tested, are far better suited to be used in multi-threaded system and can be used by Java 8 lambda expressions (that require final objects only). All these cases are perhaps out of scope or too advanced for this exercise. however, in terms of general good practices, this principle is well known. That is why I prefer to use Function over Consumer for movement operation in the enum.

Separation of Concerns

This CS principle can be viewed as a higher hierarchy variant of the Single Responsibility principle. You have a UserInterface class that is supposed to be the (only) place that both takes user input and displays data. Yet, there are several places that affect the output that is displayed on the console: the dungeon prints its own grid, and is responsible for the symbols of the player and of an empty point. VampireGroup decides the symbol of a vampire and MovementKey hard codes key binding.

now what if you wanted to create an HTML version of your game that is viewed online? what if you wanted to allow users to modify key binding? with Separation of Concerns, you should create a "black box" game engine that has its own api.this api will have a getDungeun() method that will return a string representation of the dungeon. each user interface will know how to read this string representation and display to the user (with possibly different symbols). the string representation can be custom-made, which requires custom-made parser, or use one of the industry standard formats, like XML, JSON where you have ready made parsers.

Answer 2

First of all, I think your code is not bad at all. The separation in classes and their respective use is quite okay.

Nevertheless, I have a couple of suggestions that might make your code even better:

Try to make things as unsurprising as possible

• Naming one of your classes Character is likely to send people on a wrong track, as Character is the name of Java's wrapper object class for char primitive variables.

• It's a good idea to adhere to naming conventions:
  Methods, which only return a class attribute ("getters") should be named get<Attribute>, i.e. getPoint, getPreviousPoint, getSymbol, getX, getY, etc. (you have already done it right for the "setters": setX, setY, etc)

Try to keep things as local and as static as possible

• Can you implement the classes, that are instantiated exactly once during the lifetime of your application in a way, that leverages that?
  Here, the possible candidates UserInterface, Scanner and Dungeon hold the status of your game, so static is out of the question. (You could consider using a Singleton for these, if you wanted to)
• Methods, that are used only locally should be declared private
  e.g. noOfKeys() and getRandomDirection(), which are both used exclusively from within MovementKey.
  (I would even include the functionality of those methods in getRandomKey() and erase them altogether).

Try to use existing mechanisms

To store the mapping from Direction to their respective keys (and vice versa), you create a new bidirectional Map BiMap, and use that in MovementKey.
In my Opinion, this could be more elegantly implemented by augmenting the enum Direction by an attribute which holds the keys, a getter for it and a couple of convenience methods (getRandomKey() ...)

This would make BiMap and MovementKey unnecessary.