You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by jl...@apache.org on 2009/11/22 01:20:12 UTC
svn commit: r883017 -
/ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java
Author: jleroux
Date: Sun Nov 22 00:20:12 2009
New Revision: 883017
URL: http://svn.apache.org/viewvc?rev=883017&view=rev
Log:
A modified patch from Marco Risailit " Add the possibility to filter the list of available countries from the drop down." (https://issues.apache.org/jira/browse/OFBIZ-3192) - OFBIZ-3192
I have used StringUtil.split() and enhanced for loops instead
Modified:
ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java
Modified: ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java?rev=883017&r1=883016&r2=883017&view=diff
==============================================================================
--- ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java (original)
+++ ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java Sun Nov 22 00:20:12 2009
@@ -18,19 +18,20 @@
*******************************************************************************/
package org.ofbiz.common;
+import java.util.Iterator;
import java.util.List;
-import java.util.Map;
import javolution.util.FastList;
import org.ofbiz.base.util.Debug;
+import org.ofbiz.base.util.StringUtil;
import org.ofbiz.base.util.UtilMisc;
import org.ofbiz.base.util.UtilProperties;
+import org.ofbiz.base.util.UtilValidate;
import org.ofbiz.entity.Delegator;
import org.ofbiz.entity.GenericEntityException;
import org.ofbiz.entity.GenericValue;
import org.ofbiz.entity.condition.EntityCondition;
-import org.ofbiz.entity.condition.EntityConditionList;
import org.ofbiz.entity.condition.EntityExpr;
import org.ofbiz.entity.condition.EntityOperator;
@@ -47,15 +48,44 @@
GenericValue defaultGeo = null;
if (defaultCountry != null && defaultCountry.length() > 0) {
try {
- defaultGeo = delegator.findByPrimaryKeyCache("Geo", UtilMisc.toMap("geoId", defaultCountry));
+ defaultGeo = delegator.findOne("Geo", UtilMisc.toMap("geoId", defaultCountry), true);
} catch (GenericEntityException e) {
Debug.logError(e, "Cannot lookup Geo", module);
}
}
+ List<String> countriesList = FastList.newInstance();
+ List<String> countriesAvailable = StringUtil.split(UtilProperties.getPropertyValue("general.properties", "countries.geo.id.available"), ",");
+ if (countriesAvailable != null) {
+ for(String country : countriesAvailable) {
+ GenericValue geoCountry = null;
+ try {
+ geoCountry = delegator.findOne("Geo", UtilMisc.toMap("geoId", country.trim()), true);
+ } catch (GenericEntityException e) {
+ Debug.logError(e, "The country specified into countries.geo.id.available does not exists in Geo", module);
+ }
+ if (geoCountry != null) {
+ countriesList.add(country);
+ }
+ }
+ }
try {
- List<GenericValue> countryGeoList = delegator.findByAndCache("Geo", UtilMisc.toMap("geoTypeId", "COUNTRY"), UtilMisc.toList("geoName"));
+ List<EntityExpr> exprs = UtilMisc.toList(EntityCondition.makeCondition("geoTypeId", EntityOperator.EQUALS, "COUNTRY"));
+ // only available countries
+ if (UtilValidate.isNotEmpty(countriesList)) {
+ exprs.add(EntityCondition.makeCondition("geoId", EntityOperator.IN, countriesList));
+ }
+ List<GenericValue> countryGeoList = delegator.findList("Geo", EntityCondition.makeCondition(exprs), null, UtilMisc.toList("geoName"), null, true);
if (defaultGeo != null) {
geoList.add(defaultGeo);
+ /* remove the default geo to avoid double rows in the drop-down */
+ int idx = 0;
+ for (GenericValue geo : countryGeoList) {
+ if (geo.get("geoId") != null && defaultGeo.get("geoId") != null &&
+ geo.get("geoId").equals(defaultGeo.get("geoId"))) {
+ countryGeoList.remove(idx);
+ }
+ idx += 1;
+ }
geoList.addAll(countryGeoList);
} else {
geoList = countryGeoList;
Re: svn commit: r883017 - /ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java
Posted by Adam Heath <do...@brainfood.com>.
Jacques Le Roux wrote:
> From: "Jacques Le Roux" <ja...@les7arts.com>
>> BTW, you did not mention anyting about
>> if (countriesAvailable != null) {
>> This would be my main concern. I want to refactor StringUtil .split
>> (and uses) so that it renders an empty list when now it's
>> rendering null. To avoid this useless check when using an enhanced for
>> loop (lists often use enhanced for loops) and mostly because it's
>> cleaner IMO. I have to check current uses though...
>
> 136 occurences, on the 1st teen I looked at, only one relies "on
> ==null", but anyway it's too much work and risk for what it is worth
Not to mention it is slower.
The enhanced for is not defined to do an isEmpty check on the
Collection or the array. In all likely hood, it probably always
creates the Iterator, just to then do an do-nothing hasNext call.
The real question then becomes whether the normal for each call site
is to have an empty list, or a null list. And that might be hard to
answer.
Re: svn commit: r883017 - /ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "Jacques Le Roux" <ja...@les7arts.com>
> BTW, you did not mention anyting about
> if (countriesAvailable != null) {
> This would be my main concern. I want to refactor StringUtil .split (and uses) so that it renders an empty list when now it's
> rendering null. To avoid this useless check when using an enhanced for loop (lists often use enhanced for loops) and mostly
> because it's cleaner IMO. I have to check current uses though...
136 occurences, on the 1st teen I looked at, only one relies "on ==null", but anyway it's too much work and risk for what it is
worth
Jacques
Re: svn commit: r883017 - /ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "Jacques Le Roux" <ja...@les7arts.com>
> Hi Scott,
>
> 1st I find Marco's idea very good!
> And I'd like to apply it also to languages. Who needs all languages ? Of course if you use only one language (English ;o) you
> can't
> see that as a drawback, but when you have continuously to change from one language to English, it can be boring...
> So OOTB we should even restrain to languages we currently support (a dozen I guess). I hope to apply the concept soon...
Sorry for the noise, I thought there was a problem with UtilMisc.java.availableLocales() in backend, but it works as well as in
eCommerce actually, certainly browser caches....
Jacques
Re: svn commit: r883017 - /ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Hi Scott,
1st I find Marco's idea very good!
And I'd like to apply it also to languages. Who needs all languages ? Of course if you use only one language (English ;o) you can't
see that as a drawback, but when you have continuously to change from one language to English, it can be boring...
So OOTB we should even restrain to languages we currently support (a dozen I guess). I hope to apply the concept soon...
Now about implementation, inline...
From: "Scott Gray" <sc...@hotwaxmedia.com>
> [snip]
>> + List<String> countriesList = FastList.newInstance();
>> + List<String> countriesAvailable = StringUtil .split(UtilProperties.getPropertyValue("general.properties",
>> "countries.geo.id.available"), ",");
>> + if (countriesAvailable != null) {
>> + for(String country : countriesAvailable) {
>> + GenericValue geoCountry = null;
>> + try {
>> + geoCountry = delegator.findOne("Geo", UtilMisc.toMap("geoId", country.trim()), true);
>> + } catch (GenericEntityException e) {
>> + Debug.logError(e, "The country specified into countries.geo.id.available does not exists in Geo", module);
>> + }
>> + if (geoCountry != null) {
>> + countriesList.add(country);
>> + }
>> + }
>> + }
>
> You do realize that you are going through the list of countries and grabbing each Geo just to make sure it exists and then in the
> next block retrieving them all again? Somewhat pointless to have them all, throw them away and then grab them again.
Even in worse cases (246 countries OOTB), for practical purpose I guess nobody would have noticed anything on UI level.
But I agree we can do better so I tried to do it in another, faster way: commited at r884317
BTW, you did not mention anyting about
if (countriesAvailable != null) {
This would be my main concern. I want to refactor StringUtil .split (and uses) so that it renders an empty list when now it's
rendering null. To avoid this useless check when using an enhanced for loop (lists often use enhanced for loops) and mostly because
it's cleaner IMO. I have to check current uses though...
> [snip]
>> + /* remove the default geo to avoid double rows in the drop-down */
>> + int idx = 0;
>> + for (GenericValue geo : countryGeoList) {
>> + if (geo.get("geoId") != null && defaultGeo.get("geoId") != null &&
>> + geo.get("geoId").equals(defaultGeo.get("geoId"))) {
>> + countryGeoList.remove(idx);
>> + }
>> + idx += 1;
>> + }
>> geoList.addAll(countryGeoList);
>> } else {
>> geoList = countryGeoList;
>
> Enhanced for loops are great but they're not really appropriate when you do actually need to know the index during iteration.
I mostly prefer them because I like the concept, it's easier to write, to read and that's important points to me. I agree that it's
maybe less efficient than a for loop with index on list for this case, or rather ListIterator wich would have been perfect.
> Also the geoId null checks are somewhat superfluous considering that geoId is the primary key.
Thanks for the geoId the primary key spot, good idea indeed, lesson learned. Even more now that we guarantee a primary key can't be
null (since r835303)
Thanks
Jacques
Re: svn commit: r883017 - /ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java
Posted by Scott Gray <sc...@hotwaxmedia.com>.
On 22/11/2009, at 1:20 PM, jleroux@apache.org wrote:
> Author: jleroux
> Date: Sun Nov 22 00:20:12 2009
> New Revision: 883017
>
> URL: http://svn.apache.org/viewvc?rev=883017&view=rev
> Log:
> A modified patch from Marco Risailit " Add the possibility to filter
> the list of available countries from the drop down." (https://issues.apache.org/jira/browse/OFBIZ-3192
> ) - OFBIZ-3192
> I have used StringUtil.split() and enhanced for loops instead
>
> Modified:
> ofbiz/trunk/framework/common/src/org/ofbiz/common/
> CommonWorkers.java
>
> Modified: ofbiz/trunk/framework/common/src/org/ofbiz/common/
> CommonWorkers.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonWorkers.java?rev=883017&r1=883016&r2=883017&view=diff
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- ofbiz/trunk/framework/common/src/org/ofbiz/common/
> CommonWorkers.java (original)
> +++ ofbiz/trunk/framework/common/src/org/ofbiz/common/
> CommonWorkers.java Sun Nov 22 00:20:12 2009
[snip]
> + List<String> countriesList = FastList.newInstance();
> + List<String> countriesAvailable =
> StringUtil
> .split(UtilProperties.getPropertyValue("general.properties",
> "countries.geo.id.available"), ",");
> + if (countriesAvailable != null) {
> + for(String country : countriesAvailable) {
> + GenericValue geoCountry = null;
> + try {
> + geoCountry = delegator.findOne("Geo",
> UtilMisc.toMap("geoId", country.trim()), true);
> + } catch (GenericEntityException e) {
> + Debug.logError(e, "The country specified into
> countries.geo.id.available does not exists in Geo", module);
> + }
> + if (geoCountry != null) {
> + countriesList.add(country);
> + }
> + }
> + }
You do realize that you are going through the list of countries and
grabbing each Geo just to make sure it exists and then in the next
block retrieving them all again? Somewhat pointless to have them all,
throw them away and then grab them again.
[snip]
> + /* remove the default geo to avoid double rows in
> the drop-down */
> + int idx = 0;
> + for (GenericValue geo : countryGeoList) {
> + if (geo.get("geoId") != null &&
> defaultGeo.get("geoId") != null &&
> +
> geo.get("geoId").equals(defaultGeo.get("geoId"))) {
> + countryGeoList.remove(idx);
> + }
> + idx += 1;
> + }
> geoList.addAll(countryGeoList);
> } else {
> geoList = countryGeoList;
Enhanced for loops are great but they're not really appropriate when
you do actually need to know the index during iteration.
Also the geoId null checks are somewhat superfluous considering that
geoId is the primary key.