On Fri, 31 Aug 2012 10:24:28 PM Michael T. Pope wrote: > There are a couple of interesting loose ends I am still looking at, but I am > out of time for now. An amended patch will follow in due course.
Follow up comments.
- I like this patch because it generally simplifies other Unit code.
For example we can get rid of a bunch of:
unit.getGoodsContainer().getGoodsCount(<type>)
because now that Unit is a GoodsLocation we can just call
unit.getGoodsCount(<type>).
This was already true of Settlement but the opportunity was missed
prior to svn.10119. Perhaps we can completely subsume GoodsContainer
into GoodsLocation one day?
- However there is confusion lurking. We already have zero argument
forms of GoodsContainer and Unit.getGoodsCount(), which does not
return a count of goods, but the number of cargo slots taken by the
goods in a container or unit. Since we also already have
Unit.getSpaceLeft(), Unit.getSpaceTaken(), UnitType.getSpace() etc,
I have renamed the zero argument forms to
GoodsContainer.getSpaceTaken(), Unit.getGoodsSpaceTaken() and
introduced a Unit.getUnitSpaceTaken(). The two former forms are
used a fair bit for checking whether a carrier unit has goods to
trade etc, so to make that read even better I added a boolean test
against zero: hasGoodsCargo(). This is all in svn.10121.
- GoodsLocation contained getWarehouseCapacity. That belongs in Settlement.
- I rebased the patch against trunk(attached), and cleaned up a few
getUnitList() problems, in the process hoisting some useful
previously unit-specific routines up to UnitLocation. The uses in
readAttributes/Children are still broken.
- Thus, Unit serialization remains broken. Ideally Unit/GoodsLocation
should handle their own data, but I am hoping you have an idea of
how to do that elegantly...
- There is some hairiness involved in getNoAddReason. Not too
confident that it is right yet.
- Beware of something that tricked me thoroughly. Colony is a
GoodsLocation, and therefore a UnitLocation, but it does *not* use
the units list inside UnitLocation (its units are distributed about
its WorkLocations). However some of the UnitLocation routines work
because Colony provides a correct overriding implementation of
getUnitList. Should we make certain that all the UnitLocation
routines are either working correctly or overridden?
Cheers,
Mike Pope
signature.asc
Description: This is a digitally signed message part.
diff --git a/src/net/sf/freecol/common/model/GoodsLocation.java b/src/net/sf/freecol/common/model/GoodsLocation.java
index 6c5745f..4d91949 100644
--- a/src/net/sf/freecol/common/model/GoodsLocation.java
+++ b/src/net/sf/freecol/common/model/GoodsLocation.java
@@ -28,6 +28,8 @@ import javax.xml.stream.XMLStreamException;
import javax.xml.stream.XMLStreamReader;
import javax.xml.stream.XMLStreamWriter;
+import org.w3c.dom.Element;
+
/**
* The <code>GoodsLocation</code> is a place where {@link Unit}s and
* {@link Goods} can be put. The GoodsLocation can not store any other
@@ -71,6 +73,19 @@ public abstract class GoodsLocation extends UnitLocation {
}
/**
+ * Initialize this object from an XML-representation of this object.
+ *
+ * @param game The <code>Game</code> in which this <code>Unit</code>
+ * belong.
+ * @param e An XML-element that will be used to initialize this object.
+ */
+ // Only Unit needs this
+ public GoodsLocation(Game game, Element e) {
+ super(game, e);
+ readFromXMLElement(e);
+ }
+
+ /**
* Creates a new <code>GoodsLocation</code> instance.
*
* @param game a <code>Game</code> value
@@ -184,16 +199,6 @@ public abstract class GoodsLocation extends UnitLocation {
}
/**
- * Gets the storage capacity of this settlement.
- *
- * @return The storage capacity of this settlement.
- * @see #getGoodsCapacity
- */
- public int getWarehouseCapacity() {
- return getGoodsCapacity();
- }
-
- /**
* Removes a specified amount of a type of Goods from this Settlement.
*
* @param type The type of Goods to remove from this settlement.
@@ -262,14 +267,8 @@ public abstract class GoodsLocation extends UnitLocation {
goodsContainer = null;
}
objects.addAll(super.disposeList());
- return objects;
- }
- /**
- * Dispose of this GoodsLocation.
- */
- public void dispose() {
- disposeList();
+ return objects;
}
/**
@@ -278,9 +277,9 @@ public abstract class GoodsLocation extends UnitLocation {
public NoAddReason getNoAddReason(Locatable locatable) {
Goods goods = (locatable instanceof Goods) ? (Goods)locatable : null;
if (goods != null) {
- // TODO: does not account for packing
- if (goods.getSpaceTaken() + getSpaceTaken() > getGoodsCapacity())
- return NoAddReason.CAPACITY_EXCEEDED;
+ return (goods.getSpaceTaken() + getSpaceTaken()
+ > getGoodsCapacity()) ? NoAddReason.CAPACITY_EXCEEDED
+ : NoAddReason.NONE;
}
return super.getNoAddReason(locatable);
}
diff --git a/src/net/sf/freecol/common/model/Tile.java b/src/net/sf/freecol/common/model/Tile.java
index c9b7d81..c6e3a11 100644
--- a/src/net/sf/freecol/common/model/Tile.java
+++ b/src/net/sf/freecol/common/model/Tile.java
@@ -573,24 +573,6 @@ public final class Tile extends UnitLocation implements Named, Ownable {
}
/**
- * Gets the first <code>Unit</code> on this tile.
- *
- * @return The first <code>Unit</code> on this tile.
- */
- public Unit getFirstUnit() {
- return isEmpty() ? null : getUnitList().get(0);
- }
-
- /**
- * Gets the last <code>Unit</code> on this tile.
- *
- * @return The last <code>Unit</code> on this tile.
- */
- public Unit getLastUnit() {
- return isEmpty() ? null : getUnitList().get(getUnitCount() - 1);
- }
-
- /**
* Returns the total amount of Units at this Location. This also includes
* units in a carrier
*
diff --git a/src/net/sf/freecol/common/model/Unit.java b/src/net/sf/freecol/common/model/Unit.java
index 40803d4..4c72669 100644
--- a/src/net/sf/freecol/common/model/Unit.java
+++ b/src/net/sf/freecol/common/model/Unit.java
@@ -62,14 +62,14 @@ import org.w3c.dom.Element;
* Every <code>Unit</code> is owned by a {@link Player} and has a
* {@link Location}.
*/
-public class Unit extends FreeColGameObject
- implements Consumer, Locatable, Location, Movable, Nameable, Ownable {
+public class Unit extends GoodsLocation
+ implements Consumer, Locatable, Movable, Nameable, Ownable {
private static final Logger logger = Logger.getLogger(Unit.class.getName());
private static final EquipmentType horsesEq[] = { null, null };
private static final EquipmentType musketsEq[] = { null, null };
-
+
/** A comparator to order units by skill level. */
private static Comparator<Unit> skillLevelComp
= new Comparator<Unit>() {
@@ -285,10 +285,6 @@ public class Unit extends FreeColGameObject
protected String ethnicity = null;
- protected List<Unit> units = Collections.emptyList();
-
- protected GoodsContainer goodsContainer;
-
protected Location entryLocation;
protected Location location;
@@ -434,6 +430,16 @@ public class Unit extends FreeColGameObject
}
/**
+ * Returns true if this unit can carry treasure (like a treasure train)
+ *
+ * @return <code>true</code> if this <code>Unit</code> is capable of
+ * carrying treasure.
+ */
+ public boolean canCarryTreasure() {
+ return unitType.hasAbility(Ability.CARRY_TREASURE);
+ }
+
+ /**
* Returns the current amount of treasure in this unit. Should be type of
* TREASURE_TRAIN.
*
@@ -1434,7 +1440,7 @@ public class Unit extends FreeColGameObject
: search(start, threatDecider, CostDeciders.avoidIllegal(),
reverseRange, getCarrier());
}
-
+
/**
* Checks if there is a credible threatening unit to this unit
* within a range of moves.
@@ -2072,7 +2078,7 @@ public class Unit extends FreeColGameObject
* @return The number cargo slots occupied by goods.
*/
public int getGoodsSpaceTaken() {
- return (canCarryGoods()) ? goodsContainer.getSpaceTaken() : 0;
+ return (canCarryGoods()) ? getGoodsContainer().getSpaceTaken() : 0;
}
/**
@@ -2133,33 +2139,29 @@ public class Unit extends FreeColGameObject
}
/**
- * Checks whether or not the specified locatable may be added to this
- * <code>Unit</code>. The locatable cannot be added is this
- * <code>Unit</code> if it is not a carrier or if there is no room left.
- *
- * It is not an error to try to add something that is already present
- * (add() above will return success in that case).
- *
- * @param locatable The <code>Locatable</code> to test the addabillity of.
- * @return The result.
+ * {@inheritDoc}
*/
- public boolean canAdd(Locatable locatable) {
+ public NoAddReason getNoAddReason(Locatable locatable) {
if (locatable == this) {
- return false;
+ return NoAddReason.ALREADY_PRESENT;
} else if (locatable instanceof Unit) {
- if (units.equals(Collections.emptyList())) {
- units = new ArrayList<Unit>();
- }
- return canCarryUnits()
- && !units.contains((Unit)locatable)
- && getSpaceLeft() >= ((Unit)locatable).getSpaceTaken();
+ return (!canCarryUnits())
+ ? NoAddReason.WRONG_TYPE
+ : (((Unit)locatable).getSpaceTaken() > getSpaceLeft())
+ ? NoAddReason.CAPACITY_EXCEEDED
+ : super.getNoAddReason(locatable);
} else if (locatable instanceof Goods) {
- return canCarryGoods()
- && (getLoadableAmount(((Goods)locatable).getType())
- > ((Goods)locatable).getAmount());
- } else {
- return false;
+ Goods goods = (Goods)locatable;
+ return (!canCarryGoods())
+ ? NoAddReason.WRONG_TYPE
+ : (goods.getAmount() > getLoadableAmount(goods.getType()))
+ ? NoAddReason.CAPACITY_EXCEEDED
+ : NoAddReason.NONE;
+ // Do not call super.getNoAddReason for goods because
+ // the capacity test in GoodsLocation.getNoAddReason does not
+ // account for packing and is thus too conservative.
}
+ return super.getNoAddReason(locatable);
}
/**
@@ -2173,17 +2175,22 @@ public class Unit extends FreeColGameObject
return false;
} else if (locatable instanceof Unit) {
Unit unit = (Unit)locatable;
- spendAllMoves();
- unit.setState(UnitState.SENTRY);
- return units.add(unit);
+ if (super.add(locatable)) {
+ spendAllMoves();
+ ((Unit)locatable).setState(UnitState.SENTRY);
+ return true;
+ }
} else if (locatable instanceof Goods) {
- Goods goods = (Goods) locatable;
- spendAllMoves();
- return goodsContainer.addGoods(goods);
+ Goods goods = (Goods)locatable;
+ if (super.addGoods(goods)) {
+ spendAllMoves();
+ return true;
+ }
} else {
throw new IllegalStateException("Can not be added to unit: "
- + ((FreeColGameObject) locatable).toString());
+ + ((FreeColGameObject)locatable).toString());
}
+ return false;
}
/**
@@ -2196,12 +2203,12 @@ public class Unit extends FreeColGameObject
if (locatable == null) {
throw new IllegalArgumentException("Locatable must not be 'null'.");
} else if (locatable instanceof Unit && canCarryUnits()) {
- if (units.remove((Unit)locatable)) {
+ if (super.remove((Unit)locatable)) {
spendAllMoves();
return true;
}
} else if (locatable instanceof Goods && canCarryGoods()) {
- if (goodsContainer.removeGoods((Goods)locatable) != null) {
+ if (super.removeGoods((Goods)locatable) != null) {
spendAllMoves();
return true;
}
@@ -2212,68 +2219,18 @@ public class Unit extends FreeColGameObject
return false;
}
- /**
- * Checks if this <code>Unit</code> contains the specified
- * <code>Locatable</code>.
- *
- * @param locatable The <code>Locatable</code> to test the presence of.
- * @return True if the locatable is on board this carrier.
- */
- public boolean contains(Locatable locatable) {
- if (locatable instanceof Unit && canCarryUnits()) {
- return units.contains((Unit)locatable);
- } else if (locatable instanceof Goods && canCarryGoods()) {
- return goodsContainer.contains((Goods)locatable);
- } else {
- return false;
- }
- }
-
- /**
- * Gets the amount of goods of a specified type aboard this unit.
- *
- * @param type The <code>GoodsType</code> to query.
- * @return The amount of goods.
- */
- public int getGoodsCount(GoodsType type) {
- return getGoodsContainer().getGoodsCount(type);
- }
-
- /**
- * Gets the amount of Units at this Location.
- *
- * @return The amount of Units at this Location.
- */
- public int getUnitCount() {
- return units.size();
- }
+ // Superclass contains() should work, as should canAdd() because
+ // getNoAddReason() was provided above.
/**
- * Gets the first <code>Unit</code> beeing carried by this
- * <code>Unit</code>.
- *
- * @return The <code>Unit</code>.
+ * {@inheritDoc}
*/
- public Unit getFirstUnit() {
- if (units.isEmpty()) {
- return null;
- } else {
- return units.get(0);
- }
+ public int getGoodsCapacity() {
+ throw new RuntimeException("Do not call this method, unless we implement spoilage.");
}
- /**
- * Gets the last <code>Unit</code> beeing carried by this
- * <code>Unit</code>.
- *
- * @return The <code>Unit</code>.
- */
- public Unit getLastUnit() {
- if (units.isEmpty()) {
- return null;
- } else {
- return units.get(units.size() - 1);
- }
+ public List<Goods> getGoodsList() {
+ return getGoodsContainer().getGoods();
}
/**
@@ -2297,55 +2254,6 @@ public class Unit extends FreeColGameObject
}
/**
- * Gets a <code>Iterator</code> of every <code>Unit</code> directly
- * located on this <code>Location</code>.
- *
- * @return The <code>Iterator</code>.
- */
- public Iterator<Unit> getUnitIterator() {
- return new ArrayList<Unit>(units).iterator();
- }
-
- /**
- * Gets a list of the units in this carrier unit.
- *
- * @return The list of units in this carrier unit.
- */
- public List<Unit> getUnitList() {
- return new ArrayList<Unit>(units);
- }
-
- /**
- * Gets a <code>Iterator</code> of every <code>Unit</code> directly
- * located on this <code>Location</code>.
- *
- * @return The <code>Iterator</code>.
- */
- public Iterator<Goods> getGoodsIterator() {
- if (canCarryGoods()) {
- return goodsContainer.getGoodsIterator();
- } else {
- return EmptyIterator.getInstance();
- }
- }
-
- /**
- * Returns a <code>List</code> containing the goods carried by this unit.
- * @return a <code>List</code> containing the goods carried by this unit.
- */
- public List<Goods> getGoodsList() {
- if (canCarryGoods()) {
- return goodsContainer.getGoods();
- } else {
- return Collections.emptyList();
- }
- }
-
- public GoodsContainer getGoodsContainer() {
- return goodsContainer;
- }
-
- /**
* Sets the units location without updating any other variables
*
* @param newLocation The new Location
@@ -3152,18 +3060,6 @@ public class Unit extends FreeColGameObject
}
/**
- * Move the given unit to the front of this carrier (make sure it'll be the
- * first unit in this unit's unit list).
- *
- * @param u The unit to move to the front.
- */
- public void moveToFront(Unit u) {
- if (canCarryUnits() && units.remove(u)) {
- units.add(0, u);
- }
- }
-
- /**
* Gets the amount of work left.
*
* @return The amount of work left.
@@ -3348,16 +3244,6 @@ public class Unit extends FreeColGameObject
}
/**
- * Returns true if this unit can carry treasure (like a treasure train)
- *
- * @return <code>true</code> if this <code>Unit</code> is capable of
- * carrying treasure.
- */
- public boolean canCarryTreasure() {
- return unitType.hasAbility(Ability.CARRY_TREASURE);
- }
-
- /**
* Returns true if this unit is a ship that can capture enemy goods.
*
* @return <code>true</code> if this <code>Unit</code> is capable of
@@ -3415,9 +3301,6 @@ public class Unit extends FreeColGameObject
*/
public List<FreeColGameObject> disposeList() {
List<FreeColGameObject> objects = new ArrayList<FreeColGameObject>();
- while (units.size() > 0) {
- objects.addAll(units.remove(0).disposeList());
- }
if (location != null) {
location.remove(this);
@@ -3438,22 +3321,11 @@ public class Unit extends FreeColGameObject
getOwner().invalidateCanSeeTiles();
getOwner().removeUnit(this);
- if (unitType.canCarryGoods()) {
- objects.addAll(goodsContainer.disposeList());
- }
-
objects.addAll(super.disposeList());
return objects;
}
/**
- * Removes all references to this object.
- */
- public void dispose() {
- disposeList();
- }
-
- /**
* Return how many turns left to be repaired
*
* @return turns to be repaired
@@ -3642,7 +3514,7 @@ public class Unit extends FreeColGameObject
}
return tile;
}
-
+
/**
* Get a settlement by id, validating as much as possible.
* Designed for message unpacking where the id should not be trusted.
@@ -3655,7 +3527,7 @@ public class Unit extends FreeColGameObject
public Settlement getAdjacentSettlementSafely(String settlementId)
throws IllegalStateException {
Game game = getOwner().getGame();
-
+
Settlement settlement = game.getFreeColGameObject(settlementId,
Settlement.class);
if (settlement == null) {
@@ -3744,6 +3616,7 @@ public class Unit extends FreeColGameObject
private void unitsToXML(XMLStreamWriter out, Player player,
boolean showAll, boolean toSavedGame)
throws XMLStreamException {
+ List<Unit> units = getUnitList();
if (!units.isEmpty()) {
out.writeStartElement(UNITS_TAG_NAME);
for (Unit unit : units) {
@@ -3855,13 +3728,12 @@ public class Unit extends FreeColGameObject
if (full) {
unitsToXML(out, player, showAll, toSavedGame);
if (getType().canCarryGoods()) {
- goodsContainer.toXML(out, player, showAll, toSavedGame);
+ getGoodsContainer().toXML(out, player, showAll, toSavedGame);
}
} else {
if (getType().canCarryGoods()) {
- out.writeAttribute("visibleGoodsCount",
- Integer.toString(getGoodsSpaceTaken()));
- goodsContainer.toXML(out, player, showAll, toSavedGame);
+ out.writeAttribute("visibleGoodsCount", Integer.toString(getVisibleGoodsCount()));
+ getGoodsContainer().toXML(out, player, showAll, toSavedGame);
}
}
@@ -3970,8 +3842,8 @@ public class Unit extends FreeColGameObject
entryLocation = newLocation(in.getAttributeValue(null, "entryLocation"));
location = newLocation(in.getAttributeValue(null, "location"));
- units.clear();
- if (goodsContainer != null) goodsContainer.removeAll();
+ getUnitList().clear();
+ if (getGoodsContainer() != null) getGoodsContainer().removeAll();
equipment.clear();
setWorkImprovement(null);
}
@@ -3980,19 +3852,19 @@ public class Unit extends FreeColGameObject
Game game = getGame();
while (in.nextTag() != XMLStreamConstants.END_ELEMENT) {
if (in.getLocalName().equals(UNITS_TAG_NAME)) {
- units = new ArrayList<Unit>();
+ getUnitList().clear();
while (in.nextTag() != XMLStreamConstants.END_ELEMENT) {
if (in.getLocalName().equals(Unit.getXMLElementTagName())) {
- units.add(updateFreeColGameObject(in, Unit.class));
+ getUnitList().add(updateFreeColGameObject(in, Unit.class));
}
}
} else if (in.getLocalName().equals(GoodsContainer.getXMLElementTagName())) {
- goodsContainer = game.getFreeColGameObject(in.getAttributeValue(null, ID_ATTRIBUTE),
- GoodsContainer.class);
- if (goodsContainer != null) {
- goodsContainer.readFromXML(in);
+ setGoodsContainer(game.getFreeColGameObject(in.getAttributeValue(null, ID_ATTRIBUTE),
+ GoodsContainer.class));
+ if (getGoodsContainer() != null) {
+ getGoodsContainer().readFromXML(in);
} else {
- goodsContainer = new GoodsContainer(game, this, in);
+ setGoodsContainer(new GoodsContainer(game, this, in));
}
} else if (in.getLocalName().equals(EQUIPMENT_TAG)) {
String xLength = in.getAttributeValue(null, ARRAY_SIZE);
@@ -4017,9 +3889,9 @@ public class Unit extends FreeColGameObject
}
// ensure all carriers have a goods container, just in case
- if (goodsContainer == null && getType().canCarryGoods()) {
- logger.warning("Carrier with ID " + getId() + " did not have a \"goodsContainer\"-tag.");
- goodsContainer = new GoodsContainer(game, this);
+ if (getGoodsContainer() == null && getType().canCarryGoods()) {
+ logger.warning("Carrier with ID " + getId() + " did not have a \"getGoodsContainer()\"-tag.");
+ setGoodsContainer(new GoodsContainer(game, this));
}
setRole();
diff --git a/src/net/sf/freecol/common/model/UnitLocation.java b/src/net/sf/freecol/common/model/UnitLocation.java
index 0fe8bb8..cb8d0bc 100644
--- a/src/net/sf/freecol/common/model/UnitLocation.java
+++ b/src/net/sf/freecol/common/model/UnitLocation.java
@@ -29,6 +29,8 @@ import javax.xml.stream.XMLStreamException;
import javax.xml.stream.XMLStreamReader;
import javax.xml.stream.XMLStreamWriter;
+import org.w3c.dom.Element;
+
/**
* The <code>UnitLocation</code> is a place where a <code>Unit</code>
* can be put. The UnitLocation can not store any other Locatables,
@@ -136,6 +138,12 @@ public abstract class UnitLocation extends FreeColGameObject implements Location
super(game, id);
}
+ // Only Unit needs this
+ public UnitLocation(Game game, Element e) {
+ super(game, e);
+ readFromXMLElement(e);
+ }
+
/**
* Removes all references to this object.
*
@@ -151,13 +159,6 @@ public abstract class UnitLocation extends FreeColGameObject implements Location
}
/**
- * Dispose of this UnitLocation.
- */
- public void dispose() {
- disposeList();
- }
-
- /**
* Gets the current space taken by the units in this location.
*
* @return The sum of the space taken by the units in this location.
@@ -186,6 +187,37 @@ public abstract class UnitLocation extends FreeColGameObject implements Location
return units.size() >= getUnitCapacity();
}
+ /**
+ * Gets the first <code>Unit</code> beeing carried by this
+ * <code>Unit</code>.
+ *
+ * @return The <code>Unit</code>.
+ */
+ public Unit getFirstUnit() {
+ List<Unit> units = getUnitList();
+ return (units.isEmpty()) ? null : units.get(0);
+ }
+
+ /**
+ * Gets the last <code>Unit</code> beeing carried by this
+ * <code>Unit</code>.
+ *
+ * @return The <code>Unit</code>.
+ */
+ public Unit getLastUnit() {
+ List<Unit> units = getUnitList();
+ return (units.isEmpty()) ? null : units.get(units.size()-1);
+ }
+
+ /**
+ * Move the given unit to the front of the units list.
+ *
+ * @param u The <code>Unit</code> to move to the front.
+ */
+ public void moveToFront(Unit u) {
+ if (units.remove(u)) units.add(0, u);
+ }
+
// Interface Location
diff --git a/src/net/sf/freecol/server/model/ServerUnit.java b/src/net/sf/freecol/server/model/ServerUnit.java
index f0fdb22..dab7c82 100644
--- a/src/net/sf/freecol/server/model/ServerUnit.java
+++ b/src/net/sf/freecol/server/model/ServerUnit.java
@@ -124,7 +124,7 @@ public class ServerUnit extends Unit implements ServerModelObject {
visibleGoodsCount = -1;
if (type.canCarryGoods()) {
- goodsContainer = new GoodsContainer(game, this);
+ setGoodsContainer(new GoodsContainer(game, this));
}
UnitType newType = type.getTargetType(ChangeType.CREATION, owner);
------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________ Freecol-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/freecol-developers
