OFBiz › OFBiz - Dev

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

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

25 messages Options

David E Jones-2

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

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() {
>
>

Adrian Crum

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

Plus, you can eliminate one if() construct:

if (testReq != null) {
return "TRUE".equals(testReq.toUpperCase());
}

-Adrian

David E Jones wrote:

> 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() {
>>
>>
>
>

BJ Freeman

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

In reply to this post by David E Jones-2

that does not look like my patch.
mine has an uppcase in it.

David E Jones sent the following on 12/18/2007 10:19 AM:

> 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

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

Administrator

Not both, just one had, I have already dealed with that in trunk rev. 605190 and release4.0 rev. 605189

Jacques

De : "BJ Freeman" <[hidden email]>

> that does not look like my patch.
> mine has an uppcase in it.
>
>
> David E Jones sent the following on 12/18/2007 10:19 AM:
> > 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

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

Administrator

In reply to this post by David E Jones-2

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...
2. I let BJ answer, personally I would put false but I did not know why BJ put this so I let it.
3. I even could have rewritten it
"TRUE".equals(testReq.toUpperCase()) ? true : false;
but I did not thought it was such important

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

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

Administrator

In reply to this post by Adrian Crum

Yes clearer than my proposition using ternary operator and no worry about default value : + 1
We can play a lot with thing like this, I wrote thousands of APL lines code, I'm used to play with these kind of boolean stuffes, I
must say that from this POV, APL is more fun than any other languages ;o)

Jacques

----- Message d'origine -----
De : "Adrian Crum" <[hidden email]>
À : <[hidden email]>
Envoyé : mardi 18 décembre 2007 19:26
Objet : Re: svn commit: r605186 -
/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java

> Plus, you can eliminate one if() construct:
>
> if (testReq != null) {
> return "TRUE".equals(testReq.toUpperCase());
> }
>
> -Adrian
>
> David E Jones wrote:
>
> > 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/AIMPaymentSer

vices.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

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

Administrator

I thought a bit about that, and since I spoke about APL, that's how it would be written in an APL style in Java :

return testReq == null ? true : ("TRUE".equals(testReq.toUpperCase()) ? true : else;

(Note I choose to put true if null but maybe it should be false as BJ did...)

We used to call this type of style "one liner syndrom". The goal (play?) is to put the as less as characters as possible, it's a bit
better read when written

return (testReq == null) ? true : ("TRUE".equals(testReq.toUpperCase()) ? true : else;

But with Java you can't really have fun with this, it's not APL...

Maybe some may argue that to use blocks and brackets still makes sense since it's far more readable (at a glance)...
It was always a dilemma for me when I was writting APL and C or Pascal in the same day... I did not take the straight road, it's
not the one I prefer...

Jacques

PS : sorry for bothering :p

> Yes clearer than my proposition using ternary operator and no worry about default value : + 1
> We can play a lot with thing like this, I wrote thousands of APL lines code, I'm used to play with these kind of boolean stuffes,
I

> must say that from this POV, APL is more fun than any other languages ;o)
>
> Jacques
>
> ----- Message d'origine -----
> De : "Adrian Crum" <[hidden email]>
> À : <[hidden email]>
> Envoyé : mardi 18 décembre 2007 19:26
> Objet : Re: svn commit: r605186 -
> /ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
>
>
> > Plus, you can eliminate one if() construct:
> >
> > if (testReq != null) {
> > return "TRUE".equals(testReq.toUpperCase());
> > }
> >
> > -Adrian
> >
> > David E Jones wrote:
> >
> > > 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/AIMPaymentSer
> vices.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() {
> > >>
> > >>
> > >
> > >
> >
>

Adrian Crum

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

It seems to me it should be:

return testReq == null ? false : "TRUE".equals(testReq.toUpperCase());

Anything that is not "TRUE" is false.

-Adrian

Jacques Le Roux wrote:

> I thought a bit about that, and since I spoke about APL, that's how it would be written in an APL style in Java :
>
> return testReq == null ? true : ("TRUE".equals(testReq.toUpperCase()) ? true : else;
>
> (Note I choose to put true if null but maybe it should be false as BJ did...)
>
> We used to call this type of style "one liner syndrom". The goal (play?) is to put the as less as characters as possible, it's a bit
> better read when written
>
> return (testReq == null) ? true : ("TRUE".equals(testReq.toUpperCase()) ? true : else;
>
> But with Java you can't really have fun with this, it's not APL...
>
> Maybe some may argue that to use blocks and brackets still makes sense since it's far more readable (at a glance)...
> It was always a dilemma for me when I was writting APL and C or Pascal in the same day... I did not take the straight road, it's
> not the one I prefer...
>
> Jacques
>
> PS : sorry for bothering :p
>
>
>>Yes clearer than my proposition using ternary operator and no worry about default value : + 1
>>We can play a lot with thing like this, I wrote thousands of APL lines code, I'm used to play with these kind of boolean stuffes,
>
> I
>
>>must say that from this POV, APL is more fun than any other languages ;o)
>>
>>Jacques
>>
>>----- Message d'origine -----
>>De : "Adrian Crum" <[hidden email]>
>>À : <[hidden email]>
>>Envoyé : mardi 18 décembre 2007 19:26
>>Objet : Re: svn commit: r605186 -
>>/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
>>
>>
>>
>>>Plus, you can eliminate one if() construct:
>>>
>>>if (testReq != null) {
>>> return "TRUE".equals(testReq.toUpperCase());
>>>}
>>>
>>>-Adrian
>>>
>>>David E Jones wrote:
>>>
>>>
>>>>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/AIMPaymentSer
>>
>>vices.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

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

Administrator

Yes good, no worry with null in last expression evaluation ? I'm still far from a Java expert :(

Jacques

Ooops I re-read it should have been
return testReq == null ? true : ("TRUE".equals(testReq.toUpperCase()) ? true : false; // (not else at end of course, but I guess you
got it, was obvious)
But I prefer yours anyway !

De : "Adrian Crum" <[hidden email]>=

> It seems to me it should be:
>
> return testReq == null ? false : "TRUE".equals(testReq.toUpperCase());
>
> Anything that is not "TRUE" is false.
>
> -Adrian
>
> Jacques Le Roux wrote:
>
> > I thought a bit about that, and since I spoke about APL, that's how it would be written in an APL style in Java :
> >
> > return testReq == null ? true : ("TRUE".equals(testReq.toUpperCase()) ? true : else;
> >
> > (Note I choose to put true if null but maybe it should be false as BJ did...)
> >
> > We used to call this type of style "one liner syndrom". The goal (play?) is to put the as less as characters as possible, it's a

bit
> > better read when written
> >
> > return (testReq == null) ? true : ("TRUE".equals(testReq.toUpperCase()) ? true : else;
> >
> > But with Java you can't really have fun with this, it's not APL...
> >
> > Maybe some may argue that to use blocks and brackets still makes sense since it's far more readable (at a glance)...
> > It was always a dilemma for me when I was writting APL and C or Pascal in the same day... I did not take the straight road,
it's
> > not the one I prefer...
> >
> > Jacques
> >
> > PS : sorry for bothering :p
> >
> >
> >>Yes clearer than my proposition using ternary operator and no worry about default value : + 1
> >>We can play a lot with thing like this, I wrote thousands of APL lines code, I'm used to play with these kind of boolean
stuffes,

> >
> > I
> >
> >>must say that from this POV, APL is more fun than any other languages ;o)
> >>
> >>Jacques
> >>
> >>----- Message d'origine -----
> >>De : "Adrian Crum" <[hidden email]>
> >>À : <[hidden email]>
> >>Envoyé : mardi 18 décembre 2007 19:26
> >>Objet : Re: svn commit: r605186 -
> >>/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
> >>
> >>
> >>
> >>>Plus, you can eliminate one if() construct:
> >>>
> >>>if (testReq != null) {
> >>> return "TRUE".equals(testReq.toUpperCase());
> >>>}
> >>>
> >>>-Adrian
> >>>
> >>>David E Jones wrote:
> >>>
> >>>
> >>>>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/AIMPaymentSe

> >>
> >>vices.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() {
> >>>>>
> >>>>>
> >>>>
> >>>>
> >
> >
>

jonwimp

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

In reply to this post by David E Jones-2

Why was the 1-liner code removed? The original was concise, and was able to take null values
without blowing up.

Or if we're worried about class cast exceptions:

return "TRUE".equals(AIMProperties.get("testReq"));

Jonathon

David E Jones wrote:

> 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() {
>>
>>
>
>

Jacopo Cappellato

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

In reply to this post by Jacques Le Roux

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() {
>>>
>>>

Scott Gray

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

Big +1 here, we should never commit anything that we don't completely
understand, it should almost be as if we had written the code ourselves. A
committer is answerable for his commits not the contributor.

Regards
Scott

On 19/12/2007, Jacopo Cappellato <[hidden email]> wrote:
>
> Jacques Le Roux wrote:
> > 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.
>
>

BJ Freeman

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

In reply to this post by Jacopo Cappellato

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() {
>>>>
>>>>
>
>
>
>
>

BJ Freeman

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

In reply to this post by Jacopo Cappellato

The main motivation was that this would never evaluate to test mode,
which is import when first bringin this code online.

Jacopo Cappellato sent the following on 12/19/2007 2:09 AM:

Jacopo Cappellato

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

In reply to this post by BJ Freeman

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)

ok

> second if the properties is null it would not evaluate correct, and
> there is not use using more cpu cycles to evaluate.

What do you mean with "would not evaluate correct"?
The original code, if the passed in parameter is null, would have
returned false.

Jacopo

> 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() {
>>>>>
>>>>>
>>
>>
>>
>>

jonwimp

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

In reply to this post by BJ Freeman

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?

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() {
>>>>>
>>>>>
>>
>>
>>
>>
>
>

BJ Freeman

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

In reply to this post by Jacopo Cappellato

Jacopo Cappellato sent the following on 12/19/2007 3:18 AM:

> 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)
>
> ok
>
>> second if the properties is null it would not evaluate correct, and
>> there is not use using more cpu cycles to evaluate.
>
> What do you mean with "would not evaluate correct"?
> The original code, if the passed in parameter is null, would have
> returned false.

that was my ignorance on java ternary handling nulls.. guess need to dig
into code to see how it handles things like that.

>
> Jacopo
>
>> 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() {
>>>>>>
>>>>>>
>>>
>>>
>>>
>>>
>
>
>
>

Scott Gray

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

In reply to this post by jonwimp

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

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

In reply to this post by jonwimp

Jonathon -- Improov wrote:
> Why not this?
>
> "true".equalsIgnoreCase(AIMProperties.get("testReq"));

+1

>
> 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?

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

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

Administrator

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"));

> > 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() {
> >>>>>>
> >>>>>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
>