5
\$\begingroup\$

My programs that I have written so far included only a few lines of source code. When I tried to write larger programs, it usually failed because I'm not a master of software engineering. This time, I want to play it safe and publish now from time to time parts of the program, which should be my first, successful major project.

I intend to write a textual role playing game.

In my role playing game, the environment should be represented as a two-dimensional field. All surrounding objects, such as houses, natural objects and NPCs should be represented as points on the field. With special commands you will also be able to see your inventory, stats etc. but that will come later.

So I've written a field class and a base class for all the entities that can appear on the field. It was a little bit more complex than I expected. I'd like to know if these two classes can be used as a starting point for representing the environment objects of the game, or if my code is inappropriate and should be improved, or even a completely different concept should be implemented.

Entity.java

public class Entity {
    public String name;
    public char symbol;
    public boolean walkable;
    // decides if entity can appear on several positions simultaneously
    public boolean multifarious;

    public Entity(String name, char symbol, boolean walkable) {
        this.name = name;
        this.symbol = symbol;
        this.walkable = walkable;
        this.multifarious = false;
    }

    public Entity(String name, char symbol, boolean walkable, boolean multifarious) {
        this(name, symbol, walkable);
        this.multifarious = multifarious;
    }

    public boolean isWalkable() {
        return walkable;
    }

    public boolean isMultifarious() {
        return multifarious;
    }

    public String getName() {
        return name;
    }

    public char getSymbol() {
        return symbol;
    }

    @Override
    public String toString() {
        return name + ", " + symbol + ", walkable: " + walkable;
    }
}

Field.java

import java.util.List;
import java.util.ArrayList;

public class Field {
    private int height;
    private int width;
    private List<List<List<Entity>>> positions;
    private boolean multipleEntitiesOnPosition;
    private char emptyPositionRepresentation;
    private List<Entity> placedEntities;

    public Field(int height, int width, boolean multipleEntitiesOnPosition) {
        this.height = height;
        this.width = width;

        positions = new ArrayList<List<List<Entity>>>();
        for (int i = 0; i < height; i++) {
            positions.add(new ArrayList<List<Entity>>());
            for (int j = 0; j < width; j++) {
                positions.get(i).add(new ArrayList<Entity>());
            }
        }

        this.multipleEntitiesOnPosition = multipleEntitiesOnPosition;
        emptyPositionRepresentation = ' ';
        placedEntities = new ArrayList<Entity>();
    }

    public Field(int height, int width, boolean multipleEntitiesOnPosition, char emptyPositionRepresentation) {
        this(height, width, multipleEntitiesOnPosition);
        this.emptyPositionRepresentation = emptyPositionRepresentation;
    }

    public int getHeight() {
        return height;
    }

    public int getWidth() {
        return width;
    }

    public void addEntity(Entity entity, int x, int y) {
        // check if coordinates are valid
        if (x >= height) {
            System.err.println("error: x is greater than height");
            return;
        }
        if (y >= width) {
            System.err.println("error: y is greater than width");
            return;
        }
        if (x < 0 || y < 0) {
            System.err.println("error: negative positions argument(s)");
            return;
        }

        // check if entity is allready on field
        if (placedEntities.contains(entity) && !entity.isMultifarious()) {
            System.err.println("error: entity is allready on field");
            return;
        }

        List<Entity> entityList = positions.get(x).get(y);

        // check if entity is allready on that position
        if (entityList.contains(entity)) {
            System.err.println("error: entity is allready on that position");
            return;
        }

        // check if entity is allready placed on position
        if (!multipleEntitiesOnPosition && !entityList.isEmpty()) {
            System.err.println("error: multiple entities on same position are forbidden");
            return;
        }

        // check if entity gets placed on another entity that is not walkable
        for (Entity ent : entityList) {
            if (!ent.isWalkable()) {
                System.err.println("error: entity can not be placed on an entity that is not walkable (" + entity + ")");
                return;
            }
        }

        placedEntities.add(entity);
        entityList.add(entity);
    }

    public void removeEntity(Entity entity) {
        if (!placedEntities.contains(entity)) {
            System.err.println("This entity is not on the field.");
            return;
        }

        for (int i = 0; i < height; i++) {
            for (int j = 0; j < width; j++) {
                List<Entity> entities = positions.get(i).get(j);
                if (entities.contains(entity)) {
                    entities.remove(entity);
                    if (!entity.isMultifarious()) {
                        return;
                    } 
                }
            }
        }
    }

