You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by jl...@apache.org on 2017/03/28 15:26:02 UTC
svn commit: r1789163 - in
/ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbiz/widget/model:
AbstractModelAction.java ModelFormAction.java ModelTreeAction.java
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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java ModelTreeAction.java
Posted by Taher Alkhateeb <sl...@gmail.com>.
inline
On Wed, Mar 29, 2017 at 6:43 PM, Jacques Le Roux <
jacques.le.roux@les7arts.com> 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 <
>> jacques.le.roux@les7arts.com> 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 <
>>>> jacques.le.roux@les7arts.com> 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, <jl...@apache.org> 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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java
ModelTreeAction.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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 <
> jacques.le.roux@les7arts.com> 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 <
>>> jacques.le.roux@les7arts.com> 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, <jl...@apache.org> 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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java ModelTreeAction.java
Posted by Taher Alkhateeb <sl...@gmail.com>.
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 <
jacques.le.roux@les7arts.com> 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 <
>> jacques.le.roux@les7arts.com> 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, <jl...@apache.org> 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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java
ModelTreeAction.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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 <
> jacques.le.roux@les7arts.com> 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, <jl...@apache.org> 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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java ModelTreeAction.java
Posted by Taher Alkhateeb <sl...@gmail.com>.
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 <
jacques.le.roux@les7arts.com> 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, <jl...@apache.org> 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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java ModelTreeAction.java
Posted by Jacopo Cappellato <ja...@hotwaxsystems.com>.
On Wed, Mar 29, 2017 at 2:17 PM, Jacques Le Roux <
jacques.le.roux@les7arts.com> wrote:
> Le 29/03/2017 à 13:53, Jacopo Cappellato a écrit :
>
>> On Wed, Mar 29, 2017 at 12:25 PM, Jacques Le Roux <
>> jacques.le.roux@les7arts.com> 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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java
ModelTreeAction.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
Le 29/03/2017 � 13:53, Jacopo Cappellato a �crit :
> On Wed, Mar 29, 2017 at 12:25 PM, Jacques Le Roux <
> jacques.le.roux@les7arts.com> 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? How this case is different from the other 70+?
Jacques
Re: svn commit: r1789163 - in /ofbiz/ofbiz-framework/trunk/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java ModelTreeAction.java
Posted by Jacopo Cappellato <ja...@hotwaxsystems.com>.
On Wed, Mar 29, 2017 at 12:25 PM, Jacques Le Roux <
jacques.le.roux@les7arts.com> 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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java
ModelTreeAction.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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 <
>> jacques.le.roux@les7arts.com> 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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java
ModelTreeAction.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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 <
> jacques.le.roux@les7arts.com> 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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java ModelTreeAction.java
Posted by Jacopo Cappellato <ja...@hotwaxsystems.com>.
On Tue, Mar 28, 2017 at 7:38 PM, Jacques Le Roux <
jacques.le.roux@les7arts.com> 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/framework/widget/src/main/java/org/apache/ofbi
z/widget/model: AbstractModelAction.java ModelFormAction.java
ModelTreeAction.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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, <jl...@apache.org> 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/framework/widget/src/main/java/org/apache/ofbiz/widget/model:
AbstractModelAction.java ModelFormAction.java ModelTreeAction.java
Posted by Jacopo Cappellato <ja...@hotwaxsystems.com>.
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, <jl...@apache.org> 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 {
>
>
>