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