Re: svn commit: r619566 - /ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ModelScreenWidget.java

[David E Jones]

This is an okay pattern Adrian, but one that is avoided in most parts  
of the framework.

The reason is caching and a balance between production performance and  
development convenience.

In production looking up a local field in an object is definitely  
faster than doing a Map lookup, but not by much. In fact you have to  
do it tens of thousands of times to make a noticeable difference, and  
that just doesn't happen when rendering screens or related artifacts.

The downside to this change is that the cache clearing won't cause the  
form defs to be reloaded any more, in production or development. In  
this case it's maybe not so bad because if we clear the screen cache  
AND the form cache (or have timeouts on both) then it will reload.

Even so, my vote would be revert this and avoid the pattern in the  
future in order to keep things flexible in the framework.

-David


On Feb 7, 2008, at 11:54 AM, [EMAIL PROTECTED] wrote:

Author: adrianc
Date: Thu Feb  7 10:54:07 2008
New Revision: 619566

URL: http://svn.apache.org/viewvc?rev=619566&view=rev
Log:
Small enhancement to the ModelScreenWidget class. Created member  
variables for ModelForm and ModelMenu, and provided accessor methods  
to retrieve those variables.

This should speed up screen rendering a bit.

Modified:
   ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ 
ModelScreenWidget.java

Modified: ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ 
ModelScreenWidget.java
URL: 
http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ModelScreenWidget.java?rev=619566&r1=619565&r2=619566&view=diff
= 
= 
= 
= 
= 
= 
= 
= 
======================================================================
--- ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ 
ModelScreenWidget.java (original)
+++ ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ 
ModelScreenWidget.java Thu Feb  7 10:54:07 2008
@@ -585,6 +585,7 @@
        protected FlexibleStringExpander nameExdr;
        protected FlexibleStringExpander locationExdr;
        protected FlexibleStringExpander shareScopeExdr;
+        protected ModelForm modelForm = null;

        public Form(ModelScreen modelScreen, Element formElement) {
            super(modelScreen, formElement);
@@ -603,25 +604,7 @@
                ((MapStack) context).push();
            }

-            String name = this.getName(context);
-            String location = this.getLocation(context);
-            ModelForm modelForm = null;
-            try {
-                modelForm =  
FormFactory.getFormFromLocation(this.getLocation(context),  
this.getName(context), this.modelScreen.getDelegator(context),  
this.modelScreen.getDispatcher(context));
-            } catch (IOException e) {
-                String errMsg = "Error rendering included form  
named [" + name + "] at location [" + location + "]: " + e.toString();
-                Debug.logError(e, errMsg, module);
-                throw new RuntimeException(errMsg);
-            } catch (SAXException e) {
-                String errMsg = "Error rendering included form  
named [" + name + "] at location [" + location + "]: " + e.toString();
-                Debug.logError(e, errMsg, module);
-                throw new RuntimeException(errMsg);
-            } catch (ParserConfigurationException e) {
-                String errMsg = "Error rendering included form  
named [" + name + "] at location [" + location + "]: " + e.toString();
-                Debug.logError(e, errMsg, module);
-                throw new RuntimeException(errMsg);
-            }
-
+            getModelForm(context);
            // try finding the formStringRenderer by name in the  
context in case one was prepared and put there
            FormStringRenderer formStringRenderer =  
(FormStringRenderer) context.get("formStringRenderer");
            // if there was no formStringRenderer put in place, now  
try finding the request/response in the context and creating a new one
@@ -641,7 +624,7 @@
            try {
                modelForm.renderFormString(writer, context,  
formStringRenderer);
            } catch (IOException e) {
-                String errMsg = "Error rendering included form  
named [" + name + "] at location [" + location + "]: " + e.toString();
+                String errMsg = "Error rendering included form  
named [" + name + "] at location [" + this.getLocation(context) +  
"]: " + e.toString();
                Debug.logError(e, errMsg, module);
                throw new RuntimeException(errMsg);
            }
@@ -651,6 +634,21 @@
            }
        }

