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 < [email protected]> 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 < >> [email protected]> 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, <[email protected]> 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 { >>>>> >>>>> >>>>> >>>>> >>>>> >
