On Jun 25, 2012, at 7:12 PM, Jacques Le Roux wrote:
> From: "Jacopo Cappellato" <jacopo.cappell...@hotwaxmedia.com>
>> Hi all,
>>
>> this is part of my review of framework code to make it cleaner. I see a
>> series of questionable changes in this commit, do you
>> agree?
>> Jacques, before I start my review please let me know if you want to do a
>> first pass on it.
>
> I can certainly have a fresh look, depends on timing though...
> What did you saw at 1st glance which makes you want to improve, is it related
> to GeronimoMultiOfbizInstances?
>
> Jacques
Mostly all the code in the commit seems questionable and I would like to see it
reverted or completely refactored.
In particular, the changes to ContextFilter are awful:
String GeronimoMultiOfbizInstances = (String)
config.getServletContext().getAttribute("GeronimoMultiOfbizInstances");
if (UtilValidate.isNotEmpty(GeronimoMultiOfbizInstances)) {
String ofbizHome = System.getProperty("ofbiz.home");
if (GeronimoMultiOfbizInstances.equalsIgnoreCase("true") &&
UtilValidate.isEmpty(ofbizHome)) {
ofbizHome = System.getProperty("ofbiz.home"); // This is only
used in case of Geronimo or WASCE using OFBiz multi-instances. It allows to
retrieve ofbiz.home value set in JVM env
System.out.println("Set OFBIZ_HOME to - " + ofbizHome);
System.setProperty("ofbiz.home", ofbizHome);
}
}
In just a few lines added to the class I see:
* bad variable name ("GeronimoMultiOfbizInstances")
* two if blocks that should be one
* a bad practice for logging
* all the code seems like a bad tweak
* the code doesn't make any sense to me... to the point that I may be
misunderstanding it: why do you get the property and then set it? how this is
going to fix the problem? what was the problem?
Why did you commit code like this? Did you really think it was good for OFBiz?
Regards,
Jacopo
