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 < [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 { >>> >>> >>> >>> >
