Re: [Geoserver-devel] catalog dao split patch review #2

[Justin D. <jde...@op...>]

>
> Logging guard is a good habit and also a requirement I have put
> forward for our (GeoSolutions) code.
> There are plenty of measurements around the web that shows why we
> should use it, especially with logging levels like FINE, FINER, etc..
> Bottom line is, I would find it hard to ask ETj to not use this, since
> it is usually easy to forget good habits :-).

Well this this case I would be astonished if there was a measurable 
different. And last time I checked GeoServer coding conventions trump 
ones used by any specific company. That said there is no hard and fast 
rule about logging guards and it can never hurt so no issue there. Sorry 
for being too picky.

There is however a hard and fast rule about curly braces though. And for 
good reason. As soon as people write code without curly braces on if 
statements and forget to set the proper formatting setup in their ide 
code essentially becomes unreadable.
>
> Ciao,
> Simone.
>
>>
>> * I see a lot of commented out code. Just remove it, it makes the patch
>> cleaner to review.
>>
>> CatalogDAO
>> ----------
>>
>> * Still no javadocs
>>
>> * needsIdRemapping() ? What is the purpose of this method. Seems to
>> expose implementation details uncessarily. Some explanation is required.
>>
>>
>> CatalogImpl
>> -----------
>>
>> * add(WorkspaceInfo):
>>
>>    - Yes validate already contains this check, the check can probably be
>> removed
>>
>> * setDefaultWorkspace(WorkspaceInfo)
>>
>>    - Remove the check, the contract of setDefaultWorkspace states that
>> the ws does not need to be previously added to the catalog
>>
>> * postSave(CatatlogInfo)
>>
>>    - Why is the call to proxy.commit() commented out?
>>
>> * CatalogSync
>>
>>    - Why a separate class for this? Why not just keep it as
>> Catalog.sync() and just call through to the dao.
>>
>> MemoryCatalogDAO
>> ----------------
>>
>> * The save() methods seems to duplicate each other. The old catalog
>> factored this out into a common method. Again a pure refactor would have
>> maintained this I would think.
>>
>> * getLayersByRsource():
>>
>>    - Some checks seems to be commented out? Were these checks added by
>> you, I actually could not find them in the original CatalogImpl class.
>>
>>
>> -Justin
>>
>> --
>> Justin Deoliveira
>> OpenGeo - http://opengeo.org
>> Enterprise support for open source geospatial.
>>
>> ------------------------------------------------------------------------------
>> Download Intel® Parallel Studio Eval
>> Try the new software tools for yourself. Speed compiling, find bugs
>> proactively, and fine-tune applications for parallel performance.
>> See why Intel Parallel Studio got high marks during beta.
>> http://p.sf.net/sfu/intel-sw-dev
>> _______________________________________________
>> Geoserver-devel mailing list
>> Geo...@li...
>> https://lists.sourceforge.net/lists/listinfo/geoserver-devel
>>


-- 
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.