[Fwd: 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...]

[Si Chen <si...@opensourcestrategies.com>]

Hi.

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?

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

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

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