Hi Jacques, I did a first pass and completed most of them; but I know that there are still several areas with similar patterns and these could be enhanced as well with slightly different approaches.
Kind regards, Jacopo On Nov 10, 2012, at 10:30 AM, Jacques Le Roux wrote: > I'm currently reading "Java Concurrency in Practice" and I agree on these > change. I saw you continued at > http://svn.apache.org/viewvc?view=revision&revision=1343696, is that all? > > Jacques > > Jacopo Cappellato wrote: >> Hi devs, >> >> please review my commit below because I am working to apply a similar >> pattern to a lot of other classes in OFBiz. This effort is >> not a search-and-replace task but instead I am reviewing each individual >> usage of static UtilCache objects and refactoring (when >> necessary) the code to follow the pattern you see below. >> Here are some points to notice (feedback is highly appreciated because I am >> planning to use a similar approach on several other >> classes): 1) if possible, make the static fields that hold the reference to >> UtilCache a private and final field; I know this is >> not required but in my opinion makes the code easier to read and more secure >> 2) when useful, use the new method >> UtilCache.putIfAbsentAndGet that always returns a value (the reference of >> the object passed to it or the reference of the object >> found in the cache); of course this is not mandatory because I could use >> putIfAbsent and then use the reference of the passed >> object (if the returnedvalue is null, which means that it was added to the >> cache) but I think that putIfAbsentAndGet often makes >> the code slightly more readable 3) removed the synchronized block in favour >> of two calls to the thread safe UtilCache object: >> "get" and "putIfAbsentAndGet" (or "putIfAbsent") >> >> Kind regards, >> >> Jacopo >> >> Begin forwarded message: >> >>> From: [email protected] >>> Subject: svn commit: r1343277 - >>> /ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java >>> Date: May 28, 2012 5:02:39 PM GMT+02:00 >>> To: [email protected] >>> Reply-To: [email protected] >>> >>> Author: jacopoc >>> Date: Mon May 28 15:02:39 2012 >>> New Revision: 1343277 >>> >>> URL: http://svn.apache.org/viewvc?rev=1343277&view=rev >>> Log: >>> Improved code that manages the cache: >>> * removed unnecessary synchronization >>> * protected the UtilCache object (static field) by making it private and >>> final >>> >>> Modified: >>> >>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java >>> >>> Modified: >>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java >>> URL: >>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java?rev=1343277&r1=1343276&r2=1343277&view=diff >>> ============================================================================== >>> --- >>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java >>> (original) +++ >>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java >>> Mon May 28 15:02:39 2012 @@ -38,7 +38,7 @@ public >>> class RegionManager { >>> >>> public static final String module = RegionManager.class.getName(); >>> >>> - protected static UtilCache<URL, Map<String, Region>> regionCache = >>> UtilCache.createUtilCache("webapp.Regions.Config", 0, 0); >>> + private static final UtilCache<URL, Map<String, Region>> regionCache = >>> UtilCache.createUtilCache("webapp.Regions.Config", >>> 0, 0); >>> >>> protected URL regionFile = null; >>> >>> @@ -54,14 +54,8 @@ public class RegionManager { >>> public Map<String, Region> getRegions() { >>> Map<String, Region> regions = regionCache.get(regionFile); >>> if (regions == null) { >>> - synchronized (this) { >>> - regions = regionCache.get(regionFile); >>> - if (regions == null) { >>> - if (Debug.verboseOn()) Debug.logVerbose("Regions not >>> loaded for " + regionFile + ", loading now", module); >>> - regions = readRegionXml(regionFile); >>> - regionCache.put(regionFile, regions); >>> - } >>> - } >>> + if (Debug.verboseOn()) Debug.logVerbose("Regions not loaded >>> for " + regionFile + ", loading now", module); >>> + regions = regionCache.putIfAbsentAndGet(regionFile, >>> readRegionXml(regionFile)); >>> } >>> return regions; >>> }
