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 {
