Re: svn commit: r605186 - /ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java

[Jacques Le Roux]

I find it quite intersting :p

Jacques

De : "Scott Gray" <[hidden email]>

> Perfect, could someone commit it and put this poor thread out of it's misery
> :-)
>
> Scott
>
> On 20/12/2007, Jonathon -- Improov <[hidden email]> wrote:
> >
> > Why not this?
> >
> > "true".equalsIgnoreCase(AIMProperties.get("testReq"));
> >
> > That would work no matter what case, upper or lower or mixed. Unless we
> > don't want case to be ignored?
> >
> >
>

[Jacopo Cappellato]

Jacques Le Roux wrote:
> De : "Jacopo Cappellato" <[hidden email]>
>> Jonathon -- Improov wrote:
>>> Why not this?
>> +1
>
> I will dot it soon using my preferred one (mix of Jonathon's and Adrian's)
> return testReq == null ? false : "true".equalsIgnoreCase(AIMProperties.get("testReq"));

You can do the same with:

"true".equalsIgnoreCase(AIMProperties.get("testReq"));

in fact, if AIMProperties.get("testReq") is null then the
equalsIgnoreCase method will return false.

Jacopo

>
>>> That would work no matter what case, upper or lower or mixed. Unless we
>>> don't want case to be ignored?
>> Yes, this is one doubt I have: who is setting the "testReq" parameter?
>> Why it was initially compared to TRUE and not true? Is there a reason or
>> just a bug?
>
> I will look at that too
>
> Jacques
>
>> Jacopo
>>
>>
>>> The above is also better than:
>>>
>>> testReq.equalsIgnoreCase("true");
>>>
>>> The 1st statement doesn't require any testing of "testReq" for null
>>> value. The 2nd statement will bomb if "testReq" is null.
>>>
>>> I think I'm getting more and more lost in this thread. Time to bug out. :)
>>>
>>> Jonathon
>>>
>>> BJ Freeman wrote:
>>>> first, the orgninal code would never evaluate since lowercase true is
>>>> correct.
>>>> return ("TRUE".equals((String) should be return ("true".equals((String)
>>>> second if the properties is null it would not evaluate correct, and
>>>> there is not use using more cpu cycles to evaluate.
>>>> if(testReq.equals("TRUE")) this is operator error thought I had changed
>>>> it like I did in versiion 4.0
>>>> if(testReq.toUpperCase().equals("TRUE"))
>>>> proably should have been
>>>> if(testReq.tolowerCase().equals("true"))
>>>> the tolowerCase make sure if a user puts in TRUE is will still get
>>>> evaluated properly.
>>>>
>>>>
>>>> Jacopo Cappellato sent the following on 12/19/2007 2:09 AM:
>>>>> Jacques, BJ,
>>>>>
>>>>> after having read the comments in the issue and the commit logs I really
>>>>> don't understand what was the bug and how this patch is going to fix it.
>>>>> Please, see my comments below:
>>>>>
>>>>> Jacques Le Roux wrote:
>>>>>> David,
>>>>>>
>>>>>> 1. I had already refactored the code, please see trunk rev. 605190 and
>>>>>> release4.0 rev. 605189. BTW there are tons and tons of such
>>>>>> bad code formating eveywhere in the code...
>>>>> This is an exaggeration and by the way this is not a good reason for
>>>>> adding new ones
>>>>>
>>>>>> 2. I let BJ answer, personally I would put false but I did not know
>>>>>> why BJ put this so I let it.
>>>>> This is alone a good reason to not commit in the trunk and release
>>>>> branch.
>>>>>
>>>>>> 3. I even could have rewritten it
>>>>>>         "TRUE".equals(testReq.toUpperCase()) ? true : false;
>>>>>>     but I did not thought it was such important
>>>>>>
>>>>> After a very quick look, in my opinion, the best code snippet was the
>>>>> one modified by the patch.
>>>>>
>>>>> Jacopo
>>>>>
>>>>>> Jacques
>>>>>>
>>>>>>
>>>>>> De : "David E Jones" <[hidden email]>
>>>>>>> 1. Bad code formating
>>>>>>> 2. Makes the default true, is that what we really want?
>>>>>>> 3. If 2 is true then should use more compact and easy to read,
>>>>>>> like if != false instead of if = true
>>>>>>>
>>>>>>> -David
>>>>>>>
>>>>>>>
>>>>>>> On Tue, 18 Dec 2007 11:37:55 -0000
>>>>>>> [hidden email] wrote:
>>>>>>>
>>>>>>>> Author: jleroux
>>>>>>>> Date: Tue Dec 18 03:37:47 2007
>>>>>>>> New Revision: 605186
>>>>>>>>
>>>>>>>> URL: http://svn.apache.org/viewvc?rev=605186&view=rev
>>>>>>>> Log:
>>>>>>>> A patch from BJ Freeman "Allows better testing of testmode from
>>>>>>>> propties file of
>>>>>>>> authorize.net" (https://issues.apache.org/jira/browse/OFBIZ-1450) -
>>>>>>>> OFBIZ-1450
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>>
>>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
>>>>>>>>
>>>>>>>>
>>>>>>>> URL:
>>>>>>>>
> http://svn.apache.org/viewvc/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java?rev=605186&r1=605185&r2=605186&view=diff
>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>>
>>>>>>>>
>>>>>>>> ---
>>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
>>>>>>>>
>>>>>>>>
>>>>>>>> (original) +++
>>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
>>>>>>>>
>>>>>>>>
>>>>>>>> Tue Dec 18 03:37:47 2007 @@ -376,7 +376,15 @@ } private static
>>>>>>>> boolean isTestMode() {
>>>>>>>> -         return ("TRUE".equals((String)
>>>>>>>> AIMProperties.get("testReq")));
>>>>>>>> +       boolean ret = true;
>>>>>>>> +        String testReq = (String)AIMProperties.get("testReq");
>>>>>>>> +        if(testReq != null) {
>>>>>>>> +            if(testReq.equals("TRUE"))
>>>>>>>> +                ret = true;
>>>>>>>> +            else
>>>>>>>> +                ret = false;
>>>>>>>> +        }
>>>>>>>> +        return ret;
>>>>>>>>      }
>>>>>>>>
>>>>>>>>      private static String getVersion() {
>>>>>>>>
>>>>>>>>
>>>>>
>>>>>
>>>>>
>>>>

[Jacques Le Roux]

Yes, true. I saw it when Jonathon send his message and then forgot. So it will simpley be

return "true".equalsIgnoreCase(AIMProperties.get("testReq"));

Finally Scoot was right : a bit miserable :o) Though, I was not aware of equalsIgnoreCase... Lesson learned...

Jacques


De : "Jacopo Cappellato" <[hidden email]>

> Jacques Le Roux wrote:
> > De : "Jacopo Cappellato" <[hidden email]>
> >> Jonathon -- Improov wrote:
> >>> Why not this?
> >> +1
> >
> > I will dot it soon using my preferred one (mix of Jonathon's and Adrian's)
> > return testReq == null ? false : "true".equalsIgnoreCase(AIMProperties.get("testReq"));
>
> You can do the same with:
>
> "true".equalsIgnoreCase(AIMProperties.get("testReq"));
>
> in fact, if AIMProperties.get("testReq") is null then the
> equalsIgnoreCase method will return false.
>
> Jacopo
>
>
> >
> >>> That would work no matter what case, upper or lower or mixed. Unless we
> >>> don't want case to be ignored?
> >> Yes, this is one doubt I have: who is setting the "testReq" parameter?
> >> Why it was initially compared to TRUE and not true? Is there a reason or
> >> just a bug?
> >
> > I will look at that too
> >
> > Jacques
> >
> >> Jacopo
> >>
> >>
> >>> The above is also better than:
> >>>
> >>> testReq.equalsIgnoreCase("true");
> >>>
> >>> The 1st statement doesn't require any testing of "testReq" for null
> >>> value. The 2nd statement will bomb if "testReq" is null.
> >>>
> >>> I think I'm getting more and more lost in this thread. Time to bug out. :)
> >>>
> >>> Jonathon
> >>>
> >>> BJ Freeman wrote:
> >>>> first, the orgninal code would never evaluate since lowercase true is
> >>>> correct.
> >>>> return ("TRUE".equals((String) should be return ("true".equals((String)
> >>>> second if the properties is null it would not evaluate correct, and
> >>>> there is not use using more cpu cycles to evaluate.
> >>>> if(testReq.equals("TRUE")) this is operator error thought I had changed
> >>>> it like I did in versiion 4.0
> >>>> if(testReq.toUpperCase().equals("TRUE"))
> >>>> proably should have been
> >>>> if(testReq.tolowerCase().equals("true"))
> >>>> the tolowerCase make sure if a user puts in TRUE is will still get
> >>>> evaluated properly.
> >>>>
> >>>>
> >>>> Jacopo Cappellato sent the following on 12/19/2007 2:09 AM:
> >>>>> Jacques, BJ,
> >>>>>
> >>>>> after having read the comments in the issue and the commit logs I really
> >>>>> don't understand what was the bug and how this patch is going to fix it.
> >>>>> Please, see my comments below:
> >>>>>
> >>>>> Jacques Le Roux wrote:
> >>>>>> David,
> >>>>>>
> >>>>>> 1. I had already refactored the code, please see trunk rev. 605190 and
> >>>>>> release4.0 rev. 605189. BTW there are tons and tons of such
> >>>>>> bad code formating eveywhere in the code...
> >>>>> This is an exaggeration and by the way this is not a good reason for
> >>>>> adding new ones
> >>>>>
> >>>>>> 2. I let BJ answer, personally I would put false but I did not know
> >>>>>> why BJ put this so I let it.
> >>>>> This is alone a good reason to not commit in the trunk and release
> >>>>> branch.
> >>>>>
> >>>>>> 3. I even could have rewritten it
> >>>>>>         "TRUE".equals(testReq.toUpperCase()) ? true : false;
> >>>>>>     but I did not thought it was such important
> >>>>>>
> >>>>> After a very quick look, in my opinion, the best code snippet was the
> >>>>> one modified by the patch.
> >>>>>
> >>>>> Jacopo
> >>>>>
> >>>>>> Jacques
> >>>>>>
> >>>>>>
> >>>>>> De : "David E Jones" <[hidden email]>
> >>>>>>> 1. Bad code formating
> >>>>>>> 2. Makes the default true, is that what we really want?
> >>>>>>> 3. If 2 is true then should use more compact and easy to read,
> >>>>>>> like if != false instead of if = true
> >>>>>>>
> >>>>>>> -David
> >>>>>>>
> >>>>>>>
> >>>>>>> On Tue, 18 Dec 2007 11:37:55 -0000
> >>>>>>> [hidden email] wrote:
> >>>>>>>
> >>>>>>>> Author: jleroux
> >>>>>>>> Date: Tue Dec 18 03:37:47 2007
> >>>>>>>> New Revision: 605186
> >>>>>>>>
> >>>>>>>> URL: http://svn.apache.org/viewvc?rev=605186&view=rev
> >>>>>>>> Log:
> >>>>>>>> A patch from BJ Freeman "Allows better testing of testmode from
> >>>>>>>> propties file of
> >>>>>>>> authorize.net" (https://issues.apache.org/jira/browse/OFBIZ-1450) -
> >>>>>>>> OFBIZ-1450
> >>>>>>>>
> >>>>>>>> Modified:
> >>>>>>>>
> >>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Modified:
> >>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> URL:
> >>>>>>>>
> >

http://svn.apache.org/viewvc/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java?rev=605186&r1=605185&r2=605186&view=diff

> >>>>>>
> >>>>>>>> ==============================================================================
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> ---
> >>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> (original) +++
> >>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Tue Dec 18 03:37:47 2007 @@ -376,7 +376,15 @@ } private static
> >>>>>>>> boolean isTestMode() {
> >>>>>>>> -         return ("TRUE".equals((String)
> >>>>>>>> AIMProperties.get("testReq")));
> >>>>>>>> +       boolean ret = true;
> >>>>>>>> +        String testReq = (String)AIMProperties.get("testReq");
> >>>>>>>> +        if(testReq != null) {
> >>>>>>>> +            if(testReq.equals("TRUE"))
> >>>>>>>> +                ret = true;
> >>>>>>>> +            else
> >>>>>>>> +                ret = false;
> >>>>>>>> +        }
> >>>>>>>> +        return ret;
> >>>>>>>>      }
> >>>>>>>>
> >>>>>>>>      private static String getVersion() {
> >>>>>>>>
> >>>>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>
>

[Jacques Le Roux]

OK, done finally, needed a cast

return "true".equalsIgnoreCase((String)AIMProperties.get("testReq"));

When there are no conventions, it's always a trade off between readability (using more verbose forms) and conciness. Sometime
conciness is better, it's the case here

BTW, I found this advice http://docs.sun.com/app/docs/doc/819-3681/abebf?a=view#abebm is there any performance advantages (I reckon
it's would anyway be marginal) ?

Thanks for helping this out and sorry to have bothered some

Jacques

De : "Jacques Le Roux" <[hidden email]>

> Yes, true. I saw it when Jonathon send his message and then forgot. So it will simpley be
>
> return "true".equalsIgnoreCase(AIMProperties.get("testReq"));
>
> Finally Scoot was right : a bit miserable :o) Though, I was not aware of equalsIgnoreCase... Lesson learned...
>
> Jacques
>
>
> De : "Jacopo Cappellato" <[hidden email]>
> > Jacques Le Roux wrote:
> > > De : "Jacopo Cappellato" <[hidden email]>
> > >> Jonathon -- Improov wrote:
> > >>> Why not this?
> > >> +1
> > >
> > > I will dot it soon using my preferred one (mix of Jonathon's and Adrian's)
> > > return testReq == null ? false : "true".equalsIgnoreCase(AIMProperties.get("testReq"));
> >
> > You can do the same with:
> >
> > "true".equalsIgnoreCase(AIMProperties.get("testReq"));
> >
> > in fact, if AIMProperties.get("testReq") is null then the
> > equalsIgnoreCase method will return false.
> >
> > Jacopo
> >
> >
> > >
> > >>> That would work no matter what case, upper or lower or mixed. Unless we
> > >>> don't want case to be ignored?
> > >> Yes, this is one doubt I have: who is setting the "testReq" parameter?
> > >> Why it was initially compared to TRUE and not true? Is there a reason or
> > >> just a bug?
> > >
> > > I will look at that too
> > >
> > > Jacques
> > >
> > >> Jacopo
> > >>
> > >>
> > >>> The above is also better than:
> > >>>
> > >>> testReq.equalsIgnoreCase("true");
> > >>>
> > >>> The 1st statement doesn't require any testing of "testReq" for null
> > >>> value. The 2nd statement will bomb if "testReq" is null.
> > >>>
> > >>> I think I'm getting more and more lost in this thread. Time to bug out. :)
> > >>>
> > >>> Jonathon
> > >>>
> > >>> BJ Freeman wrote:
> > >>>> first, the orgninal code would never evaluate since lowercase true is
> > >>>> correct.
> > >>>> return ("TRUE".equals((String) should be return ("true".equals((String)
> > >>>> second if the properties is null it would not evaluate correct, and
> > >>>> there is not use using more cpu cycles to evaluate.
> > >>>> if(testReq.equals("TRUE")) this is operator error thought I had changed
> > >>>> it like I did in versiion 4.0
> > >>>> if(testReq.toUpperCase().equals("TRUE"))
> > >>>> proably should have been
> > >>>> if(testReq.tolowerCase().equals("true"))
> > >>>> the tolowerCase make sure if a user puts in TRUE is will still get
> > >>>> evaluated properly.
> > >>>>
> > >>>>
> > >>>> Jacopo Cappellato sent the following on 12/19/2007 2:09 AM:
> > >>>>> Jacques, BJ,
> > >>>>>
> > >>>>> after having read the comments in the issue and the commit logs I really
> > >>>>> don't understand what was the bug and how this patch is going to fix it.
> > >>>>> Please, see my comments below:
> > >>>>>
> > >>>>> Jacques Le Roux wrote:
> > >>>>>> David,
> > >>>>>>
> > >>>>>> 1. I had already refactored the code, please see trunk rev. 605190 and
> > >>>>>> release4.0 rev. 605189. BTW there are tons and tons of such
> > >>>>>> bad code formating eveywhere in the code...
> > >>>>> This is an exaggeration and by the way this is not a good reason for
> > >>>>> adding new ones
> > >>>>>
> > >>>>>> 2. I let BJ answer, personally I would put false but I did not know
> > >>>>>> why BJ put this so I let it.
> > >>>>> This is alone a good reason to not commit in the trunk and release
> > >>>>> branch.
> > >>>>>
> > >>>>>> 3. I even could have rewritten it
> > >>>>>>         "TRUE".equals(testReq.toUpperCase()) ? true : false;
> > >>>>>>     but I did not thought it was such important
> > >>>>>>
> > >>>>> After a very quick look, in my opinion, the best code snippet was the
> > >>>>> one modified by the patch.
> > >>>>>
> > >>>>> Jacopo
> > >>>>>
> > >>>>>> Jacques
> > >>>>>>
> > >>>>>>
> > >>>>>> De : "David E Jones" <[hidden email]>
> > >>>>>>> 1. Bad code formating
> > >>>>>>> 2. Makes the default true, is that what we really want?
> > >>>>>>> 3. If 2 is true then should use more compact and easy to read,
> > >>>>>>> like if != false instead of if = true
> > >>>>>>>
> > >>>>>>> -David
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> On Tue, 18 Dec 2007 11:37:55 -0000
> > >>>>>>> [hidden email] wrote:
> > >>>>>>>
> > >>>>>>>> Author: jleroux
> > >>>>>>>> Date: Tue Dec 18 03:37:47 2007
> > >>>>>>>> New Revision: 605186
> > >>>>>>>>
> > >>>>>>>> URL: http://svn.apache.org/viewvc?rev=605186&view=rev
> > >>>>>>>> Log:
> > >>>>>>>> A patch from BJ Freeman "Allows better testing of testmode from
> > >>>>>>>> propties file of
> > >>>>>>>> authorize.net" (https://issues.apache.org/jira/browse/OFBIZ-1450) -
> > >>>>>>>> OFBIZ-1450
> > >>>>>>>>
> > >>>>>>>> Modified:
> > >>>>>>>>
> > >>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> Modified:
> > >>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> URL:
> > >>>>>>>>
> > >
>

http://svn.apache.org/viewvc/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java?rev=605186&r1=605185&r2=605186&view=diff

> > >>>>>>
> > >>>>>>>> ==============================================================================
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> ---
> > >>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> (original) +++
> > >>>>>>>> ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> Tue Dec 18 03:37:47 2007 @@ -376,7 +376,15 @@ } private static
> > >>>>>>>> boolean isTestMode() {
> > >>>>>>>> -         return ("TRUE".equals((String)
> > >>>>>>>> AIMProperties.get("testReq")));
> > >>>>>>>> +       boolean ret = true;
> > >>>>>>>> +        String testReq = (String)AIMProperties.get("testReq");
> > >>>>>>>> +        if(testReq != null) {
> > >>>>>>>> +            if(testReq.equals("TRUE"))
> > >>>>>>>> +                ret = true;
> > >>>>>>>> +            else
> > >>>>>>>> +                ret = false;
> > >>>>>>>> +        }
> > >>>>>>>> +        return ret;
> > >>>>>>>>      }
> > >>>>>>>>
> > >>>>>>>>      private static String getVersion() {
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>
> >
>

[Jacques Le Roux]

> BTW, I found this advice http://docs.sun.com/app/docs/doc/819-3681/abebf?a=view#abebm is there any performance advantages (I
reckon
> it's would anyway be marginal) ?

Ok, forget it : http://www-128.ibm.com/developerworks/java/library/j-jtp1029.html

Jacques