REFACTOR: refactored withVariantRequests to address long method code smell. #124
Conversation
…smell. Moved the logic for updating the Shopify product variants to a separate method, updateVariants. This makes the code more readable and maintainable.
REFACTOR: refactored withVariantRequests to address long method code …
| @@ -148,57 +148,55 @@ public PublishedStep withVariantRequests(final List<ShopifyVariantRequest> varia | |||
| changed = true; | |||
| } | |||
|
|
|||
| final List<ShopifyVariant> shopifyVariants = new ArrayList<>(variantRequests.size()); | |||
| final List<Integer> positions = new ArrayList<>(variantRequests.size()); | |||
| List<ShopifyVariant> shopifyVariants = new ArrayList<>(variantRequests.size()); | |||
There was a problem hiding this comment.
It's generally a good idea to mark something as final unless you are reassigning the object. This prevents something down the line from changing what this variable references. Can you add back in since you didn't change how these are used?
| if (shopifyVariantRequest.hasChanged()) { | ||
| changed = true; | ||
| } | ||
|
|
||
| final ShopifyVariant shopifyVariant = shopifyVariantRequest.getRequest(); | ||
|
|
||
| if (shopifyVariant.getPosition() == 0) { | ||
|
|
||
| maxPosition = maxPosition + 1; | ||
| maxPosition += 1; |
There was a problem hiding this comment.
It's not the best idea to change variables passed into the function as this changes the reference. Calling code may not know or expect the value they pass in to be manipulated so it's a good idea to avoid changing a value of a method argument.
In this case, you don't need to pass maxPosition as an object to this function. It can be calculated right in this function directly because it's not used any where else and you don't have to worry about this. It also cuts down an argument needed to call this function.
| return this; | ||
| } | ||
|
|
||
| private void updateVariants(final List<ShopifyVariantRequest> variantRequests, |
There was a problem hiding this comment.
This is a good example of why you shouldn't modify method arguments. You are changing the shopifyVariants in the updateVariants call. This isn't immediately obvious until you inspect that method. It would be better to have this updateVariants be the list that does everything it's already doing but return a list of ShopifyVariants that is then used in the setVariants call. This keeps all values easy to see where they came from and there isn't mystery to whether or not something outside this function changes values of a variable.
Also once you do this, you can remove the shopifyVariants variable from above and just have it assigned right on the updateVariants function. Something like this:
final List<ShopifyVariant> shopifyVariants = updateVariants(variantRequests, positions, maxPosition);
shopifyProduct.setVariants(shopifyVariants);
| return this; | ||
| } | ||
|
|
||
| private void updateVariants(final List<ShopifyVariantRequest> variantRequests, |
There was a problem hiding this comment.
Can you rename this function to be something like mapVariantRequestToShopifyVariant or something like that? For a second here i though we were updating variants right here but it's just mapping them within the context of a product.
| } | ||
|
|
||
| } | ||
|
|
||
| Collections.sort(variantRequests, new ShopifyVariantRequestPositionComparator()); |
There was a problem hiding this comment.
It looks like some of this code got dropped but not added back in? Can y ou double check this?
Moved the logic for updating the Shopify product variants to a separate method, updateVariants.
This makes the code more readable and maintainable.