Hi Emanuele, I took an initial look over the catalog refactoring patch and here is what I came up with.
First off great work. I know this is a big effort and I think you for taking the lead on this. It is great to see things moving in the right direction. * CatalogDAO The first thing I noticed and something that actually made the patch kind of hard to review was that method names have changed: Catalog.add --> CatalogDAO.save Catalog.save --> CatalogDAO.update Catalog.remove --> CatalogDAO.delete Why the changes? Any reason not to keep the names the same. Developers who are already used to the existing method naming scheme will probably get confused as I did. Also, save and update are sort of ambiguous. If we do stick with a new naming scheme I would like to see save be renamed something more unambigious like add or create, something that explicitly indicates that this is a new resource being added. There are also no javadocs. This is going to be a very central piece of api so I would like to see something. Again if the same naming scheme was kept the javadocs from Catalog should be able to be carried across verbatim. * InMemoryCatalogDAO Rename to just MemoryCatalogDAO I notice that this class also has no notion of proxying that is occurring. This seems odd that this is still the responsibility of CatalogImpl. The whole point of this excercise is to have a single CatalogImpl and multiple daos for different "storage formats". Since using proxy is an implementation detail of a memory based catalog it should be totally encapsulated by that dao implementation. Ie we will not want to proxy objects coming from the hibernate dao. * LayerGroupInfoImpl In getStyles() i see some code for lazily instantiating the styles list. This pattern is not really used throughout the catalog and configuration beans. As an alternative I think i would rather see either: a) XStreamPersister ensure that the styles list is property initializes b) implement a readResolve() method that will do the instantiation * CatalogWriter, LegacyCatalogReader I notice that the patch modifies the storage format to put a "data" element directly under the "catalog" root element. What is the purpose of this? The whole point of these classes is to maintain backwards compatability with old 1.x style data directories. I am not sure why it would change. And that is it for now :) -Justin -- Justin Deoliveira OpenGeo - http://opengeo.org Enterprise support for open source geospatial. ------------------------------------------------------------------------------ SOLARIS 10 is the OS for Data Centers - provides features such as DTrace, Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW http://p.sf.net/sfu/solaris-dev2dev _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
