Re: svn commit: r495945 - in /ofbiz/trunk: applications/ecommerce/webapp/ecommerce/cart/ applications/order/src/org/ofbiz/order/order/ applications/order/src/org/ofbiz/order/shoppingcart/ applications/order/src/org/ofbiz/order/shoppingcart/product/ applica

[Jacques Le Roux <ja...@les7arts.com>]

Si,

Thanks for your review.

> This is a pretty core change so I thought we should pay a little more
> attention.  I don't find anything wrong with it but there are a few
> coding consistency issues:
>
> 1.  Is it better to keep a version of the old addOrIncreaseItem method
>  around in ShoppingCart so we don't have to make so many changes all
> over the place?

This might have been an easier way of doing. I think that now that it's done it's not necessary to revert all back (there are no
compilation problems, hence no signature problems)

> 2.  Should the parentProductId be before dispatcher, for consistency's sake?

Yes, that's a point I did not consider. Not so important but I will change it, easy to do and better for eventual future changes.

> 3.  I think code like this is usually written as:
>
> +                        if (parentProductId != null)
> +                            virtualProductId = parentProductId;
> +                        else
> +                            virtualProductId = ProductWorker.getVariantVirtualId(product);
>
> +                        if (parentProductId != null) {
> +                            virtualProductId = parentProductId;
> }
> +                        else {
> +                            virtualProductId = ProductWorker.getVariantVirtualId(product);
> }

Defintively I should have change that ! I will soon.

Jacques