ofbiz-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Taher Alkhateeb <slidingfilame...@gmail.com>
Subject 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
Date Wed, 29 Mar 2017 18:22:23 GMT
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, <jleroux@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 {
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message