    public String toString() {
        StringBuilder returnValue = new StringBuilder();
        for (int i = 0; i < height; i++) {
            for (int j = 0; j < width; j++) {
                List<Entity> entities = positions.get(i).get(j);
                if (!entities.isEmpty()) {
                    // only the last entity of a list is drawed on the field
                    char lastSymbol = entities.get(entities.size() - 1).getSymbol();
                    returnValue.append(lastSymbol);
                } else {
                    returnValue.append(emptyPositionRepresentation);
                }
            }
            returnValue.append('\n');
        }
        return returnValue.toString();
    }
}

Main.java (Test class)

// This class tests the functionality of "Field" and "Entity"
public class Main {
    public static void main(String[] args) {
        // first attempt
        Field field = new Field(20, 40, true, '.');
        Entity player = new Entity("Player 1", '#', true);
        field.addEntity(player, 10, 20);
        System.out.println(field.toString());
    }
}
\$\endgroup\$
  • \$\begingroup\$ Not a review but a suggestion: add a class that manages "all the entities", and a factory method that can take a 2-d "picture" of a field and create the field with entities. (That is, something you can pass a string 2d map picture, and it builds the corresponding data structure.) \$\endgroup\$ – Austin Hastings May 2 at 3:05
  • \$\begingroup\$ Yes, this will be essential for later use! This is also already planed :) \$\endgroup\$ – Dexter Thorn May 2 at 7:49
1
\$\begingroup\$

I'm going through your code from top to bottom.

Entity

  • In your current code, an entity does not change after it has been constructed. Therefore all its fields should be marked as final, for example:

    private final String name;
    
  • Making the fields final will produce a compile error in one of the constructors. Currently, the constructor with more arguments calls the constructor with fewer arguments. It should be the other way round: the simple constructors should call the more complete ones:

    public Entity(String name, char symbol, boolean walkable) {
        this(name, symbol, walkable, false);
    }
    
    public Entity(String name, char symbol, boolean walkable, boolean multifarious) {
        this.name = name;
        this.symbol = symbol;
        this.walkable = walkable;
        this.multifarious = false;
    }
    
  • The field name walkable sounds strange. I first thought you meant canWalk, but that guess was wrong. To avoid this confusion, you should add a short comment:

    // If true, other entities can walk on this entity.
    
  • In the toString method, it might look more natural to say:

    public String toString() {
        String walkableStr = walkable ? "walkable" : "not walkable";
        return name + ", " + symbol + ", " + walkableStr;
    }
    

Field

My first thought was: what the hell is a List<List<List<Entity>>>, and is it really intended? Then I saw that it is indeed needed. It just looks really complicated and overdesigned at first.

To avoid this first impression, it helps when you add a comment to this variable:

// positions[x][y] lists the entities at that cell.

This comment has the added benefit of clearly saying whether x or y comes first. While the order x, y is most common, there's enough code out there that uses y, x or row, col. Therefore readers of your code cannot be sure until they see the actual code.

Typo: allready should be already.

If you have lots of entities, List.contains will become slow because it searches the complete list from the beginning until the end. Instead you should use a HashSet<Entity> instead of the List<Entity> since Set.contains is faster.

No part of your program should write directly to System.out or System.err. If your game becomes successful one day, you probably want to port it to Android, iOS, Windows, Linux, other platforms. Not all of them have System.out, and they differ in how they accept input from the user. Therefore it's a good idea to define a UserInterface for everything related to user interaction:

interface UserInterface {

    void inform(String message);

    /* The user tried to make a mistake, such as placing an entity 
     * on a cell where there already is another entity. */
    void error(String message);

    String ask(String prompt);

    /* An internal programming error has happened. This is not the user's fault. */
    void programmingError(String message);
}

This can be used for talking to the user. The last method in that interface is when you as the programmer made a mistake, such as passing negative coordinates. You may still want to inform the user, but the user cannot do anything about the error anyway. An entirely different case is when the user tries to place an entity on an invalid location. Currently you handle these types of errors in the same way, but you shouldn't.

The removeEntity method is unused.

In removeEntity you forgot to remove the entity from placedEntities. Watch out when removing a multifarious entity that is on the field in several locations. It's probably better to use a Multiset (from Google Guava) or a Map<Entity, Integer> to count how often each entity is on the field.

To make adding or removing entities faster, you should store them in a different way, in order to avoid the 2-dimensional for loop in removeEntity. It's better to remember for each entity at which positions it is. Again, Google Guava is a great library that contains a Table class and a Multimap for exactly this purpose.

