Jacques,
didn't we just agreed upon a slower process and review from more committers when changing these core aspects of the framework? Especially when you change the patch there is no chance for anyone to review before it gets committed {#emotions_dlg.sad} Michael Am 29.06.18 um 12:03 schrieb [hidden email]: > Author: jleroux > Date: Fri Jun 29 10:03:22 2018 > New Revision: 1834662 > > URL: http://svn.apache.org/viewvc?rev=1834662&view=rev > Log: > Improved: Factorize code logic from ‘ConfigXMLReader’ > (OFBIZ-10453) > > There is a lot of repetitive code in ConfigXMLReader. > Using a functional interface as a parameter of a generic algorithm avoids those > repetitions. > > Thanks: Mathieu Lirzin > > Modified: > ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java > > Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java?rev=1834662&r1=1834661&r2=1834662&view=diff > ============================================================================== > --- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java (original) > +++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java Fri Jun 29 10:03:22 2018 > @@ -32,6 +32,7 @@ import java.util.List; > import java.util.Map; > import java.util.Objects; > import java.util.Set; > +import java.util.function.Function; > import java.util.stream.Collectors; > > import javax.servlet.ServletContext; > @@ -218,120 +219,67 @@ public class ConfigXMLReader { > } > } > > - public Map<String, Event> getAfterLoginEventList() throws WebAppConfigurationException { > - MapContext<String, Event> result = MapContext.getMapContext(); > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - result.push(controllerConfig.getAfterLoginEventList()); > + private <K, V> Map<K, V> pushIncludes(Function<ControllerConfig, Map<K, V>> f) throws WebAppConfigurationException { > + MapContext<K, V> res = MapContext.getMapContext(); > + for (URL include : includes) { > + res.push(getControllerConfig(include).pushIncludes(f)); > + } > + res.push(f.apply(this)); > + return res; > + } > + > + private String getIncludes(Function<ControllerConfig, String> f) throws WebAppConfigurationException { > + String val = f.apply(this); > + if (val != null) { > + return val; > + } > + for (URL include : includes) { > + String inc = getControllerConfig(include).getIncludes(f); > + if (inc != null) { > + return inc; > + } > } > - result.push(afterLoginEventList); > - return result; > + return null; > + } > + > + public Map<String, Event> getAfterLoginEventList() throws WebAppConfigurationException { > + return pushIncludes(ccfg -> ccfg.afterLoginEventList); > } > > public Map<String, Event> getBeforeLogoutEventList() throws WebAppConfigurationException { > - MapContext<String, Event> result = MapContext.getMapContext(); > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - result.push(controllerConfig.getBeforeLogoutEventList()); > - } > - result.push(beforeLogoutEventList); > - return result; > + return pushIncludes(ccfg -> ccfg.beforeLogoutEventList); > } > > public String getDefaultRequest() throws WebAppConfigurationException { > - if (defaultRequest != null) { > - return defaultRequest; > - } > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - String defaultRequest = controllerConfig.getDefaultRequest(); > - if (defaultRequest != null) { > - return defaultRequest; > - } > - } > - return null; > + return getIncludes(ccfg -> ccfg.defaultRequest); > } > > public String getErrorpage() throws WebAppConfigurationException { > - if (errorpage != null) { > - return errorpage; > - } > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - String errorpage = controllerConfig.getErrorpage(); > - if (errorpage != null) { > - return errorpage; > - } > - } > - return null; > + return getIncludes(ccfg -> ccfg.errorpage); > } > > public Map<String, String> getEventHandlerMap() throws WebAppConfigurationException { > - MapContext<String, String> result = MapContext.getMapContext(); > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - result.push(controllerConfig.getEventHandlerMap()); > - } > - result.push(eventHandlerMap); > - return result; > + return pushIncludes(ccfg -> ccfg.eventHandlerMap); > } > > public Map<String, Event> getFirstVisitEventList() throws WebAppConfigurationException { > - MapContext<String, Event> result = MapContext.getMapContext(); > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - result.push(controllerConfig.getFirstVisitEventList()); > - } > - result.push(firstVisitEventList); > - return result; > + return pushIncludes(ccfg -> ccfg.firstVisitEventList); > } > > public String getOwner() throws WebAppConfigurationException { > - if (owner != null) { > - return owner; > - } > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - String owner = controllerConfig.getOwner(); > - if (owner != null) { > - return owner; > - } > - } > - return null; > + return getIncludes(ccfg -> ccfg.owner); > } > > public Map<String, Event> getPostprocessorEventList() throws WebAppConfigurationException { > - MapContext<String, Event> result = MapContext.getMapContext(); > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - result.push(controllerConfig.getPostprocessorEventList()); > - } > - result.push(postprocessorEventList); > - return result; > + return pushIncludes(ccfg -> ccfg.postprocessorEventList); > } > > public Map<String, Event> getPreprocessorEventList() throws WebAppConfigurationException { > - MapContext<String, Event> result = MapContext.getMapContext(); > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - result.push(controllerConfig.getPreprocessorEventList()); > - } > - result.push(preprocessorEventList); > - return result; > + return pushIncludes(ccfg -> ccfg.preprocessorEventList); > } > > public String getProtectView() throws WebAppConfigurationException { > - if (protectView != null) { > - return protectView; > - } > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - String protectView = controllerConfig.getProtectView(); > - if (protectView != null) { > - return protectView; > - } > - } > - return null; > + return getIncludes(ccfg -> ccfg.protectView); > } > > // XXX: Temporary wrapper to handle conversion from Map to MultiMap. > @@ -391,51 +339,19 @@ public class ConfigXMLReader { > } > > public String getSecurityClass() throws WebAppConfigurationException { > - if (securityClass != null) { > - return securityClass; > - } > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - String securityClass = controllerConfig.getSecurityClass(); > - if (securityClass != null) { > - return securityClass; > - } > - } > - return null; > + return getIncludes(ccfg -> ccfg.securityClass); > } > > public String getStatusCode() throws WebAppConfigurationException { > - if (statusCode != null) { > - return statusCode; > - } > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - String statusCode = controllerConfig.getStatusCode(); > - if (statusCode != null) { > - return statusCode; > - } > - } > - return null; > + return getIncludes(ccfg -> ccfg.statusCode); > } > > public Map<String, String> getViewHandlerMap() throws WebAppConfigurationException { > - MapContext<String, String> result = MapContext.getMapContext(); > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - result.push(controllerConfig.getViewHandlerMap()); > - } > - result.push(viewHandlerMap); > - return result; > + return pushIncludes(ccfg -> ccfg.viewHandlerMap); > } > > public Map<String, ViewMap> getViewMapMap() throws WebAppConfigurationException { > - MapContext<String, ViewMap> result = MapContext.getMapContext(); > - for (URL includeLocation : includes) { > - ControllerConfig controllerConfig = getControllerConfig(includeLocation); > - result.push(controllerConfig.getViewMapMap()); > - } > - result.push(viewMapMap); > - return result; > + return pushIncludes(ccfg -> ccfg.viewMapMap); > } > > private void loadGeneralConfig(Element rootElement) { > > smime.p7s (5K) Download Attachment |
Administrator
|
Michael
It's not a change just a refactorization. I thought it was simple enough to be committed. If we go this way for simple and small changes like here things will be quite slow I also answered in the Jira since you also asked the same there Jacques Le 29/06/2018 à 12:21, Michael Brohl a écrit : > Jacques, > > didn't we just agreed upon a slower process and review from more committers when changing these core aspects of the framework? > > Especially when you change the patch there is no chance for anyone to review before it gets committed {#emotions_dlg.sad} > > Michael > > Am 29.06.18 um 12:03 schrieb [hidden email]: >> Author: jleroux >> Date: Fri Jun 29 10:03:22 2018 >> New Revision: 1834662 >> >> URL: http://svn.apache.org/viewvc?rev=1834662&view=rev >> Log: >> Improved: Factorize code logic from ‘ConfigXMLReader’ >> (OFBIZ-10453) >> >> There is a lot of repetitive code in ConfigXMLReader. >> Using a functional interface as a parameter of a generic algorithm avoids those >> repetitions. >> >> Thanks: Mathieu Lirzin >> >> Modified: >> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >> >> Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java?rev=1834662&r1=1834661&r2=1834662&view=diff >> ============================================================================== >> --- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java (original) >> +++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java Fri Jun 29 10:03:22 2018 >> @@ -32,6 +32,7 @@ import java.util.List; >> import java.util.Map; >> import java.util.Objects; >> import java.util.Set; >> +import java.util.function.Function; >> import java.util.stream.Collectors; >> import javax.servlet.ServletContext; >> @@ -218,120 +219,67 @@ public class ConfigXMLReader { >> } >> } >> - public Map<String, Event> getAfterLoginEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getAfterLoginEventList()); >> + private <K, V> Map<K, V> pushIncludes(Function<ControllerConfig, Map<K, V>> f) throws WebAppConfigurationException { >> + MapContext<K, V> res = MapContext.getMapContext(); >> + for (URL include : includes) { >> + res.push(getControllerConfig(include).pushIncludes(f)); >> + } >> + res.push(f.apply(this)); >> + return res; >> + } >> + >> + private String getIncludes(Function<ControllerConfig, String> f) throws WebAppConfigurationException { >> + String val = f.apply(this); >> + if (val != null) { >> + return val; >> + } >> + for (URL include : includes) { >> + String inc = getControllerConfig(include).getIncludes(f); >> + if (inc != null) { >> + return inc; >> + } >> } >> - result.push(afterLoginEventList); >> - return result; >> + return null; >> + } >> + >> + public Map<String, Event> getAfterLoginEventList() throws WebAppConfigurationException { >> + return pushIncludes(ccfg -> ccfg.afterLoginEventList); >> } >> public Map<String, Event> getBeforeLogoutEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getBeforeLogoutEventList()); >> - } >> - result.push(beforeLogoutEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.beforeLogoutEventList); >> } >> public String getDefaultRequest() throws WebAppConfigurationException { >> - if (defaultRequest != null) { >> - return defaultRequest; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String defaultRequest = controllerConfig.getDefaultRequest(); >> - if (defaultRequest != null) { >> - return defaultRequest; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.defaultRequest); >> } >> public String getErrorpage() throws WebAppConfigurationException { >> - if (errorpage != null) { >> - return errorpage; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String errorpage = controllerConfig.getErrorpage(); >> - if (errorpage != null) { >> - return errorpage; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.errorpage); >> } >> public Map<String, String> getEventHandlerMap() throws WebAppConfigurationException { >> - MapContext<String, String> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getEventHandlerMap()); >> - } >> - result.push(eventHandlerMap); >> - return result; >> + return pushIncludes(ccfg -> ccfg.eventHandlerMap); >> } >> public Map<String, Event> getFirstVisitEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getFirstVisitEventList()); >> - } >> - result.push(firstVisitEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.firstVisitEventList); >> } >> public String getOwner() throws WebAppConfigurationException { >> - if (owner != null) { >> - return owner; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String owner = controllerConfig.getOwner(); >> - if (owner != null) { >> - return owner; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.owner); >> } >> public Map<String, Event> getPostprocessorEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getPostprocessorEventList()); >> - } >> - result.push(postprocessorEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.postprocessorEventList); >> } >> public Map<String, Event> getPreprocessorEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getPreprocessorEventList()); >> - } >> - result.push(preprocessorEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.preprocessorEventList); >> } >> public String getProtectView() throws WebAppConfigurationException { >> - if (protectView != null) { >> - return protectView; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String protectView = controllerConfig.getProtectView(); >> - if (protectView != null) { >> - return protectView; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.protectView); >> } >> // XXX: Temporary wrapper to handle conversion from Map to MultiMap. >> @@ -391,51 +339,19 @@ public class ConfigXMLReader { >> } >> public String getSecurityClass() throws WebAppConfigurationException { >> - if (securityClass != null) { >> - return securityClass; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String securityClass = controllerConfig.getSecurityClass(); >> - if (securityClass != null) { >> - return securityClass; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.securityClass); >> } >> public String getStatusCode() throws WebAppConfigurationException { >> - if (statusCode != null) { >> - return statusCode; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String statusCode = controllerConfig.getStatusCode(); >> - if (statusCode != null) { >> - return statusCode; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.statusCode); >> } >> public Map<String, String> getViewHandlerMap() throws WebAppConfigurationException { >> - MapContext<String, String> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getViewHandlerMap()); >> - } >> - result.push(viewHandlerMap); >> - return result; >> + return pushIncludes(ccfg -> ccfg.viewHandlerMap); >> } >> public Map<String, ViewMap> getViewMapMap() throws WebAppConfigurationException { >> - MapContext<String, ViewMap> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getViewMapMap()); >> - } >> - result.push(viewMapMap); >> - return result; >> + return pushIncludes(ccfg -> ccfg.viewMapMap); >> } >> private void loadGeneralConfig(Element rootElement) { >> >> > > |
I refactored many parts of the framework. I might have done more
refactoring than anyone recently. However, I never just pushed a commit. I always ask for feedback, I make a patch and I consistently ask for people's feedback, some of that feedback came directly from you Jacques. So for you to call it just refactoring as if this makes it a benign this is surprising to me. I would join Michael in recommending that you revert and start a proper discussion. On Jun 29, 2018 1:29 PM, "Jacques Le Roux" <[hidden email]> wrote: Michael It's not a change just a refactorization. I thought it was simple enough to be committed. If we go this way for simple and small changes like here things will be quite slow I also answered in the Jira since you also asked the same there Jacques Le 29/06/2018 à 12:21, Michael Brohl a écrit : > Jacques, > > didn't we just agreed upon a slower process and review from more committers when changing these core aspects of the framework? > > Especially when you change the patch there is no chance for anyone to review before it gets committed {#emotions_dlg.sad} > > Michael > > Am 29.06.18 um 12:03 schrieb [hidden email]: >> Author: jleroux >> Date: Fri Jun 29 10:03:22 2018 >> New Revision: 1834662 >> >> URL: http://svn.apache.org/viewvc?rev=1834662&view=rev >> Log: >> Improved: Factorize code logic from ‘ConfigXMLReader’ >> (OFBIZ-10453) >> >> There is a lot of repetitive code in ConfigXMLReader. >> Using a functional interface as a parameter of a generic algorithm >> repetitions. >> >> Thanks: Mathieu Lirzin >> >> Modified: >> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >> >> Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java?rev=1834662&r1=1834661&r2=1834662&view=diff >> ============================================================================== >> --- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java (original) >> +++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java Fri Jun 29 10:03:22 2018 >> @@ -32,6 +32,7 @@ import java.util.List; >> import java.util.Map; >> import java.util.Objects; >> import java.util.Set; >> +import java.util.function.Function; >> import java.util.stream.Collectors; >> import javax.servlet.ServletContext; >> @@ -218,120 +219,67 @@ public class ConfigXMLReader { >> } >> } >> - public Map<String, Event> getAfterLoginEventList() throws >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getAfterLoginEventList()); >> + private <K, V> Map<K, V> pushIncludes(Function<ControllerConfig, Map<K, V>> f) throws WebAppConfigurationException { >> + MapContext<K, V> res = MapContext.getMapContext(); >> + for (URL include : includes) { >> + res.push(getControllerConfig(include).pushIncludes(f)); >> + } >> + res.push(f.apply(this)); >> + return res; >> + } >> + >> + private String getIncludes(Function<ControllerConfig, String> f) throws WebAppConfigurationException { >> + String val = f.apply(this); >> + if (val != null) { >> + return val; >> + } >> + for (URL include : includes) { >> + String inc = getControllerConfig(include).getIncludes(f); >> + if (inc != null) { >> + return inc; >> + } >> } >> - result.push(afterLoginEventList); >> - return result; >> + return null; >> + } >> + >> + public Map<String, Event> getAfterLoginEventList() throws >> + return pushIncludes(ccfg -> ccfg.afterLoginEventList); >> } >> public Map<String, Event> getBeforeLogoutEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getBeforeLogoutEventList()); >> - } >> - result.push(beforeLogoutEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.beforeLogoutEventList); >> } >> public String getDefaultRequest() throws WebAppConfigurationException { >> - if (defaultRequest != null) { >> - return defaultRequest; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String defaultRequest = controllerConfig.getDefaultRequest(); >> - if (defaultRequest != null) { >> - return defaultRequest; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.defaultRequest); >> } >> public String getErrorpage() throws WebAppConfigurationException { >> - if (errorpage != null) { >> - return errorpage; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String errorpage = controllerConfig.getErrorpage(); >> - if (errorpage != null) { >> - return errorpage; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.errorpage); >> } >> public Map<String, String> getEventHandlerMap() throws WebAppConfigurationException { >> - MapContext<String, String> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getEventHandlerMap()); >> - } >> - result.push(eventHandlerMap); >> - return result; >> + return pushIncludes(ccfg -> ccfg.eventHandlerMap); >> } >> public Map<String, Event> getFirstVisitEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getFirstVisitEventList()); >> - } >> - result.push(firstVisitEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.firstVisitEventList); >> } >> public String getOwner() throws WebAppConfigurationException { >> - if (owner != null) { >> - return owner; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String owner = controllerConfig.getOwner(); >> - if (owner != null) { >> - return owner; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.owner); >> } >> public Map<String, Event> getPostprocessorEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getPostprocessorEventList()); >> - } >> - result.push(postprocessorEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.postprocessorEventList); >> } >> public Map<String, Event> getPreprocessorEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getPreprocessorEventList()); >> - } >> - result.push(preprocessorEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.preprocessorEventList); >> } >> public String getProtectView() throws WebAppConfigurationException { >> - if (protectView != null) { >> - return protectView; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String protectView = controllerConfig.getProtectView(); >> - if (protectView != null) { >> - return protectView; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.protectView); >> } >> // XXX: Temporary wrapper to handle conversion from Map to MultiMap. >> @@ -391,51 +339,19 @@ public class ConfigXMLReader { >> } >> public String getSecurityClass() throws WebAppConfigurationException { >> - if (securityClass != null) { >> - return securityClass; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String securityClass = controllerConfig.getSecurityClass(); >> - if (securityClass != null) { >> - return securityClass; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.securityClass); >> } >> public String getStatusCode() throws WebAppConfigurationException { >> - if (statusCode != null) { >> - return statusCode; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String statusCode = controllerConfig.getStatusCode(); >> - if (statusCode != null) { >> - return statusCode; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.statusCode); >> } >> public Map<String, String> getViewHandlerMap() throws WebAppConfigurationException { >> - MapContext<String, String> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getViewHandlerMap()); >> - } >> - result.push(viewHandlerMap); >> - return result; >> + return pushIncludes(ccfg -> ccfg.viewHandlerMap); >> } >> public Map<String, ViewMap> getViewMapMap() throws WebAppConfigurationException { >> - MapContext<String, ViewMap> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getViewMapMap()); >> - } >> - result.push(viewMapMap); >> - return result; >> + return pushIncludes(ccfg -> ccfg.viewMapMap); >> } >> private void loadGeneralConfig(Element rootElement) { >> >> > > |
Administrator
|
I'm currently reviewing
http://svn.apache.org/viewvc?view=revision&revision=1834389 http://svn.apache.org/viewvc?view=revision&revision=1834465 I'll get back to this later, but I'm surprised that it would be an issue, it's just simple and fast to review... unlike above... Jacques Le 29/06/2018 à 16:11, Taher Alkhateeb a écrit : > I refactored many parts of the framework. I might have done more > refactoring than anyone recently. > > However, I never just pushed a commit. I always ask for feedback, I make a > patch and I consistently ask for people's feedback, some of that feedback > came directly from you Jacques. > > So for you to call it just refactoring as if this makes it a benign this is > surprising to me. > > I would join Michael in recommending that you revert and start a proper > discussion. > > On Jun 29, 2018 1:29 PM, "Jacques Le Roux" <[hidden email]> > wrote: > > Michael > > It's not a change just a refactorization. I thought it was simple enough to > be committed. If we go this way for simple and small changes like here > things will be quite slow > > I also answered in the Jira since you also asked the same there > > > Jacques > > > > Le 29/06/2018 à 12:21, Michael Brohl a écrit : >> Jacques, >> >> didn't we just agreed upon a slower process and review from more > committers when changing these core aspects of the framework? >> Especially when you change the patch there is no chance for anyone to > review before it gets committed {#emotions_dlg.sad} >> Michael >> >> Am 29.06.18 um 12:03 schrieb [hidden email]: >>> Author: jleroux >>> Date: Fri Jun 29 10:03:22 2018 >>> New Revision: 1834662 >>> >>> URL: http://svn.apache.org/viewvc?rev=1834662&view=rev >>> Log: >>> Improved: Factorize code logic from ‘ConfigXMLReader’ >>> (OFBIZ-10453) >>> >>> There is a lot of repetitive code in ConfigXMLReader. >>> Using a functional interface as a parameter of a generic algorithm > avoids those >>> repetitions. >>> >>> Thanks: Mathieu Lirzin >>> >>> Modified: >>> > ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >>> Modified: > ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >>> URL: >>> > http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java?rev=1834662&r1=1834661&r2=1834662&view=diff > ============================================================================== >>> --- > ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java > (original) >>> +++ > ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java > Fri Jun 29 10:03:22 2018 >>> @@ -32,6 +32,7 @@ import java.util.List; >>> import java.util.Map; >>> import java.util.Objects; >>> import java.util.Set; >>> +import java.util.function.Function; >>> import java.util.stream.Collectors; >>> import javax.servlet.ServletContext; >>> @@ -218,120 +219,67 @@ public class ConfigXMLReader { >>> } >>> } >>> - public Map<String, Event> getAfterLoginEventList() throws > WebAppConfigurationException { >>> - MapContext<String, Event> result = > MapContext.getMapContext(); >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - result.push(controllerConfig.getAfterLoginEventList()); >>> + private <K, V> Map<K, V> > pushIncludes(Function<ControllerConfig, Map<K, V>> f) throws > WebAppConfigurationException { >>> + MapContext<K, V> res = MapContext.getMapContext(); >>> + for (URL include : includes) { >>> + res.push(getControllerConfig(include).pushIncludes(f)); >>> + } >>> + res.push(f.apply(this)); >>> + return res; >>> + } >>> + >>> + private String getIncludes(Function<ControllerConfig, String> > f) throws WebAppConfigurationException { >>> + String val = f.apply(this); >>> + if (val != null) { >>> + return val; >>> + } >>> + for (URL include : includes) { >>> + String inc = > getControllerConfig(include).getIncludes(f); >>> + if (inc != null) { >>> + return inc; >>> + } >>> } >>> - result.push(afterLoginEventList); >>> - return result; >>> + return null; >>> + } >>> + >>> + public Map<String, Event> getAfterLoginEventList() throws > WebAppConfigurationException { >>> + return pushIncludes(ccfg -> ccfg.afterLoginEventList); >>> } >>> public Map<String, Event> getBeforeLogoutEventList() throws > WebAppConfigurationException { >>> - MapContext<String, Event> result = > MapContext.getMapContext(); >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - result.push(controllerConfig.getBeforeLogoutEventList()); >>> - } >>> - result.push(beforeLogoutEventList); >>> - return result; >>> + return pushIncludes(ccfg -> ccfg.beforeLogoutEventList); >>> } >>> public String getDefaultRequest() throws > WebAppConfigurationException { >>> - if (defaultRequest != null) { >>> - return defaultRequest; >>> - } >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - String defaultRequest = > controllerConfig.getDefaultRequest(); >>> - if (defaultRequest != null) { >>> - return defaultRequest; >>> - } >>> - } >>> - return null; >>> + return getIncludes(ccfg -> ccfg.defaultRequest); >>> } >>> public String getErrorpage() throws > WebAppConfigurationException { >>> - if (errorpage != null) { >>> - return errorpage; >>> - } >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - String errorpage = controllerConfig.getErrorpage(); >>> - if (errorpage != null) { >>> - return errorpage; >>> - } >>> - } >>> - return null; >>> + return getIncludes(ccfg -> ccfg.errorpage); >>> } >>> public Map<String, String> getEventHandlerMap() throws > WebAppConfigurationException { >>> - MapContext<String, String> result = > MapContext.getMapContext(); >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - result.push(controllerConfig.getEventHandlerMap()); >>> - } >>> - result.push(eventHandlerMap); >>> - return result; >>> + return pushIncludes(ccfg -> ccfg.eventHandlerMap); >>> } >>> public Map<String, Event> getFirstVisitEventList() throws > WebAppConfigurationException { >>> - MapContext<String, Event> result = > MapContext.getMapContext(); >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - result.push(controllerConfig.getFirstVisitEventList()); >>> - } >>> - result.push(firstVisitEventList); >>> - return result; >>> + return pushIncludes(ccfg -> ccfg.firstVisitEventList); >>> } >>> public String getOwner() throws WebAppConfigurationException > { >>> - if (owner != null) { >>> - return owner; >>> - } >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - String owner = controllerConfig.getOwner(); >>> - if (owner != null) { >>> - return owner; >>> - } >>> - } >>> - return null; >>> + return getIncludes(ccfg -> ccfg.owner); >>> } >>> public Map<String, Event> getPostprocessorEventList() throws > WebAppConfigurationException { >>> - MapContext<String, Event> result = > MapContext.getMapContext(); >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - result.push(controllerConfig.getPostprocessorEventList()); >>> - } >>> - result.push(postprocessorEventList); >>> - return result; >>> + return pushIncludes(ccfg -> ccfg.postprocessorEventList); >>> } >>> public Map<String, Event> getPreprocessorEventList() throws > WebAppConfigurationException { >>> - MapContext<String, Event> result = > MapContext.getMapContext(); >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - result.push(controllerConfig.getPreprocessorEventList()); >>> - } >>> - result.push(preprocessorEventList); >>> - return result; >>> + return pushIncludes(ccfg -> ccfg.preprocessorEventList); >>> } >>> public String getProtectView() throws > WebAppConfigurationException { >>> - if (protectView != null) { >>> - return protectView; >>> - } >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - String protectView = controllerConfig.getProtectView(); >>> - if (protectView != null) { >>> - return protectView; >>> - } >>> - } >>> - return null; >>> + return getIncludes(ccfg -> ccfg.protectView); >>> } >>> // XXX: Temporary wrapper to handle conversion from Map to > MultiMap. >>> @@ -391,51 +339,19 @@ public class ConfigXMLReader { >>> } >>> public String getSecurityClass() throws > WebAppConfigurationException { >>> - if (securityClass != null) { >>> - return securityClass; >>> - } >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - String securityClass = > controllerConfig.getSecurityClass(); >>> - if (securityClass != null) { >>> - return securityClass; >>> - } >>> - } >>> - return null; >>> + return getIncludes(ccfg -> ccfg.securityClass); >>> } >>> public String getStatusCode() throws > WebAppConfigurationException { >>> - if (statusCode != null) { >>> - return statusCode; >>> - } >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - String statusCode = controllerConfig.getStatusCode(); >>> - if (statusCode != null) { >>> - return statusCode; >>> - } >>> - } >>> - return null; >>> + return getIncludes(ccfg -> ccfg.statusCode); >>> } >>> public Map<String, String> getViewHandlerMap() throws > WebAppConfigurationException { >>> - MapContext<String, String> result = > MapContext.getMapContext(); >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - result.push(controllerConfig.getViewHandlerMap()); >>> - } >>> - result.push(viewHandlerMap); >>> - return result; >>> + return pushIncludes(ccfg -> ccfg.viewHandlerMap); >>> } >>> public Map<String, ViewMap> getViewMapMap() throws > WebAppConfigurationException { >>> - MapContext<String, ViewMap> result = > MapContext.getMapContext(); >>> - for (URL includeLocation : includes) { >>> - ControllerConfig controllerConfig = > getControllerConfig(includeLocation); >>> - result.push(controllerConfig.getViewMapMap()); >>> - } >>> - result.push(viewMapMap); >>> - return result; >>> + return pushIncludes(ccfg -> ccfg.viewMapMap); >>> } >>> private void loadGeneralConfig(Element rootElement) { >>> >>> >> |
Administrator
|
Hi Mathieu, All,
OK I reviewed those they sounds good to me, here are my remarks I did not know about XXX tag http://www.outpost9.com/reference/jargon/jargon_21.html#TAG637 funny private static final long serialVersionUID = -8765719278480440687L; In OFBiz we don't set serialVersionUID but let the system handles that for us and rather use @SuppressWarnings("serial") where necessary. More at https://markmail.org/message/jjhf5p5hbhon7vam and above. I'll try to write a word about that in our coding conventions or elsewhere? (please suggest) Not a big deal but I saw you often (automatically?) format length lines to 80 or so - private final String defaultStatusCodeString = UtilProperties.getPropertyValue("requestHandler", "status-code", "302"); + private final static String defaultStatusCodeString = + UtilProperties.getPropertyValue("requestHandler", "status-code", "302"); We have commonly decided to use a length of lines 120, see https://cwiki.apache.org/confluence/display/OFBIZ/Coding+Conventions https://svn.apache.org/repos/asf/ofbiz/tools/wiki-files/OFBizJavaFormatter.xml And thank you Jinghai for the comment about the service method <<I'm happy you returned to doGet/doPost, I guess you read this tomcat document (the service method tortured me for a while)>> It tortured me too reviewing :D. Good to know that you are already using Mathieu code. Let's document it now in this ML after the revert. To commit it again later... Jacques Le 29/06/2018 à 17:26, Jacques Le Roux a écrit : > I'm currently reviewing > > http://svn.apache.org/viewvc?view=revision&revision=1834389 > > http://svn.apache.org/viewvc?view=revision&revision=1834465 > > I'll get back to this later, but I'm surprised that it would be an issue, it's just simple and fast to review... unlike above... > > Jacques > > > Le 29/06/2018 à 16:11, Taher Alkhateeb a écrit : >> I refactored many parts of the framework. I might have done more >> refactoring than anyone recently. >> >> However, I never just pushed a commit. I always ask for feedback, I make a >> patch and I consistently ask for people's feedback, some of that feedback >> came directly from you Jacques. >> >> So for you to call it just refactoring as if this makes it a benign this is >> surprising to me. >> >> I would join Michael in recommending that you revert and start a proper >> discussion. >> >> On Jun 29, 2018 1:29 PM, "Jacques Le Roux" <[hidden email]> >> wrote: >> >> Michael >> >> It's not a change just a refactorization. I thought it was simple enough to >> be committed. If we go this way for simple and small changes like here >> things will be quite slow >> >> I also answered in the Jira since you also asked the same there >> >> >> Jacques >> >> >> >> Le 29/06/2018 à 12:21, Michael Brohl a écrit : >>> Jacques, >>> >>> didn't we just agreed upon a slower process and review from more >> committers when changing these core aspects of the framework? >>> Especially when you change the patch there is no chance for anyone to >> review before it gets committed {#emotions_dlg.sad} >>> Michael >>> >>> Am 29.06.18 um 12:03 schrieb [hidden email]: >>>> Author: jleroux >>>> Date: Fri Jun 29 10:03:22 2018 >>>> New Revision: 1834662 >>>> >>>> URL: http://svn.apache.org/viewvc?rev=1834662&view=rev >>>> Log: >>>> Improved: Factorize code logic from ‘ConfigXMLReader’ >>>> (OFBIZ-10453) >>>> >>>> There is a lot of repetitive code in ConfigXMLReader. >>>> Using a functional interface as a parameter of a generic algorithm >> avoids those >>>> repetitions. >>>> >>>> Thanks: Mathieu Lirzin >>>> >>>> Modified: >>>> >> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >>>> Modified: >> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >>>> URL: >>>> >> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java?rev=1834662&r1=1834661&r2=1834662&view=diff >> >> ============================================================================== >>>> --- >> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >> (original) >>>> +++ >> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >> Fri Jun 29 10:03:22 2018 >>>> @@ -32,6 +32,7 @@ import java.util.List; >>>> import java.util.Map; >>>> import java.util.Objects; >>>> import java.util.Set; >>>> +import java.util.function.Function; >>>> import java.util.stream.Collectors; >>>> import javax.servlet.ServletContext; >>>> @@ -218,120 +219,67 @@ public class ConfigXMLReader { >>>> } >>>> } >>>> - public Map<String, Event> getAfterLoginEventList() throws >> WebAppConfigurationException { >>>> - MapContext<String, Event> result = >> MapContext.getMapContext(); >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - result.push(controllerConfig.getAfterLoginEventList()); >>>> + private <K, V> Map<K, V> >> pushIncludes(Function<ControllerConfig, Map<K, V>> f) throws >> WebAppConfigurationException { >>>> + MapContext<K, V> res = MapContext.getMapContext(); >>>> + for (URL include : includes) { >>>> + res.push(getControllerConfig(include).pushIncludes(f)); >>>> + } >>>> + res.push(f.apply(this)); >>>> + return res; >>>> + } >>>> + >>>> + private String getIncludes(Function<ControllerConfig, String> >> f) throws WebAppConfigurationException { >>>> + String val = f.apply(this); >>>> + if (val != null) { >>>> + return val; >>>> + } >>>> + for (URL include : includes) { >>>> + String inc = >> getControllerConfig(include).getIncludes(f); >>>> + if (inc != null) { >>>> + return inc; >>>> + } >>>> } >>>> - result.push(afterLoginEventList); >>>> - return result; >>>> + return null; >>>> + } >>>> + >>>> + public Map<String, Event> getAfterLoginEventList() throws >> WebAppConfigurationException { >>>> + return pushIncludes(ccfg -> ccfg.afterLoginEventList); >>>> } >>>> public Map<String, Event> getBeforeLogoutEventList() throws >> WebAppConfigurationException { >>>> - MapContext<String, Event> result = >> MapContext.getMapContext(); >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - result.push(controllerConfig.getBeforeLogoutEventList()); >>>> - } >>>> - result.push(beforeLogoutEventList); >>>> - return result; >>>> + return pushIncludes(ccfg -> ccfg.beforeLogoutEventList); >>>> } >>>> public String getDefaultRequest() throws >> WebAppConfigurationException { >>>> - if (defaultRequest != null) { >>>> - return defaultRequest; >>>> - } >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - String defaultRequest = >> controllerConfig.getDefaultRequest(); >>>> - if (defaultRequest != null) { >>>> - return defaultRequest; >>>> - } >>>> - } >>>> - return null; >>>> + return getIncludes(ccfg -> ccfg.defaultRequest); >>>> } >>>> public String getErrorpage() throws >> WebAppConfigurationException { >>>> - if (errorpage != null) { >>>> - return errorpage; >>>> - } >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - String errorpage = controllerConfig.getErrorpage(); >>>> - if (errorpage != null) { >>>> - return errorpage; >>>> - } >>>> - } >>>> - return null; >>>> + return getIncludes(ccfg -> ccfg.errorpage); >>>> } >>>> public Map<String, String> getEventHandlerMap() throws >> WebAppConfigurationException { >>>> - MapContext<String, String> result = >> MapContext.getMapContext(); >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - result.push(controllerConfig.getEventHandlerMap()); >>>> - } >>>> - result.push(eventHandlerMap); >>>> - return result; >>>> + return pushIncludes(ccfg -> ccfg.eventHandlerMap); >>>> } >>>> public Map<String, Event> getFirstVisitEventList() throws >> WebAppConfigurationException { >>>> - MapContext<String, Event> result = >> MapContext.getMapContext(); >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - result.push(controllerConfig.getFirstVisitEventList()); >>>> - } >>>> - result.push(firstVisitEventList); >>>> - return result; >>>> + return pushIncludes(ccfg -> ccfg.firstVisitEventList); >>>> } >>>> public String getOwner() throws WebAppConfigurationException >> { >>>> - if (owner != null) { >>>> - return owner; >>>> - } >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - String owner = controllerConfig.getOwner(); >>>> - if (owner != null) { >>>> - return owner; >>>> - } >>>> - } >>>> - return null; >>>> + return getIncludes(ccfg -> ccfg.owner); >>>> } >>>> public Map<String, Event> getPostprocessorEventList() throws >> WebAppConfigurationException { >>>> - MapContext<String, Event> result = >> MapContext.getMapContext(); >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - result.push(controllerConfig.getPostprocessorEventList()); >>>> - } >>>> - result.push(postprocessorEventList); >>>> - return result; >>>> + return pushIncludes(ccfg -> ccfg.postprocessorEventList); >>>> } >>>> public Map<String, Event> getPreprocessorEventList() throws >> WebAppConfigurationException { >>>> - MapContext<String, Event> result = >> MapContext.getMapContext(); >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - result.push(controllerConfig.getPreprocessorEventList()); >>>> - } >>>> - result.push(preprocessorEventList); >>>> - return result; >>>> + return pushIncludes(ccfg -> ccfg.preprocessorEventList); >>>> } >>>> public String getProtectView() throws >> WebAppConfigurationException { >>>> - if (protectView != null) { >>>> - return protectView; >>>> - } >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - String protectView = controllerConfig.getProtectView(); >>>> - if (protectView != null) { >>>> - return protectView; >>>> - } >>>> - } >>>> - return null; >>>> + return getIncludes(ccfg -> ccfg.protectView); >>>> } >>>> // XXX: Temporary wrapper to handle conversion from Map to >> MultiMap. >>>> @@ -391,51 +339,19 @@ public class ConfigXMLReader { >>>> } >>>> public String getSecurityClass() throws >> WebAppConfigurationException { >>>> - if (securityClass != null) { >>>> - return securityClass; >>>> - } >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - String securityClass = >> controllerConfig.getSecurityClass(); >>>> - if (securityClass != null) { >>>> - return securityClass; >>>> - } >>>> - } >>>> - return null; >>>> + return getIncludes(ccfg -> ccfg.securityClass); >>>> } >>>> public String getStatusCode() throws >> WebAppConfigurationException { >>>> - if (statusCode != null) { >>>> - return statusCode; >>>> - } >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - String statusCode = controllerConfig.getStatusCode(); >>>> - if (statusCode != null) { >>>> - return statusCode; >>>> - } >>>> - } >>>> - return null; >>>> + return getIncludes(ccfg -> ccfg.statusCode); >>>> } >>>> public Map<String, String> getViewHandlerMap() throws >> WebAppConfigurationException { >>>> - MapContext<String, String> result = >> MapContext.getMapContext(); >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - result.push(controllerConfig.getViewHandlerMap()); >>>> - } >>>> - result.push(viewHandlerMap); >>>> - return result; >>>> + return pushIncludes(ccfg -> ccfg.viewHandlerMap); >>>> } >>>> public Map<String, ViewMap> getViewMapMap() throws >> WebAppConfigurationException { >>>> - MapContext<String, ViewMap> result = >> MapContext.getMapContext(); >>>> - for (URL includeLocation : includes) { >>>> - ControllerConfig controllerConfig = >> getControllerConfig(includeLocation); >>>> - result.push(controllerConfig.getViewMapMap()); >>>> - } >>>> - result.push(viewMapMap); >>>> - return result; >>>> + return pushIncludes(ccfg -> ccfg.viewMapMap); >>>> } >>>> private void loadGeneralConfig(Element rootElement) { >>>> >>>> >>> > > |
Hello Jacques,
Jacques Le Roux <[hidden email]> writes: > OK I reviewed those they sounds good to me, here are my remarks > > I did not know about XXX tag http://www.outpost9.com/reference/jargon/jargon_21.html#TAG637 funny I learned it from my fellow GNU hackers. ;-) > private static final long serialVersionUID = -8765719278480440687L; > In OFBiz we don't set serialVersionUID but let the system handles that for us and rather use @SuppressWarnings("serial") where necessary. > More at https://markmail.org/message/jjhf5p5hbhon7vam and above. I'll > try to write a word about that in our coding conventions or elsewhere? I agree this should go in OFBiz coding conventions. Maybe this goes beyond your question but it would be great if the contributing/coding guidelines was included in the OFBiz developer manual instead of the Wiki. > Not a big deal but I saw you often (automatically?) format length lines to 80 or so > - private final String defaultStatusCodeString = UtilProperties.getPropertyValue("requestHandler", "status-code", "302"); > + private final static String defaultStatusCodeString = > + UtilProperties.getPropertyValue("requestHandler", "status-code", "302"); > We have commonly decided to use a length of lines 120, see > https://cwiki.apache.org/confluence/display/OFBIZ/Coding+Conventions > https://svn.apache.org/repos/asf/ofbiz/tools/wiki-files/OFBizJavaFormatter.xml OK I will adjust my code. Maybe the line length of 120 could be explicitly stated as an exception to the Sun standard [1]? Thanks. [1] http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-136091.html#313 -- Mathieu Lirzin GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37 |
Administrator
|
Hi Mathieu,
Le 03/07/2018 à 14:53, Mathieu Lirzin a écrit : > Hello Jacques, > > Jacques Le Roux <[hidden email]> writes: [snip] >> private static final long serialVersionUID = -8765719278480440687L; >> In OFBiz we don't set serialVersionUID but let the system handles that for us and rather use @SuppressWarnings("serial") where necessary. >> More at https://markmail.org/message/jjhf5p5hbhon7vam and above. I'll >> try to write a word about that in our coding conventions or elsewhere? > I agree this should go in OFBiz coding conventions. Maybe this goes > beyond your question but it would be great if the contributing/coding > guidelines was included in the OFBiz developer manual instead of the > Wiki. I'll put it in the "coding conventions" page for now. I agree this page should better be in developer manual, I'll consider that in future, thanks for the idea. >> Not a big deal but I saw you often (automatically?) format length lines to 80 or so >> - private final String defaultStatusCodeString = UtilProperties.getPropertyValue("requestHandler", "status-code", "302"); >> + private final static String defaultStatusCodeString = >> + UtilProperties.getPropertyValue("requestHandler", "status-code", "302"); >> We have commonly decided to use a length of lines 120, see >> https://cwiki.apache.org/confluence/display/OFBIZ/Coding+Conventions >> https://svn.apache.org/repos/asf/ofbiz/tools/wiki-files/OFBizJavaFormatter.xml > OK I will adjust my code. Maybe the line length of 120 could be > explicitly stated as an exception to the Sun standard [1]? Yes, good idea, I'll do so Thanks > > Thanks. > > [1] http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-136091.html#313 > |
Administrator
|
Le 05/07/2018 à 17:41, Jacques Le Roux a écrit :
> Yes, good idea, I'll do so > > Thanks Done Jacques |
Jacques Le Roux <[hidden email]> writes:
> Le 05/07/2018 à 17:41, Jacques Le Roux a écrit : >> Yes, good idea, I'll do so >> >> Thanks > Done Thanks Jacques. -- Mathieu Lirzin GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37 |
Free forum by Nabble | Edit this page |