Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REFACTOR: refactored withVariantRequests to address long method code smell. #124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

REFACTOR: refactored withVariantRequests to address long method code smell. #124

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fac25d6
REFACTOR: refactored withVariantRequests to address long method code …
victorcaio06 Nov 13, 2023
5bba3ad
Merge pull request #1 from victorcaio06/refactor-withVariantRequests
victorcaio06 Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 24 additions & 26 deletions src/main/java/com/shopify/model/ShopifyProductUpdateRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

List<Integer> positions = new ArrayList<>(variantRequests.size());

for (int i = 0; i < variantRequests.size(); i++) {
final ShopifyVariantRequest shopifyVariantRequestForPosition = variantRequests.get(i);
positions.add(shopifyVariantRequestForPosition.getRequest().getPosition());

}

int maxPosition = variantRequests.stream().map(ShopifyVariantRequest::getRequest)
.map(ShopifyVariant::getPosition).max(Comparator.naturalOrder()).get();
int maxPosition = positions.stream().max(Comparator.naturalOrder()).get();

Collections.sort(variantRequests, new ShopifyVariantRequestOption1Comparator());

updateVariants(variantRequests, shopifyVariants, positions, maxPosition);

shopifyProduct.setVariants(shopifyVariants);

return this;
}

private void updateVariants(final List<ShopifyVariantRequest> variantRequests,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

final List<ShopifyVariant> shopifyVariants,
final List<Integer> positions,
int maxPosition) {
for (int i = 0; i < variantRequests.size(); i++) {
final ShopifyVariantRequest shopifyVariantRequest = variantRequests.get(i);

if (shopifyVariantRequest.hasChanged()) {
changed = true;
}

final ShopifyVariant shopifyVariant = shopifyVariantRequest.getRequest();

if (shopifyVariant.getPosition() == 0) {

maxPosition = maxPosition + 1;
maxPosition += 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

shopifyVariant.setPosition(maxPosition);
}

if (shopifyVariantRequest.hasImageSource()) {
final String imageSource = shopifyVariantRequest.getImageSource();
shopifyProduct.getImages().stream().filter(image -> image.getSource().equals(imageSource))
.findFirst().ifPresent(image -> {
variantPositionToImagePosition.put(shopifyVariant.getPosition(), image.getPosition());
});
shopifyProduct.getImages().stream()
.filter(image -> image.getSource().equals(imageSource))
.findFirst()
.ifPresent(
image -> variantPositionToImagePosition.put(shopifyVariant.getPosition(),
image.getPosition()));
}

}

Collections.sort(variantRequests, new ShopifyVariantRequestPositionComparator());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like some of this code got dropped but not added back in? Can y ou double check this?


for (int i = 0; i < variantRequests.size(); i++) {
final ShopifyVariantRequest shopifyVariantRequest = variantRequests.get(i);
if (shopifyVariantRequest.hasChanged()) {
changed = true;
}

final ShopifyVariant shopifyVariant = shopifyVariantRequest.getRequest();
shopifyVariants.add(shopifyVariant);

}

shopifyProduct.setVariants(shopifyVariants);
return this;
}

@Override
Expand Down
Loading