While I'm here, by using Guava's Multimap you could also replace your List<List<List<Entity>>> with a Multimap<Location, Entity>, which explains more directly what the field really contains.

In Field.toString you mention:

// only the last entity of a list is drawed on the field

This comment is deeply hidden. It should be placed right at the positions field. The whole positions field might then be:

/**
 * Stores the entities that are currently at the given location on the field.
 * Entities that are added later are placed "on top" of the previous entities.
 * All but the topmost entity must be walkable.
 */
private final ListMultimap<Location, Entity> entities = ArrayListMultimap.create();

Main

The main class is short and to the point. Very nice.

In addition to the main class, it would be good to have some unit tests for the Entity and Field classes. It's easy to automatically test for invalid placements:

@Test
public void entity_cannot_be_placed_twice() {
    Field field = new Field(5, 5, true, ' ');
    Entity player = new Entity("Player", '#', false);
    field.addEntity(player, 2, 2);

    try {
        field.addEntity(player, 2, 2);
        fail();
    } catch (IllegalArgumentException e) {
        assertEquals("the entity is already on the board", e.getMessage());
    }

    try {
        field.addEntity(player, 0, 1);
        fail();
    } catch (IllegalArgumentException e) {
        assertEquals("the entity is already on the board", e.getMessage());
    }
}

As soon as you have these automatic tests and run them regularly, you don't have to worry about accidentally breaking code that used to work before.

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

1: You are declaring your variables public and creating a getter in Entity

 public String name;
 public String getName() {
    return name;
}

2: You should not couple walkable and non-walkable objects together.

public interface Walkable {

void walk();

}

3: private List<List<List<Entity>>> positions;
You can create a two-dimensional array

Entity[][] EntityArray= new Entity[height][width];

and get entity at x and y on that array example

Entity entity = entityArray[x][y]

If null is given that point is empty.

With special commands you will also be able to see your inventory, stats etc. but that will come later. If you want this functionality I highly suggest you make an abstract class like below

public abstract class EntityExample {

private int x, y;
private char symbol;
private String name;

public EntityExample(int x, int y, char symbol, String name) {
    this.x = x;
    this.y = y;
    this.symbol = symbol;
    this.name = name;
}

public String getName() {
    return name;
}

public char getSymbol() {
    return symbol;
}

@Override
public String toString() {
    return name + ", " + symbol;
}}

all surrounding objects, houses, natural objects, and NPCs should extend this class as this is the bare minimum the Field object needs to display them.

Then you should create a more abstract class that extends EntityExample for controllable characters, for example, NPC, etc.

If this is your first big project You should create a UML and plan your OOP concepts in a way that follows Solid Principles

\$\endgroup\$
  • \$\begingroup\$ Oops, normally I make the members private, why I forgot that... \$\endgroup\$ – Dexter Thorn Apr 29 at 20:04
  • \$\begingroup\$ No problem it happens Do you have any questions =) \$\endgroup\$ – Adam Sever Apr 29 at 20:16
0
\$\begingroup\$

I agree with Adam, that you shouldn't use a List of Lists of Lists internally. An ArrayList is useful, if you need to change the number of elements, but for a fixed size as here, it would be much easier to use a two-dimensional array. Using an ArrayList for multiple entities at a position is fine for now (*), because the number of entites change.

(*) Later it may be more performant to use something else.

private List<Entity>[][] positions;

public Field(int height, int width, boolean multipleEntitiesOnPosition) {
    this.height = height;
    this.width = width;

    positions = new List<Entity>[height][width];

    // ...
}

Unless you know you'll have at least one entity at almost every position, it also make sense not to initalize the List of entites until needed:

public void addEntity(Entity entity, int x, int y) {

   // ...

   if (positions[y][x] == null) {
     positions[y][x] = new ArrayList<Entity>();
   }

   // ...

   positions[y][x].add(entity);

   //

}

The biggest problem currently is your use of System.err to write error messages. If this is just placeholder, then that would make your question off topic, so for now I'll assume that it's "real" :)

Checking validity of the coordinates is pointless here. If the given coordinates are wrong, then you have a problem in the rest of the code anyway, so you just as well can have it run into an IndexOutOfBoundsException.

And in the other error cases there is no way for the calling code to know what went wrong, so instead of writing a pointless error message return an error code.

\$\endgroup\$
  • \$\begingroup\$ That was a typo on my side. I've corrected it, but you shouldn't be using either :-) \$\endgroup\$ – RoToRa May 2 at 7:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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