Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
Locked
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
 
|
The sentence:
"obviously a PatternSyntaxException should not occur here" doesn't add any useful information and instead it seems confusing to me. What are you trying to convey? Jacopo On Tue, Mar 28, 2017 at 5:26 PM, <[hidden email]> wrote: > Author: jleroux > Date: Tue Mar 28 15:26:02 2017 > New Revision: 1789163 > > URL: http://svn.apache.org/viewvc?rev=1789163&view=rev > Log: > Fixed: Fix Default or Empty Catch block in Java files > (OFBIZ-8341) > > Obviously a PatternSyntaxException should not occur there. > So I simply put a comment to document the fact even it it seems obvious. > > I note though that the repeated pattern is a smell for refactoring... > > Modified: > ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/AbstractModelAction.java > ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/ModelFormAction.java > ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/ModelTreeAction.java > > Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/AbstractModelAction.java > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ > framework/widget/src/main/java/org/apache/ofbiz/widget/ > model/AbstractModelAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff > ============================================================ > ================== > --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/AbstractModelAction.java (original) > +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/AbstractModelAction.java Tue Mar 28 > 15:26:02 2017 > @@ -715,7 +715,7 @@ public abstract class AbstractModelActio > String queryStringEncoded = > queryString.replaceAll("&", "%26"); > context.put("queryStringEncoded", > queryStringEncoded); > } catch (PatternSyntaxException e) { > - > + // obviously a PatternSyntaxException should not > occur here > } > } > } else { > > Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/ModelFormAction.java > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ > framework/widget/src/main/java/org/apache/ofbiz/widget/ > model/ModelFormAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff > ============================================================ > ================== > --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/ModelFormAction.java (original) > +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/ModelFormAction.java Tue Mar 28 > 15:26:02 2017 > @@ -218,7 +218,7 @@ public abstract class ModelFormAction { > String queryStringEncoded = > queryString.replaceAll("&", "%26"); > context.put("queryStringEncoded", > queryStringEncoded); > } catch (PatternSyntaxException e) { > - > + // obviously a PatternSyntaxException should not > occur here > } > } > } else { > > Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/ModelTreeAction.java > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ > framework/widget/src/main/java/org/apache/ofbiz/widget/ > model/ModelTreeAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff > ============================================================ > ================== > --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/ModelTreeAction.java (original) > +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ > java/org/apache/ofbiz/widget/model/ModelTreeAction.java Tue Mar 28 > 15:26:02 2017 > @@ -417,7 +417,7 @@ public abstract class ModelTreeAction ex > String queryStringEncoded = > queryString.replaceAll("&", "%26"); > context.put("queryStringEncoded", > queryStringEncoded); > } catch (PatternSyntaxException e) { > - > + // obviously a PatternSyntaxException should > not occur here > } > } > } else { > > > |
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
Administrator
|
Yikes, I thought it was clear. I mean that people should not be worried about this swallowed exception because it's intended since no
PatternSyntaxException should not occur there I think it's better to say something than letting the catch empty. Maybe what I said is not what I wanted to say and can be understood in another way? What did you understand? What would you say? Jacques Le 28/03/2017 à 17:41, Jacopo Cappellato a écrit : > The sentence: > > "obviously a PatternSyntaxException should not occur here" > > doesn't add any useful information and instead it seems confusing to me. > What are you trying to convey? > > Jacopo > > > > On Tue, Mar 28, 2017 at 5:26 PM, <[hidden email]> wrote: > >> Author: jleroux >> Date: Tue Mar 28 15:26:02 2017 >> New Revision: 1789163 >> >> URL: http://svn.apache.org/viewvc?rev=1789163&view=rev >> Log: >> Fixed: Fix Default or Empty Catch block in Java files >> (OFBIZ-8341) >> >> Obviously a PatternSyntaxException should not occur there. >> So I simply put a comment to document the fact even it it seems obvious. >> >> I note though that the repeated pattern is a smell for refactoring... >> >> Modified: >> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/ModelFormAction.java >> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >> >> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >> framework/widget/src/main/java/org/apache/ofbiz/widget/ >> model/AbstractModelAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >> ============================================================ >> ================== >> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/AbstractModelAction.java (original) >> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/AbstractModelAction.java Tue Mar 28 >> 15:26:02 2017 >> @@ -715,7 +715,7 @@ public abstract class AbstractModelActio >> String queryStringEncoded = >> queryString.replaceAll("&", "%26"); >> context.put("queryStringEncoded", >> queryStringEncoded); >> } catch (PatternSyntaxException e) { >> - >> + // obviously a PatternSyntaxException should not >> occur here >> } >> } >> } else { >> >> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/ModelFormAction.java >> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >> framework/widget/src/main/java/org/apache/ofbiz/widget/ >> model/ModelFormAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >> ============================================================ >> ================== >> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/ModelFormAction.java (original) >> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/ModelFormAction.java Tue Mar 28 >> 15:26:02 2017 >> @@ -218,7 +218,7 @@ public abstract class ModelFormAction { >> String queryStringEncoded = >> queryString.replaceAll("&", "%26"); >> context.put("queryStringEncoded", >> queryStringEncoded); >> } catch (PatternSyntaxException e) { >> - >> + // obviously a PatternSyntaxException should not >> occur here >> } >> } >> } else { >> >> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >> framework/widget/src/main/java/org/apache/ofbiz/widget/ >> model/ModelTreeAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >> ============================================================ >> ================== >> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/ModelTreeAction.java (original) >> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >> java/org/apache/ofbiz/widget/model/ModelTreeAction.java Tue Mar 28 >> 15:26:02 2017 >> @@ -417,7 +417,7 @@ public abstract class ModelTreeAction ex >> String queryStringEncoded = >> queryString.replaceAll("&", "%26"); >> context.put("queryStringEncoded", >> queryStringEncoded); >> } catch (PatternSyntaxException e) { >> - >> + // obviously a PatternSyntaxException should >> not occur here >> } >> } >> } else { >> >> >> |
|
You can actually remove the entire try/catch blocks because
PatternSyntaxException is a RuntimeException. So the whole thing can be removed because it's not checked. However, if we intentionally added a try/catch on a runtime exception then this means we expect it to happen and therefore we should do something if that happens (when calling replaceAll() I guess). So I think in either case the comment provided is not a solution because it does not add clarity. We must investigate _why_ the try/catch was introduced in the first place and whether it should be removed altogether. Also this code smells of copy-paste pattern. It could be improved and to remove this duplication which violates DRY IMO. On Tue, Mar 28, 2017 at 8:38 PM, Jacques Le Roux < [hidden email]> wrote: > Yikes, I thought it was clear. I mean that people should not be worried > about this swallowed exception because it's intended since no > PatternSyntaxException should not occur there > > I think it's better to say something than letting the catch empty. Maybe > what I said is not what I wanted to say and can be understood in another > way? What did you understand? > > What would you say? > > Jacques > > > > Le 28/03/2017 à 17:41, Jacopo Cappellato a écrit : > >> The sentence: >> >> "obviously a PatternSyntaxException should not occur here" >> >> doesn't add any useful information and instead it seems confusing to me. >> What are you trying to convey? >> >> Jacopo >> >> >> >> On Tue, Mar 28, 2017 at 5:26 PM, <[hidden email]> wrote: >> >> Author: jleroux >>> Date: Tue Mar 28 15:26:02 2017 >>> New Revision: 1789163 >>> >>> URL: http://svn.apache.org/viewvc?rev=1789163&view=rev >>> Log: >>> Fixed: Fix Default or Empty Catch block in Java files >>> (OFBIZ-8341) >>> >>> Obviously a PatternSyntaxException should not occur there. >>> So I simply put a comment to document the fact even it it seems obvious. >>> >>> I note though that the repeated pattern is a smell for refactoring... >>> >>> Modified: >>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/ModelFormAction.java >>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >>> >>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>> model/AbstractModelAction.java?rev=1789163&r1=1789162&r2= >>> 1789163&view=diff >>> ============================================================ >>> ================== >>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java (original) >>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java Tue Mar 28 >>> 15:26:02 2017 >>> @@ -715,7 +715,7 @@ public abstract class AbstractModelActio >>> String queryStringEncoded = >>> queryString.replaceAll("&", "%26"); >>> context.put("queryStringEncoded", >>> queryStringEncoded); >>> } catch (PatternSyntaxException e) { >>> - >>> + // obviously a PatternSyntaxException should >>> not >>> occur here >>> } >>> } >>> } else { >>> >>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/ModelFormAction.java >>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>> model/ModelFormAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >>> ============================================================ >>> ================== >>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/ModelFormAction.java (original) >>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/ModelFormAction.java Tue Mar 28 >>> 15:26:02 2017 >>> @@ -218,7 +218,7 @@ public abstract class ModelFormAction { >>> String queryStringEncoded = >>> queryString.replaceAll("&", "%26"); >>> context.put("queryStringEncoded", >>> queryStringEncoded); >>> } catch (PatternSyntaxException e) { >>> - >>> + // obviously a PatternSyntaxException should >>> not >>> occur here >>> } >>> } >>> } else { >>> >>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>> model/ModelTreeAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >>> ============================================================ >>> ================== >>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java (original) >>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java Tue Mar 28 >>> 15:26:02 2017 >>> @@ -417,7 +417,7 @@ public abstract class ModelTreeAction ex >>> String queryStringEncoded = >>> queryString.replaceAll("&", "%26"); >>> context.put("queryStringEncoded", >>> queryStringEncoded); >>> } catch (PatternSyntaxException e) { >>> - >>> + // obviously a PatternSyntaxException should >>> not occur here >>> } >>> } >>> } else { >>> >>> >>> >>> > |
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
|
Administrator
|
Le 28/03/2017 à 20:04, Taher Alkhateeb a écrit :
> You can actually remove the entire try/catch blocks because > PatternSyntaxException is a RuntimeException. So the whole thing can be > removed because it's not checked. However, if we intentionally added a > try/catch on a runtime exception then this means we expect it to happen and > therefore we should do something if that happens (when calling replaceAll() > I guess). So I think in either case the comment provided is not a solution > because it does not add clarity. We must investigate _why_ the try/catch > was introduced in the first place and whether it should be removed > altogether. Thanks for your analysis. Sincerely I have no real ideas of why it's there. It's pre Apache era so like more than 10 years ago. Maybe while creating this code the exception was thrown and the committer forgot to remove the catch after fixing the regexp? Or it was added automatically by an IDE then? Anyway, I see no needs for this catch: "&" can't be wrong as a Java regexp. It's not a reserved chars https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html BTW we have 72 other ".replaceAll(", and none catch PatternSyntaxException. So I think it's OK to remove it... > Also this code smells of copy-paste pattern. It could be improved and to > remove this duplication which violates DRY IMO. While reviewing I created https://issues.apache.org/jira/browse/OFBIZ-9287 for that Jacques > > On Tue, Mar 28, 2017 at 8:38 PM, Jacques Le Roux < > [hidden email]> wrote: > >> Yikes, I thought it was clear. I mean that people should not be worried >> about this swallowed exception because it's intended since no >> PatternSyntaxException should not occur there >> >> I think it's better to say something than letting the catch empty. Maybe >> what I said is not what I wanted to say and can be understood in another >> way? What did you understand? >> >> What would you say? >> >> Jacques >> >> >> >> Le 28/03/2017 à 17:41, Jacopo Cappellato a écrit : >> >>> The sentence: >>> >>> "obviously a PatternSyntaxException should not occur here" >>> >>> doesn't add any useful information and instead it seems confusing to me. >>> What are you trying to convey? >>> >>> Jacopo >>> >>> >>> >>> On Tue, Mar 28, 2017 at 5:26 PM, <[hidden email]> wrote: >>> >>> Author: jleroux >>>> Date: Tue Mar 28 15:26:02 2017 >>>> New Revision: 1789163 >>>> >>>> URL: http://svn.apache.org/viewvc?rev=1789163&view=rev >>>> Log: >>>> Fixed: Fix Default or Empty Catch block in Java files >>>> (OFBIZ-8341) >>>> >>>> Obviously a PatternSyntaxException should not occur there. >>>> So I simply put a comment to document the fact even it it seems obvious. >>>> >>>> I note though that the repeated pattern is a smell for refactoring... >>>> >>>> Modified: >>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java >>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >>>> >>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>> model/AbstractModelAction.java?rev=1789163&r1=1789162&r2= >>>> 1789163&view=diff >>>> ============================================================ >>>> ================== >>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java (original) >>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java Tue Mar 28 >>>> 15:26:02 2017 >>>> @@ -715,7 +715,7 @@ public abstract class AbstractModelActio >>>> String queryStringEncoded = >>>> queryString.replaceAll("&", "%26"); >>>> context.put("queryStringEncoded", >>>> queryStringEncoded); >>>> } catch (PatternSyntaxException e) { >>>> - >>>> + // obviously a PatternSyntaxException should >>>> not >>>> occur here >>>> } >>>> } >>>> } else { >>>> >>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java >>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>> model/ModelFormAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >>>> ============================================================ >>>> ================== >>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java (original) >>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java Tue Mar 28 >>>> 15:26:02 2017 >>>> @@ -218,7 +218,7 @@ public abstract class ModelFormAction { >>>> String queryStringEncoded = >>>> queryString.replaceAll("&", "%26"); >>>> context.put("queryStringEncoded", >>>> queryStringEncoded); >>>> } catch (PatternSyntaxException e) { >>>> - >>>> + // obviously a PatternSyntaxException should >>>> not >>>> occur here >>>> } >>>> } >>>> } else { >>>> >>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>> model/ModelTreeAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >>>> ============================================================ >>>> ================== >>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java (original) >>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java Tue Mar 28 >>>> 15:26:02 2017 >>>> @@ -417,7 +417,7 @@ public abstract class ModelTreeAction ex >>>> String queryStringEncoded = >>>> queryString.replaceAll("&", "%26"); >>>> context.put("queryStringEncoded", >>>> queryStringEncoded); >>>> } catch (PatternSyntaxException e) { >>>> - >>>> + // obviously a PatternSyntaxException should >>>> not occur here >>>> } >>>> } >>>> } else { >>>> >>>> >>>> >>>> |
|
|
Okay so your reply is making a point. If you were not entirely sure about
the purpose of the exception handling block, and whether there is a need for the replaceAll call, then why did you commit this comment? Why introduce this confusion on what is essentially already confusing and old code that requires some pruning. It would have been more beneficial and less time consuming for all of us if you did a thorough investigation of the code. And it seems you _did_ that after comments from Jacopo and myself which is why you reached the conclusions in your latest post in here right? My recommendation is to minimize these _bulk_ commits where you identify some problem and try to solve it _everywhere_. So example for such bulk commits: - Let's put try-with-resources everywhere - Let's put this certain comment everywhere - Let's put an annotation to suppress warnings everywhere - Let's Convert all objects of a certain type to another type everywhere Doing bulk commits introduces the risk of making errors because code does not work exactly the same way everywhere. Everything has context and should be studied carefully. So, my 2 cents. On Tue, Mar 28, 2017 at 11:17 PM, Jacques Le Roux < [hidden email]> wrote: > Le 28/03/2017 à 20:04, Taher Alkhateeb a écrit : > >> You can actually remove the entire try/catch blocks because >> PatternSyntaxException is a RuntimeException. So the whole thing can be >> removed because it's not checked. However, if we intentionally added a >> try/catch on a runtime exception then this means we expect it to happen >> and >> therefore we should do something if that happens (when calling >> replaceAll() >> I guess). So I think in either case the comment provided is not a solution >> because it does not add clarity. We must investigate _why_ the try/catch >> was introduced in the first place and whether it should be removed >> altogether. >> > Thanks for your analysis. > > Sincerely I have no real ideas of why it's there. It's pre Apache era so > like more than 10 years ago. > Maybe while creating this code the exception was thrown and the committer > forgot to remove the catch after fixing the regexp? Or it was added > automatically by an IDE then? > Anyway, I see no needs for this catch: "&" can't be wrong as a Java > regexp. It's not a reserved chars https://docs.oracle.com/javase > /8/docs/api/java/util/regex/Pattern.html > BTW we have 72 other ".replaceAll(", and none catch > PatternSyntaxException. So I think it's OK to remove it... > >> Also this code smells of copy-paste pattern. It could be improved and to >> remove this duplication which violates DRY IMO. >> > While reviewing I created https://issues.apache.org/jira/browse/OFBIZ-9287 > for that > > Jacques > > >> On Tue, Mar 28, 2017 at 8:38 PM, Jacques Le Roux < >> [hidden email]> wrote: >> >> Yikes, I thought it was clear. I mean that people should not be worried >>> about this swallowed exception because it's intended since no >>> PatternSyntaxException should not occur there >>> >>> I think it's better to say something than letting the catch empty. Maybe >>> what I said is not what I wanted to say and can be understood in another >>> way? What did you understand? >>> >>> What would you say? >>> >>> Jacques >>> >>> >>> >>> Le 28/03/2017 à 17:41, Jacopo Cappellato a écrit : >>> >>> The sentence: >>>> >>>> "obviously a PatternSyntaxException should not occur here" >>>> >>>> doesn't add any useful information and instead it seems confusing to me. >>>> What are you trying to convey? >>>> >>>> Jacopo >>>> >>>> >>>> >>>> On Tue, Mar 28, 2017 at 5:26 PM, <[hidden email]> wrote: >>>> >>>> Author: jleroux >>>> >>>>> Date: Tue Mar 28 15:26:02 2017 >>>>> New Revision: 1789163 >>>>> >>>>> URL: http://svn.apache.org/viewvc?rev=1789163&view=rev >>>>> Log: >>>>> Fixed: Fix Default or Empty Catch block in Java files >>>>> (OFBIZ-8341) >>>>> >>>>> Obviously a PatternSyntaxException should not occur there. >>>>> So I simply put a comment to document the fact even it it seems >>>>> obvious. >>>>> >>>>> I note though that the repeated pattern is a smell for refactoring... >>>>> >>>>> Modified: >>>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java >>>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >>>>> >>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>>> model/AbstractModelAction.java?rev=1789163&r1=1789162&r2= >>>>> 1789163&view=diff >>>>> ============================================================ >>>>> ================== >>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java (original) >>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java Tue Mar 28 >>>>> 15:26:02 2017 >>>>> @@ -715,7 +715,7 @@ public abstract class AbstractModelActio >>>>> String queryStringEncoded = >>>>> queryString.replaceAll("&", "%26"); >>>>> context.put("queryStringEncoded", >>>>> queryStringEncoded); >>>>> } catch (PatternSyntaxException e) { >>>>> - >>>>> + // obviously a PatternSyntaxException should >>>>> not >>>>> occur here >>>>> } >>>>> } >>>>> } else { >>>>> >>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java >>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>>> model/ModelFormAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >>>>> ============================================================ >>>>> ================== >>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java (original) >>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java Tue Mar 28 >>>>> 15:26:02 2017 >>>>> @@ -218,7 +218,7 @@ public abstract class ModelFormAction { >>>>> String queryStringEncoded = >>>>> queryString.replaceAll("&", "%26"); >>>>> context.put("queryStringEncoded", >>>>> queryStringEncoded); >>>>> } catch (PatternSyntaxException e) { >>>>> - >>>>> + // obviously a PatternSyntaxException should >>>>> not >>>>> occur here >>>>> } >>>>> } >>>>> } else { >>>>> >>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>>> model/ModelTreeAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >>>>> ============================================================ >>>>> ================== >>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java (original) >>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java Tue Mar 28 >>>>> 15:26:02 2017 >>>>> @@ -417,7 +417,7 @@ public abstract class ModelTreeAction ex >>>>> String queryStringEncoded = >>>>> queryString.replaceAll("&", "%26"); >>>>> context.put("queryStringEncoded", >>>>> queryStringEncoded); >>>>> } catch (PatternSyntaxException e) { >>>>> - >>>>> + // obviously a PatternSyntaxException >>>>> should >>>>> not occur here >>>>> } >>>>> } >>>>> } else { >>>>> >>>>> >>>>> >>>>> >>>>> > |
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
|
|
In reply to this post by Jacques Le Roux
On Tue, Mar 28, 2017 at 7:38 PM, Jacques Le Roux <
[hidden email]> wrote: > Yikes, I thought it was clear. I mean that people should not be worried > about this swallowed exception because it's intended since no > PatternSyntaxException should not occur there > I can confirm that the comment you have added is not clear, and even the explanatory sentence you wrote above is confusing. > I think it's better to say something than letting the catch empty. It is better to add *useful* information but if you are adding a comment that is stating the obvious then it is better to leave it empty. > Maybe what I said is not what I wanted to say and can be understood in > another way? What did you understand? > When I read your comment: "obviously a PatternSyntaxException should not occur here" I think: "hmmmm... it doesn't seem obvious to me that such an exception should not occur... but if the committer stated it is *obvious* then it means I am missing something" Instead, at least based on your reply to Taher, it seems that you ignore why the catch block was added. > > What would you say? I would either explain why the code is swallowing the runtime exception or remove your comment. Jacopo |
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
|
Administrator
|
I think we can all agree to remove it
Jacques Le 29/03/2017 à 09:45, Jacopo Cappellato a écrit : > On Tue, Mar 28, 2017 at 7:38 PM, Jacques Le Roux < > [hidden email]> wrote: > >> Yikes, I thought it was clear. I mean that people should not be worried >> about this swallowed exception because it's intended since no >> PatternSyntaxException should not occur there >> > I can confirm that the comment you have added is not clear, and even the > explanatory sentence you wrote above is confusing. > > >> I think it's better to say something than letting the catch empty. > > It is better to add *useful* information but if you are adding a comment > that is stating the obvious then it is better to leave it empty. > > >> Maybe what I said is not what I wanted to say and can be understood in >> another way? What did you understand? >> > When I read your comment: > "obviously a PatternSyntaxException should not occur here" > > I think: "hmmmm... it doesn't seem obvious to me that such an exception > should not occur... but if the committer stated it is *obvious* then it > means I am missing something" > > Instead, at least based on your reply to Taher, it seems that you ignore > why the catch block was added. > > >> What would you say? > > I would either explain why the code is swallowing the runtime exception or > remove your comment. > > Jacopo > |
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
|
Administrator
|
Mmm, maybe I was not totally clear if you did not read my answer to Taher.
I meant to remove the useless try and catch. We have 70+ other cases like that w/o try and catch Jacques Le 29/03/2017 à 11:01, Jacques Le Roux a écrit : > I think we can all agree to remove it > > Jacques > > > Le 29/03/2017 à 09:45, Jacopo Cappellato a écrit : >> On Tue, Mar 28, 2017 at 7:38 PM, Jacques Le Roux < >> [hidden email]> wrote: >> >>> Yikes, I thought it was clear. I mean that people should not be worried >>> about this swallowed exception because it's intended since no >>> PatternSyntaxException should not occur there >>> >> I can confirm that the comment you have added is not clear, and even the >> explanatory sentence you wrote above is confusing. >> >> >>> I think it's better to say something than letting the catch empty. >> >> It is better to add *useful* information but if you are adding a comment >> that is stating the obvious then it is better to leave it empty. >> >> >>> Maybe what I said is not what I wanted to say and can be understood in >>> another way? What did you understand? >>> >> When I read your comment: >> "obviously a PatternSyntaxException should not occur here" >> >> I think: "hmmmm... it doesn't seem obvious to me that such an exception >> should not occur... but if the committer stated it is *obvious* then it >> means I am missing something" >> >> Instead, at least based on your reply to Taher, it seems that you ignore >> why the catch block was added. >> >> >>> What would you say? >> >> I would either explain why the code is swallowing the runtime exception or >> remove your comment. >> >> Jacopo >> > |
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
|
|
On Wed, Mar 29, 2017 at 12:25 PM, Jacques Le Roux <
[hidden email]> wrote: > Mmm, maybe I was not totally clear if you did not read my answer to Taher. > > I meant to remove the useless try and catch. We have 70+ other cases like > that w/o try and catch I would recommend to revert your commit and start a new thread to discuss this new task. Jacopo |
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
|
Administrator
|
Le 29/03/2017 à 13:53, Jacopo Cappellato a écrit :
> On Wed, Mar 29, 2017 at 12:25 PM, Jacques Le Roux < > [hidden email]> wrote: > >> Mmm, maybe I was not totally clear if you did not read my answer to Taher. >> >> I meant to remove the useless try and catch. We have 70+ other cases like >> that w/o try and catch > > I would recommend to revert your commit and start a new thread to discuss > this new task. > > Jacopo > Jacques |
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
|
|
On Wed, Mar 29, 2017 at 2:17 PM, Jacques Le Roux <
[hidden email]> wrote: > Le 29/03/2017 à 13:53, Jacopo Cappellato a écrit : > >> On Wed, Mar 29, 2017 at 12:25 PM, Jacques Le Roux < >> [hidden email]> wrote: >> >> Mmm, maybe I was not totally clear if you did not read my answer to Taher. >>> >>> I meant to remove the useless try and catch. We have 70+ other cases like >>> that w/o try and catch >>> >> >> I would recommend to revert your commit and start a new thread to discuss >> this new task. >> >> Jacopo >> >> But seriously, why? It is not clear enough already? > I have already explained it to you and your response makes me think you are not willing to collaborate so I am not going to invest further time on this topic. Jacopo |
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/f
|
Administrator
|
In reply to this post by taher
There is some truth in what you say, but I like action.
So I'll remove these last useless PatternSyntaxException with the useless comment I put in. Then "Fix Default or Empty Catch block in Java files" OFBIZ-8341 will be done. I will explain each commit in the Jira to answer Jacopo and everybody interested. Then no comments on swallowed exceptions will be necessary. I hope we will be more cautious on this subject in our reviews from now. I put try-with-resources everywhere I found, I don't think we need a Jira for that. Maybe some remain to be done. As you suggest, we can do it when stumbling upon it. I like to clean code when my IDE shows me warning and such. I think everybody should do so, then it's just a small action. If we don't do so, later it's overwhelming. Mind you, at r1788869 I did not clean all OFBiz code but only the new Birt classes where it was necessary. I agree that cleaning code should be done with caution and I did not then, I already answered you about that. But it was not really bulk changes, just small changes in 4 Java classes. We can surely minimise risk by avoiding bulk changes, especially when they are complex. The changes I did recently, and you relate with the 4 points below, are not complex nor context dependent. That's why I called them "no functional changes" I'd not like that an error like in r1788869, paralyses committers, or worse that we think refactoring is reserved to an elite. Everyone makes errors, this is how you learn best, by doing! Sometimes stupid error happens. It's generally not too serious when it's not at the design or architecture level. You can always change code, hardly the structure you are in. Fortunately, 15 years ago, the OFBiz founders made what, I think, was then among the best choices. Since then a lot of code was added, and we need to improve it. My 2cts Jacques Le 29/03/2017 à 08:08, Taher Alkhateeb a écrit : > Okay so your reply is making a point. If you were not entirely sure about > the purpose of the exception handling block, and whether there is a need > for the replaceAll call, then why did you commit this comment? Why > introduce this confusion on what is essentially already confusing and old > code that requires some pruning. > > It would have been more beneficial and less time consuming for all of us if > you did a thorough investigation of the code. And it seems you _did_ that > after comments from Jacopo and myself which is why you reached the > conclusions in your latest post in here right? > > My recommendation is to minimize these _bulk_ commits where you identify > some problem and try to solve it _everywhere_. So example for such bulk > commits: > - Let's put try-with-resources everywhere > - Let's put this certain comment everywhere > - Let's put an annotation to suppress warnings everywhere > - Let's Convert all objects of a certain type to another type everywhere > > Doing bulk commits introduces the risk of making errors because code does > not work exactly the same way everywhere. Everything has context and should > be studied carefully. > > So, my 2 cents. > > On Tue, Mar 28, 2017 at 11:17 PM, Jacques Le Roux < > [hidden email]> wrote: > >> Le 28/03/2017 à 20:04, Taher Alkhateeb a écrit : >> >>> You can actually remove the entire try/catch blocks because >>> PatternSyntaxException is a RuntimeException. So the whole thing can be >>> removed because it's not checked. However, if we intentionally added a >>> try/catch on a runtime exception then this means we expect it to happen >>> and >>> therefore we should do something if that happens (when calling >>> replaceAll() >>> I guess). So I think in either case the comment provided is not a solution >>> because it does not add clarity. We must investigate _why_ the try/catch >>> was introduced in the first place and whether it should be removed >>> altogether. >>> >> Thanks for your analysis. >> >> Sincerely I have no real ideas of why it's there. It's pre Apache era so >> like more than 10 years ago. >> Maybe while creating this code the exception was thrown and the committer >> forgot to remove the catch after fixing the regexp? Or it was added >> automatically by an IDE then? >> Anyway, I see no needs for this catch: "&" can't be wrong as a Java >> regexp. It's not a reserved chars https://docs.oracle.com/javase >> /8/docs/api/java/util/regex/Pattern.html >> BTW we have 72 other ".replaceAll(", and none catch >> PatternSyntaxException. So I think it's OK to remove it... >> >>> Also this code smells of copy-paste pattern. It could be improved and to >>> remove this duplication which violates DRY IMO. >>> >> While reviewing I created https://issues.apache.org/jira/browse/OFBIZ-9287 >> for that >> >> Jacques >> >> >>> On Tue, Mar 28, 2017 at 8:38 PM, Jacques Le Roux < >>> [hidden email]> wrote: >>> >>> Yikes, I thought it was clear. I mean that people should not be worried >>>> about this swallowed exception because it's intended since no >>>> PatternSyntaxException should not occur there >>>> >>>> I think it's better to say something than letting the catch empty. Maybe >>>> what I said is not what I wanted to say and can be understood in another >>>> way? What did you understand? >>>> >>>> What would you say? >>>> >>>> Jacques >>>> >>>> >>>> >>>> Le 28/03/2017 à 17:41, Jacopo Cappellato a écrit : >>>> >>>> The sentence: >>>>> "obviously a PatternSyntaxException should not occur here" >>>>> >>>>> doesn't add any useful information and instead it seems confusing to me. >>>>> What are you trying to convey? >>>>> >>>>> Jacopo >>>>> >>>>> >>>>> >>>>> On Tue, Mar 28, 2017 at 5:26 PM, <[hidden email]> wrote: >>>>> >>>>> Author: jleroux >>>>> >>>>>> Date: Tue Mar 28 15:26:02 2017 >>>>>> New Revision: 1789163 >>>>>> >>>>>> URL: http://svn.apache.org/viewvc?rev=1789163&view=rev >>>>>> Log: >>>>>> Fixed: Fix Default or Empty Catch block in Java files >>>>>> (OFBIZ-8341) >>>>>> >>>>>> Obviously a PatternSyntaxException should not occur there. >>>>>> So I simply put a comment to document the fact even it it seems >>>>>> obvious. >>>>>> >>>>>> I note though that the repeated pattern is a smell for refactoring... >>>>>> >>>>>> Modified: >>>>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>>>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java >>>>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >>>>>> >>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>>>> model/AbstractModelAction.java?rev=1789163&r1=1789162&r2= >>>>>> 1789163&view=diff >>>>>> ============================================================ >>>>>> ================== >>>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java (original) >>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java Tue Mar 28 >>>>>> 15:26:02 2017 >>>>>> @@ -715,7 +715,7 @@ public abstract class AbstractModelActio >>>>>> String queryStringEncoded = >>>>>> queryString.replaceAll("&", "%26"); >>>>>> context.put("queryStringEncoded", >>>>>> queryStringEncoded); >>>>>> } catch (PatternSyntaxException e) { >>>>>> - >>>>>> + // obviously a PatternSyntaxException should >>>>>> not >>>>>> occur here >>>>>> } >>>>>> } >>>>>> } else { >>>>>> >>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java >>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>>>> model/ModelFormAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >>>>>> ============================================================ >>>>>> ================== >>>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java (original) >>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java Tue Mar 28 >>>>>> 15:26:02 2017 >>>>>> @@ -218,7 +218,7 @@ public abstract class ModelFormAction { >>>>>> String queryStringEncoded = >>>>>> queryString.replaceAll("&", "%26"); >>>>>> context.put("queryStringEncoded", >>>>>> queryStringEncoded); >>>>>> } catch (PatternSyntaxException e) { >>>>>> - >>>>>> + // obviously a PatternSyntaxException should >>>>>> not >>>>>> occur here >>>>>> } >>>>>> } >>>>>> } else { >>>>>> >>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>>>> model/ModelTreeAction.java?rev=1789163&r1=1789162&r2=1789163&view=diff >>>>>> ============================================================ >>>>>> ================== >>>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java (original) >>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java Tue Mar 28 >>>>>> 15:26:02 2017 >>>>>> @@ -417,7 +417,7 @@ public abstract class ModelTreeAction ex >>>>>> String queryStringEncoded = >>>>>> queryString.replaceAll("&", "%26"); >>>>>> context.put("queryStringEncoded", >>>>>> queryStringEncoded); >>>>>> } catch (PatternSyntaxException e) { >>>>>> - >>>>>> + // obviously a PatternSyntaxException >>>>>> should >>>>>> not occur here >>>>>> } >>>>>> } >>>>>> } else { >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> |
|
|
inline
On Wed, Mar 29, 2017 at 6:43 PM, Jacques Le Roux < [hidden email]> wrote: > There is some truth in what you say, but I like action. > > So I'll remove these last useless PatternSyntaxException with the useless > comment I put in. > Then "Fix Default or Empty Catch block in Java files" OFBIZ-8341 will be > done. > I will explain each commit in the Jira to answer Jacopo and everybody > interested. > Then no comments on swallowed exceptions will be necessary. I hope we > will be more cautious on this subject in our reviews from now. > > I put try-with-resources everywhere I found, I don't think we need a Jira > for that. Maybe some remain to be done. As you suggest, we can do it when > stumbling upon it. > > I like to clean code when my IDE shows me warning and such. I think > everybody should do so, then it's just a small action. If we don't do so, > later it's overwhelming. Mind you, at r1788869 I did not clean all OFBiz code but only the new Birt > classes where it was necessary. > I agree that cleaning code should be done with caution and I did not then, > I already answered you about that. > > But it was not really bulk changes, just small changes in 4 Java classes. > We can surely minimise risk by avoiding bulk changes, especially when they > are complex. > The changes I did recently, and you relate with the 4 points below, are > not complex nor context dependent. That's why I called them "no functional > changes" > > I'd not like that an error like in r1788869, paralyses committers, or > worse that we think refactoring is reserved to an elite. Everyone makes > errors, this is how you learn best, by doing! > I think we don't _learn_ by committing inappropriate code and wasting other people's time in reviewing and highlighting issues. We learn by discussing, asking questions, working on and studying code, reading books, and many other activities. But committing incorrect code is not _learning_, that's just breaking a system and wasting people's time. Also refactoring and committing is open to any committer. However, It is the committer's responsibility if she/he is not very comfortable in committing something to review it or ask for feedback or help. I don't commit everything I write for example. Many times I open a JIRA, attach a patch, and ask for help or opinion from people. I do that because I'm not 100% comfortable in committing that code. I care deeply about the quality of the code and every time I commit I feel a little nervous because every commit matters! So please, let's keep this discussion objective and neutral. > Sometimes stupid error happens. It's generally not too serious when it's > not at the design or architecture level. You can always change code, hardly > the structure you are in. > Fortunately, 15 years ago, the OFBiz founders made what, I think, was then > among the best choices. Since then a lot of code was added, and we need to > improve it. > > My 2cts > > Jacques > > > > Le 29/03/2017 à 08:08, Taher Alkhateeb a écrit : > >> Okay so your reply is making a point. If you were not entirely sure about >> the purpose of the exception handling block, and whether there is a need >> for the replaceAll call, then why did you commit this comment? Why >> introduce this confusion on what is essentially already confusing and old >> code that requires some pruning. >> >> It would have been more beneficial and less time consuming for all of us >> if >> you did a thorough investigation of the code. And it seems you _did_ that >> after comments from Jacopo and myself which is why you reached the >> conclusions in your latest post in here right? >> >> My recommendation is to minimize these _bulk_ commits where you identify >> some problem and try to solve it _everywhere_. So example for such bulk >> commits: >> - Let's put try-with-resources everywhere >> - Let's put this certain comment everywhere >> - Let's put an annotation to suppress warnings everywhere >> - Let's Convert all objects of a certain type to another type everywhere >> >> Doing bulk commits introduces the risk of making errors because code does >> not work exactly the same way everywhere. Everything has context and >> should >> be studied carefully. >> >> So, my 2 cents. >> >> On Tue, Mar 28, 2017 at 11:17 PM, Jacques Le Roux < >> [hidden email]> wrote: >> >> Le 28/03/2017 à 20:04, Taher Alkhateeb a écrit : >>> >>> You can actually remove the entire try/catch blocks because >>>> PatternSyntaxException is a RuntimeException. So the whole thing can be >>>> removed because it's not checked. However, if we intentionally added a >>>> try/catch on a runtime exception then this means we expect it to happen >>>> and >>>> therefore we should do something if that happens (when calling >>>> replaceAll() >>>> I guess). So I think in either case the comment provided is not a >>>> solution >>>> because it does not add clarity. We must investigate _why_ the try/catch >>>> was introduced in the first place and whether it should be removed >>>> altogether. >>>> >>>> Thanks for your analysis. >>> >>> Sincerely I have no real ideas of why it's there. It's pre Apache era so >>> like more than 10 years ago. >>> Maybe while creating this code the exception was thrown and the committer >>> forgot to remove the catch after fixing the regexp? Or it was added >>> automatically by an IDE then? >>> Anyway, I see no needs for this catch: "&" can't be wrong as a Java >>> regexp. It's not a reserved chars https://docs.oracle.com/javase >>> /8/docs/api/java/util/regex/Pattern.html >>> BTW we have 72 other ".replaceAll(", and none catch >>> PatternSyntaxException. So I think it's OK to remove it... >>> >>> Also this code smells of copy-paste pattern. It could be improved and to >>>> remove this duplication which violates DRY IMO. >>>> >>>> While reviewing I created https://issues.apache.org/jira >>> /browse/OFBIZ-9287 >>> for that >>> >>> Jacques >>> >>> >>> On Tue, Mar 28, 2017 at 8:38 PM, Jacques Le Roux < >>>> [hidden email]> wrote: >>>> >>>> Yikes, I thought it was clear. I mean that people should not be worried >>>> >>>>> about this swallowed exception because it's intended since no >>>>> PatternSyntaxException should not occur there >>>>> >>>>> I think it's better to say something than letting the catch empty. >>>>> Maybe >>>>> what I said is not what I wanted to say and can be understood in >>>>> another >>>>> way? What did you understand? >>>>> >>>>> What would you say? >>>>> >>>>> Jacques >>>>> >>>>> >>>>> >>>>> Le 28/03/2017 à 17:41, Jacopo Cappellato a écrit : >>>>> >>>>> The sentence: >>>>> >>>>>> "obviously a PatternSyntaxException should not occur here" >>>>>> >>>>>> doesn't add any useful information and instead it seems confusing to >>>>>> me. >>>>>> What are you trying to convey? >>>>>> >>>>>> Jacopo >>>>>> >>>>>> >>>>>> >>>>>> On Tue, Mar 28, 2017 at 5:26 PM, <[hidden email]> wrote: >>>>>> >>>>>> Author: jleroux >>>>>> >>>>>> Date: Tue Mar 28 15:26:02 2017 >>>>>>> New Revision: 1789163 >>>>>>> >>>>>>> URL: http://svn.apache.org/viewvc?rev=1789163&view=rev >>>>>>> Log: >>>>>>> Fixed: Fix Default or Empty Catch block in Java files >>>>>>> (OFBIZ-8341) >>>>>>> >>>>>>> Obviously a PatternSyntaxException should not occur there. >>>>>>> So I simply put a comment to document the fact even it it seems >>>>>>> obvious. >>>>>>> >>>>>>> I note though that the repeated pattern is a smell for refactoring... >>>>>>> >>>>>>> Modified: >>>>>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>>>>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java >>>>>>> ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >>>>>>> >>>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>>>>> model/AbstractModelAction.java?rev=1789163&r1=1789162&r2= >>>>>>> 1789163&view=diff >>>>>>> ============================================================ >>>>>>> ================== >>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java >>>>>>> (original) >>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/AbstractModelAction.java Tue Mar >>>>>>> 28 >>>>>>> 15:26:02 2017 >>>>>>> @@ -715,7 +715,7 @@ public abstract class AbstractModelActio >>>>>>> String queryStringEncoded = >>>>>>> queryString.replaceAll("&", "%26"); >>>>>>> context.put("queryStringEncoded", >>>>>>> queryStringEncoded); >>>>>>> } catch (PatternSyntaxException e) { >>>>>>> - >>>>>>> + // obviously a PatternSyntaxException >>>>>>> should >>>>>>> not >>>>>>> occur here >>>>>>> } >>>>>>> } >>>>>>> } else { >>>>>>> >>>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java >>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>>>>> model/ModelFormAction.java?rev=1789163&r1=1789162&r2=1789163 >>>>>>> &view=diff >>>>>>> ============================================================ >>>>>>> ================== >>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java (original) >>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/ModelFormAction.java Tue Mar 28 >>>>>>> 15:26:02 2017 >>>>>>> @@ -218,7 +218,7 @@ public abstract class ModelFormAction { >>>>>>> String queryStringEncoded = >>>>>>> queryString.replaceAll("&", "%26"); >>>>>>> context.put("queryStringEncoded", >>>>>>> queryStringEncoded); >>>>>>> } catch (PatternSyntaxException e) { >>>>>>> - >>>>>>> + // obviously a PatternSyntaxException >>>>>>> should >>>>>>> not >>>>>>> occur here >>>>>>> } >>>>>>> } >>>>>>> } else { >>>>>>> >>>>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java >>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/ >>>>>>> framework/widget/src/main/java/org/apache/ofbiz/widget/ >>>>>>> model/ModelTreeAction.java?rev=1789163&r1=1789162&r2=1789163 >>>>>>> &view=diff >>>>>>> ============================================================ >>>>>>> ================== >>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java (original) >>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/widget/src/main/ >>>>>>> java/org/apache/ofbiz/widget/model/ModelTreeAction.java Tue Mar 28 >>>>>>> 15:26:02 2017 >>>>>>> @@ -417,7 +417,7 @@ public abstract class ModelTreeAction ex >>>>>>> String queryStringEncoded = >>>>>>> queryString.replaceAll("&", "%26"); >>>>>>> context.put("queryStringEncoded", >>>>>>> queryStringEncoded); >>>>>>> } catch (PatternSyntaxException e) { >>>>>>> - >>>>>>> + // obviously a PatternSyntaxException >>>>>>> should >>>>>>> not occur here >>>>>>> } >>>>>>> } >>>>>>> } else { >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> > |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |