Re: svn commit: r921274 - /ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java

[Scott Gray-2]

On 10/03/2010, at 3:05 AM, [hidden email] wrote:

> Author: ashish
> Date: Wed Mar 10 10:05:01 2010
> New Revision: 921274
>
> URL: http://svn.apache.org/viewvc?rev=921274&view=rev
> Log:
> Applied patch from jira issue OFBIZ-3547 - Improvement in "equals" method of "ShoppingCartItem" class.
> Thanks Awdesh for the contribution.
>
> Modified:
>    ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java
>
> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java?rev=921274&r1=921273&r2=921274&view=diff
> ==============================================================================
> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java (original)
> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java Wed Mar 10 10:05:01 2010
> @@ -2332,7 +2332,7 @@ public class ShoppingCartItem implements
>             return false;
>         }
>
> -        if ((this.attributes != null && attributes != null) &&
> +        if ((UtilValidate.isNotEmpty(this.attributes) && UtilValidate.isNotEmpty(attributes)) &&
>                 ((this.attributes.size() != attributes.size()) ||
>                 !(this.attributes.equals(attributes)))) {
>             return false;
>

Hi Ashish,

I'm not entirely clear on what improvement this resulted in, could you or Awdesh please clarify?

Thanks
Scott

[Adam Heath-2]

Scott Gray wrote:

> On 10/03/2010, at 3:05 AM, [hidden email] wrote:
>
>> Author: ashish
>> Date: Wed Mar 10 10:05:01 2010
>> New Revision: 921274
>>
>> URL: http://svn.apache.org/viewvc?rev=921274&view=rev
>> Log:
>> Applied patch from jira issue OFBIZ-3547 - Improvement in "equals" method of "ShoppingCartItem" class.
>> Thanks Awdesh for the contribution.
>>
>> Modified:
>>    ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java
>>
>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java?rev=921274&r1=921273&r2=921274&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java (original)
>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java Wed Mar 10 10:05:01 2010
>> @@ -2332,7 +2332,7 @@ public class ShoppingCartItem implements
>>             return false;
>>         }
>>
>> -        if ((this.attributes != null && attributes != null) &&
>> +        if ((UtilValidate.isNotEmpty(this.attributes) && UtilValidate.isNotEmpty(attributes)) &&
>>                 ((this.attributes.size() != attributes.size()) ||
>>                 !(this.attributes.equals(attributes)))) {
>>             return false;
>>
>
> Hi Ashish,
>
> I'm not entirely clear on what improvement this resulted in, could you or Awdesh please clarify?

Additionally, ShoppingCartItem.attributes can never be null.

this.attributes = item.getAttributes() == null ? new HashMap() : new
HashMap(item.getAttributes());

or

this.attributes = (attributes == null? FastMap.newInstance(): attributes);

This is from the 2 main constructors.

[awdesh parihar]

Hello Scott,
Following the previous condition;
((this.attributes != null && attributes != null) &&
                ((this.attributes.size() != attributes.size()) ||
                !(this.attributes.equals(attributes))))
and the information added by Adam, we can see the why these changes needed.
The equals method returns false if we have both not equals to null, and in
constructor this.attributes never be null and will be an empty map. In that
case the condition will satisfied and two same items treated as different
and will be then added as one more item in cart.
I saw this in my implementation when I was using the event as ;
ShoppingCartEvents.addListToCart()
Here when I pass a list (in my case wishlist) to add then it will add
successfully by sending the attributes as {shoppingListItemSeqId=00001,
shoppingListId=10010, surveyResponses=[]}.
After that when I again add the same list then it will again increase the
quantity in the shopping cart item. But If I try to add individual item of
wish list then addToCart will invoke and this time no attributes passed for
adding item. Now the problem comes, It will add the item as new shopping
cart item instead of increasing quantity.
One more thing need to do is that checking for null is not in our best
practice to check empty for Maps :-).

--
Awdesh Singh Parihar

On Fri, Mar 12, 2010 at 1:01 AM, Scott Gray <[hidden email]>wrote:

> On 10/03/2010, at 3:05 AM, [hidden email] wrote:
>
> > Author: ashish
> > Date: Wed Mar 10 10:05:01 2010
> > New Revision: 921274
> >
> > URL: http://svn.apache.org/viewvc?rev=921274&view=rev
> > Log:
> > Applied patch from jira issue OFBIZ-3547 - Improvement in "equals" method
> of "ShoppingCartItem" class.
> > Thanks Awdesh for the contribution.
> >
> > Modified:
> >
>  ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java
> >
> > Modified:
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java
> > URL:
> http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java?rev=921274&r1=921273&r2=921274&view=diff
> >
> ==============================================================================
> > ---
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java
> (original)
> > +++
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java
> Wed Mar 10 10:05:01 2010
> > @@ -2332,7 +2332,7 @@ public class ShoppingCartItem implements
> >             return false;
> >         }
> >
> > -        if ((this.attributes != null && attributes != null) &&
> > +        if ((UtilValidate.isNotEmpty(this.attributes) &&
> UtilValidate.isNotEmpty(attributes)) &&
> >                 ((this.attributes.size() != attributes.size()) ||
> >                 !(this.attributes.equals(attributes)))) {
> >             return false;
> >
>
> Hi Ashish,
>
> I'm not entirely clear on what improvement this resulted in, could you or
> Awdesh please clarify?
>
> Thanks
> Scott