On 03.09.2012 05:14, Michael T. Pope wrote: > 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. >>
Thanks for looking at the problem. > 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? > That might be a good idea. I don't expect it to be too hard. > - 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. > Agreed. > - 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... > I'll have a look at serialization. > - 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 Yes, we should make certain of that. But how? Will tests be sufficient? Do we need an AbstractUnitLocation, so that classes that do not use the standard unit list can inherit from that? I suspect the latter would be the approach approved by the OO methodologists. Regards Michael ------------------------------------------------------------------------------ How fast is your code? 3 out of 4 devs don\\\'t know how their code performs in production. Find out how slow your code is with AppDynamics Lite. http://ad.doubleclick.net/clk;262219672;13503038;z? http://info.appdynamics.com/FreeJavaPerformanceDownload.html _______________________________________________ Freecol-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/freecol-developers
