1
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ It doesn't look excessively redundant to me. \$\endgroup\$ – 200_success Aug 11 '13 at 0:08
  • \$\begingroup\$ The most obvious candidate would be exceptions. Probably with a rollback method which is called in the catch case. Your thoughts about them? \$\endgroup\$ – tb- Aug 12 '13 at 17:11
  • \$\begingroup\$ I have nothing against exceptions at all. The way its designed now is that there is a contract: if checkValidity() returns true, then it is definitely safe to call doAction () with the same arguments. So doAction can assume that checkValidity returned true and thus doesn't need to do any sanity checking. So the hope is to prevent exceptions in that way. \$\endgroup\$ – scott_fakename Aug 12 '13 at 19:44
  • \$\begingroup\$ I guess this wasn't the best example to illustrate the problem though. Maybe the drawback of the design is just simply that both methods need to do the same setup stuff. Might be that there really is no easy answer without a total redesign. So be it I suppose \$\endgroup\$ – scott_fakename Aug 12 '13 at 19:47
2
\$\begingroup\$

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();
}
\$\endgroup\$

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.