+        public ModelForm getModelForm(Map context) {
+            if (this.modelForm == null) {
+                String name = this.getName(context);
+                String location = this.getLocation(context);
+                try {
+                    this.modelForm =  
FormFactory.getFormFromLocation(this.getLocation(context),  
this.getName(context), this.modelScreen.getDelegator(context),  
this.modelScreen.getDispatcher(context));
+                } catch (Exception e) {
+                    String errMsg = "Error rendering included form  
named [" + name + "] at location [" + location + "]: ";
+                    Debug.logError(e, errMsg, module);
+                    throw new RuntimeException(errMsg + e);
+                }
+            }
+            return this.modelForm;
+        }
+
        public String getName(Map context) {
            return this.nameExdr.expandString(context);
        }
@@ -993,6 +991,7 @@
    public static class Menu extends ModelScreenWidget {
        protected FlexibleStringExpander nameExdr;
        protected FlexibleStringExpander locationExdr;
+        protected ModelMenu modelMenu = null;

        public Menu(ModelScreen modelScreen, Element menuElement) {
            super(modelScreen, menuElement);
@@ -1002,25 +1001,7 @@
        }

        public void renderWidgetString(Writer writer, Map context,  
ScreenStringRenderer screenStringRenderer) {
-            String name = this.getName(context);
-            String location = this.getLocation(context);
-            ModelMenu modelMenu = null;
-            try {
-                modelMenu =  
MenuFactory.getMenuFromLocation(this.getLocation(context),  
this.getName(context), this.modelScreen.getDelegator(context),  
this.modelScreen.getDispatcher(context));
-            } catch (IOException e) {
-                String errMsg = "Error rendering included menu  
named [" + name + "] at location [" + location + "]: " + e.toString();
-                Debug.logError(e, errMsg, module);
-                throw new RuntimeException(errMsg);
-            } catch (SAXException e) {
-                String errMsg = "Error rendering included menu  
named [" + name + "] at location [" + location + "]: " + e.toString();
-                Debug.logError(e, errMsg, module);
-                throw new RuntimeException(errMsg);
-            } catch (ParserConfigurationException e) {
-                String errMsg = "Error rendering included menu  
named [" + name + "] at location [" + location + "]: " + e.toString();
-                Debug.logError(e, errMsg, module);
-                throw new RuntimeException(errMsg);
-            }
-
+            getModelMenu(context);
            // try finding the menuStringRenderer by name in the  
context in case one was prepared and put there
            MenuStringRenderer menuStringRenderer =  
(MenuStringRenderer) context.get("menuStringRenderer");
            // if there was no menuStringRenderer put in place, now  
try finding the request/response in the context and creating a new one
@@ -1037,16 +1018,31 @@
            }

            StringBuffer renderBuffer = new StringBuffer();
-            modelMenu.renderMenuString(renderBuffer, context,  
menuStringRenderer);
+            this.modelMenu.renderMenuString(renderBuffer, context,  
menuStringRenderer);
            try {
                writer.write(renderBuffer.toString());
            } catch (IOException e) {
-                String errMsg = "Error rendering included menu  
named [" + name + "] at location [" + location + "]: " + e.toString();
+                String errMsg = "Error rendering included menu  
named [" + name + "] at location [" + this.getLocation(context) +  
"]: " + e.toString();
                Debug.logError(e, errMsg, module);
                throw new RuntimeException(errMsg);
            }
        }

+        public ModelMenu getModelMenu(Map context) {
+            if (this.modelMenu == null) {
+                String name = this.getName(context);
+                String location = this.getLocation(context);
+                try {
+                    this.modelMenu =  
MenuFactory.getMenuFromLocation(this.getLocation(context),  
this.getName(context), this.modelScreen.getDelegator(context),  
this.modelScreen.getDispatcher(context));
+                } catch (Exception e) {
+                    String errMsg = "Error rendering included menu  
named [" + name + "] at location [" + location + "]: ";
+                    Debug.logError(e, errMsg, module);
+                    throw new RuntimeException(errMsg + e);
+                }
+            }
+            return this.modelMenu;
+        }
+
        public String getName(Map context) {
            return this.nameExdr.expandString(context);
        }