Posted by
Jacques Le Roux on
Nov 25, 2009; 10:49pm
URL: http://ofbiz.116.s1.nabble.com/Re-svn-commit-r883017-ofbiz-trunk-framework-common-src-org-ofbiz-common-CommonWorkers-java-tp717785p787907.html
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" <
[hidden email]>
> [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