Removing redundancy from an immutable "rules class"

Question

I am writing a board game program in Java. Slightly similar to chess or civilization in that each player has a set of units, and each unit has certain actions that it can take.

The base GameAction class has two methods: One which checks to make sure the action is allowed, and one to actually do the action. For example, MoveAction's checkValidity() checks to see if the target is in range, and returns true if it is and false if it isn't, and then doTask() has the precondition that checkValidity() must return true for the same arguments... hopefully that makes sense.

However, these GameAction rules are implemented internally as stateless immutable classes so that every unit can use the same ones, to make parsing the config file easier and to make changing the actions that a given type can perform on the fly easier. For example, if I want to give all cavalry a new ability, all I have to do is change it in one place instead of finding every existing cavalry unit and adding it.

This leads to some serious code duplication though, between checkValidity() and doTask()... here is an example:

package elipsis.aow.actions;

import elipsis.aow.Board;
import elipsis.aow.GameAction;
import elipsis.aow.HexCell;
import elipsis.aow.HexCoordinate;
import elipsis.aow.Player;
import elipsis.aow.Resource;
import elipsis.aow.Unit;
import elipsis.aow.UnitStack;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

/**
 * Causes a unit to be built (either creating a new one altogether, or just
 * incrementing an existing unitstack by one, and then reflecting that creation
 * in the owner's census data.
 * @author scott
 */
public class BuildUnitAction extends GameAction {

    private Unit toBuild;

    public BuildUnitAction(Unit unit) {
        super(String.format("Build %s", unit.getName()));
        this.toBuild = unit;
    }

    /**
     * Check for validity of the action.
     * 
     * In the case of a build unit action:
     * <p>The owner must have the requisite resources.</p>
     * <p>The cells must be on top of or directly adjacent to the structure building.</p>
     * <p>The target cell must contain either no unit or a unitstack owned by the
     * current owner whose type is the same.</p>
     * @param owner
     * @param b
     * @param begin
     * @param end
     * @return 
     */
    @Override
    public boolean checkValidity(Player owner, Board b, HexCoordinate begin, HexCoordinate end) {

        if (!owner.hasResourcesFor(toBuild.getBuildCosts())) {
            return false;
        }

        HexCell start = b.getCell(begin);
        HexCell stop = b.getCell(end);

        if (!begin.equals(end) && !start.getAdjacent().contains(end)) {
            return false;
        }

        UnitStack existent = stop.getUnit();

        if (existent != null) {
            if (!existent.getType().equals(toBuild) || 
                !existent.getOwner().equals(owner)) {
                return false;
            }
        }

        return true;
    }

    @Override
    public Set<HexCoordinate> doTask(Player owner, Board b, HexCoordinate begin, HexCoordinate end) {
        Set<HexCoordinate> output = new HashSet<>(Arrays.asList(end));

        HexCell hcStop = b.getCell(end);

        UnitStack usOld = hcStop.getUnit();
        Map<String, ActionFlags> logOld;
        int strOld;

        if (usOld == null) {
            logOld = null;
            strOld = 0;
        } else {
            logOld = usOld.getActionLog();
            strOld = usOld.getStrength();
        }

        UnitStack usNew = new UnitStack(toBuild, strOld + 1, owner);
        if (logOld != null) {
            usNew.getActionLog().putAll(logOld);
        }

        hcStop.setUnit(usNew);

        owner.changeArmySize(toBuild, 1);

        // Find the cost and charge the owner.
        Map<Resource, Integer> buildCost = toBuild.getBuildCosts();
        owner.chargeBill(buildCost);

        return output;
    }
}

As you can see, both methods have to repeat the getCell() and so on.

Then the actions are called like this:

// ....
        if (gameAction.checkValidity(player, board, startCoords, endCoords)) {
            Set<HexCoordinate> affected = gameAction.doTask(p, board, startCoords, endCoords);

            hfField.notifyCellsChanged(affected);
            pidPlayer.playerChanged(p);
        }
// ....

I'm looking for advice on whether the way this is set up smells bad to anyone else, and if so, what are some things I can do to fix it, and if not, what I can do to reduce the duplication.

Answer 1

I think that this type of setup should work fine. If you find yourself repeating multiple steps in various places in code, you could move that functionality to a utility class with static methods. As long as a unit of code can be considered stateless, you should be able to move it to a static method.

Here is a simple example that may hint at what I'm talking about: Say you have some code you've been repeating in 4 different classes, such as checking a particular resource is above a certain point.

public class ResourceUtils {
     public static boolean resourceCheck(Resource r, float cost)
     {
           return r != null && r.getResourceLevel() >= cost;
     }
}

Now you can call this from anywhere else in code:

float cost = 20.4f;
if(ResourceUtils.resourceCheck(goldResource,cost))
{
     ResourceUtils.transaction(goldResource,cost))
     buildUnit();
}