Re: svn commit: r820846 - /ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/payment/PaymentWorker.java

[Adam Heath-2]

[hidden email] wrote:

> Author: lektran
> Date: Thu Oct  1 23:38:38 2009
> New Revision: 820846
>
> URL: http://svn.apache.org/viewvc?rev=820846&view=rev
> Log:
> Removed two deprecated (>3 years) methods:
> getPartyPaymentMethodValueMaps(PageContext, String, Boolean, String)
> getPaymentMethodAndRelated(PageContext, String, String, String, String, String, String, String, String)
>
> Replaced a LinkedList with a FastList and a HashMap with a FastMap
> Added some generics markup
> A couple of enhanced for loops in place of iterator + while
> Made use of auto-boxing for a couple of booleans

This is an example of a bad checkin.  It should have been done as 2
checkins.  One, for the generics/java1.5/javolution changes.  The
other checkin then would remove the methods.

The reason for this, is the method removal is an actual code
api/abi/feature change.  Those types of commits should be separate, as
it makes debugging much easier.  Just because those methods have been
removed *now*, doesn't mean external people using those methods will
notice.  They will find out about it in 1-3 years, when they deploy a
new version of ofbiz, and suddenly their code stops working.

[Scott Gray-2]

Thanks for your feedback Adam.

Regards
Scott

On 3/10/2009, at 5:49 AM, Adam Heath wrote:

> [hidden email] wrote:
>> Author: lektran
>> Date: Thu Oct  1 23:38:38 2009
>> New Revision: 820846
>>
>> URL: http://svn.apache.org/viewvc?rev=820846&view=rev
>> Log:
>> Removed two deprecated (>3 years) methods:
>> getPartyPaymentMethodValueMaps(PageContext, String, Boolean, String)
>> getPaymentMethodAndRelated(PageContext, String, String, String,  
>> String, String, String, String, String)
>>
>> Replaced a LinkedList with a FastList and a HashMap with a FastMap
>> Added some generics markup
>> A couple of enhanced for loops in place of iterator + while
>> Made use of auto-boxing for a couple of booleans
>
> This is an example of a bad checkin.  It should have been done as 2
> checkins.  One, for the generics/java1.5/javolution changes.  The
> other checkin then would remove the methods.
>
> The reason for this, is the method removal is an actual code
> api/abi/feature change.  Those types of commits should be separate, as
> it makes debugging much easier.  Just because those methods have been
> removed *now*, doesn't mean external people using those methods will
> notice.  They will find out about it in 1-3 years, when they deploy a
> new version of ofbiz, and suddenly their code stops working.