A Mess of Missions

I've mostly completed the prototype for my city building simulation, and now I have added the meta-game mission system to the game. You see these mission systems in a great many games these days. The basic idea is that the player is assigned a mission at the start of the game, such as Build 50 Industrial buildings, and once completed gains a permanent bonus that will apply in all future games.

Programming this turned out to be pretty complicated (at least as far as the number of classes goes) so I think a review of the code will be helpful. I have an abstract MayorMission class, and then a few different implementations of that depending on the mission requirements. Some of them take a value for the type of building required to complete the mission, or for example the type of zone, or number of people or money required. I feel like multiple classes here is a good idea because it simplifies the code for checking whether missions are completed, but I wonder if I have taken things too far.

MayorMission.java

public abstract class MayorMission {

    public final int targetNumber;
    public final World world;
    public final String name;
    public final PlayerAbility reward;

    public boolean isComplete;

    public MayorMission(int targetNumber, World world, String name, PlayerAbility reward) {
        this.targetNumber = targetNumber;
        this.world = world;
        this.name = name;
        this.reward = reward;
    }

    public abstract boolean checkIfComplete();
}

IsoTileTypeMission.java

```
public class IsoTileTypeMission extends MayorMission {

private final IsoTileType typeRequired;

public IsoTileTypeMission(int targetNumber, World world, IsoTileType typeRequired, String name, PlayerAbility reward) {
super(targetNumber, world, name, reward);
this.typeRequired = typeRequired;
}

@Override
public boolean checkIfComplete() {
int count = 0;
for (Tile[] row : this.world.levelData) {

There's several things to address here, so let's get to it!

Classes

Too many.

Speed

Can be better.

Declaring missions

Perfect for a config file.

checkIfMissionComplete

Slight code duplication.

Classes

BEEP BEEP BEEP Strategy pattern!

This bothers me a bit:

super(targetNumber, world, name, reward);

Instead of having that, I would use a MayorMission constructor like this:

public MayorMission(int targetNumber, World world, String name,
    PlayerAbility reward, GameGoal goal)

Where GameGoal is something like:

public interface GameGoal {
     boolean checkIfComplete();
}

This reduces the need to subclass MayorMission and let's you use composition over inheritance. This might also lead to the possibility of removing some of the arguments from the constructor.

The different GameGoal objects can then either be implemented as pure classes, or constructed using factory methods and anonymous inner classes.

So instead of:

missions.add(new ZoneTypeMission(50, this.world, 
     ZoneType.getResidentialZones(), "Build 50 Residential", 
     PlayerAbility.UNLOCK_RES_1));

We might end up with:

missions.add(new MayorMission(this.world, "Build 50 Residential", 
     PlayerAbility.UNLOCK_RES_1,
     zoneTypeGoal(50, ZoneType.getResidentialZones())));

Or how about:

missions.add(world.createMission("Build 50 Residential",
     PlayerAbility.UNLOCK_RES_1, 
     zoneTypeGoal(50, ZoneType.getResidentialZones()));

Generic tile scanning

Many of your mission goals depends on a sum of all tiles (when purely counting, the value for each tile matching the predicate is 1)

This leads to a possibilty of:

public class CountingGoal implements GameGoal {
    private final ToIntFunction tileValue;

    @Override
    public boolean checkIfComplete() {
        int sum = 0;
        for (Tile[] row : world.levelData) {
            for (Tile tile : row) {
                sum += tileValue.applyAsInt(tile);
            }
        }
        return sum >= targetNumber;
    }
}

ToIntFunction is an interface that exists in Java 8, but as you're not using Java 8 you have to declare it yourself:

public interface ToIntFunction {
     int applyAsInt(T obj);
}

This will allow you to extract implementations, such as:

new ToIntFunction() {
    @Override
    public int applyAsInt(Tile tile) {
        return tile.getBuildingType() == typeRequired ? 1 : 0;
    }
}

Or, the class version of that:

class BuildingTypeCounting implements ToIntFunction {
    private final BuildingType typeRequired;

    @Override
    public int applyAsInt(Tile tile) {
        return tile.getBuildingType() == typeRequired ? 1 : 0;
    }
}

Speed

Looping over each and every tile in the game is a bit slow. You might want to consider keeping track of the zones that actually have some content on them. You can do this by using a List residential, or perhaps even Map> tilesByType, or if there are generally a lot of empty tiles, perhaps List tilesWithBuildings would be enough.

You might want to consider splitting your world data structure so that general tile data is kept as a two-dimensional array, and objects that do something are kept in a List.

checkIfMissionComplete

private void checkIfMissionComplete() {
    if (this.game.getPlayer().assignedMission.checkIfComplete()) {
        this.game.getPlayer().assignedMission.isComplete = true;
    }
}

It feels like it is the mission's job to mark itself as completed, and then it may return true if it did mark it as completed.

private void checkIfMissionComplete() {
    this.game.getPlayer().assignedMission.markIfCompleted();
}

And in the MayorMission:

public boolean markIfCompleted() {
    boolean completed = this.checkIfComplete();
    if (completed) {
        this.isComplete = true;
    }
    return completed;
}

StackExchange Code Review Q#93265, answer score: 7