widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing

Thanks, the pointer to the files to look at was very good. In general I think this is a great change and improves the design a lot - it also shows further places for refactoring which were much harder to see before this change. Here are some thoughts:

class Storage:
looks good. Could you use more empty lines to separate the methods? Also, it feels like wares and workers are disjunct, so maybe you can make a HasWares and HasWorkers Interface (those are the names I choose for Lua, consistency would be preferred, but not needed). I see that you use a lot of interfaces here - be aware that you cannot use double inheritance freely in the code as we static_cast a lot around and this breaks with double inheritance. 

class Garrison:
I do not understand how you want to deal trainingssites with this one. It seems to implement the interface needed for warfare which will not be supported by trainingssites/warehouses. Otherwise looks good - then interface is a bit thicker than I would like it to be - but this is not your design error, but was there before. Maybe we can slim it out a bit later on.

About the use case: Interfaces are not a as good an idea in C++ than in Java. C++ does not have the concept and therefore creates storage for the classes. This leads to problems like the diamond inheritance[1]. This can be worked around using virtual inheritance, but then you can no longer static_cast your class pointers around. My suggestion for the design here would be to use composition [2] instead - and I would probably add getters into Building that return nullptr_t - productionsites and so on can then overwrite these getters to return correct values. So, by default a Building does not has_wares, nor has_workers nor has_soldiers, but some specialized buildings can overwrite these then.

[1] http://en.wikipedia.org/wiki/Diamond_inheritance#The_diamond_problem
[2] http://en.wikipedia.org/wiki/Composition_over_inheritance
-- 
https://code.launchpad.net/~widelands-dev/widelands/storages_garrisons/+merge/178704
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/storages_garrisons.