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.

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

Add remove command #1

Open
wants to merge 4 commits into
base: master
from

Conversation

@darshan3
Copy link
Member

commented Aug 18, 2019

Adding a command to remove items from inventory.

How to test:

  1. Give a few pickaxes using "give pickaxe 'amount' " command
  2. Remove one pickaxe using "remove pickaxe" command
  3. Remove many pickaxes using "remove pickaxe amount" with various values of the amount
  4. Try the remove command when you don't have a pickaxe in the inventory
  5. Try the remove command with amount parameter greater than the amount of pickaxe you have in your inventory.

Test the steps 1 to 4 for blocks and shapes too.

@darshan3

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

@jellysnake
Copy link
Member

left a comment

So you mentioned wanting to reduce redundancies in the code, and I can see why. There are definitely parts of the code that look tantalisingly similar.
Like I outlined in chat, there are really two main ways to do this.

Firstly you can split the similar bits of code up into multiple smaller functions and then reuse those in each of the logic flows. For instance, lines 373-383 and 359-369 and 165-175 all look like they could be extracted as does the return at the end of the private removeBlocks and the remove method. You keep doing this for similar code sections and reduce it like that. The ide can help here.

The second method is to take the whole monolithic section and extract it, and then to pass in the things that make it different. For instance a Comparator between two EntityRef that returns true if they should be removed. You can then use a lambda to specify this when you call the method.
This would turn the calling signature into something like this for the item part:

removeMatching(prefab, itemAmount, (prefab, currentPrefab) -> currentPrefab != null && currentPrefab.matches(prefab))

(Although it would depend on how you structured the extracted function)
With this method, you would also still want to tidy up the extracted function a bit.

The unmentioned third method is an extension of the second one. Go even further with "generifiying" the function and then move it to a removeMatching method in the inventory manager itself. This would allow the extracted method to be used by other modules and thus justify the use of lambda and the overall increased complexity.

Either method, I agree that reducing duplicate code would definitely be a good idea, and you have the capabilities to do so.

On a more general note about the code, as it's essentially a duplicate of the PR to Movingblocks/Terasology the overall structure is alright. It's just small things I would like fixed or clarified.

import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.*;

This comment has been minimized.

Copy link
@jellysnake

jellysnake Aug 18, 2019

Member

Prefer no star imports.
This is probably the IDE automatically doing it. You might need to update your settings to stop it.


@RegisterSystem
@Share(ItemCommands.class)

This comment has been minimized.

Copy link
@jellysnake

jellysnake Aug 18, 2019

Member

Why are you sharing the class? Is this needed in a different module?

}
}
} else if (matches.size() > 1) {
StringBuilder builder = new StringBuilder();

This comment has been minimized.

Copy link
@jellysnake

jellysnake Aug 18, 2019

Member

You mentioned that you wanted to try and reduce duplicate code in this file.
You could pull this section and the same in the give command into a private method like "displayItemDuplicates"

return removeBlock(blockFamily, quantity, sender);

} else if (matchingUris.size() > 1) {
StringBuilder builder = new StringBuilder();

This comment has been minimized.

Copy link
@jellysnake

jellysnake Aug 18, 2019

Member

Same as my comment in the remove items section. You could move this to a method

} else if (resolvedShapeUris.size() > 1) {
StringBuilder builder = new StringBuilder();
builder.append("Found block. Non-unique shape name, possible matches: ");
Iterator<ResourceUrn> shapeUris = sortItems(resolvedShapeUris).iterator();

This comment has been minimized.

Copy link
@jellysnake

jellysnake Aug 18, 2019

Member

You could also restructure this and replace it with the method below, as both are Set<ResourceUrn>

return null;
}

private String removeBlock(BlockFamily blockFamily, int itemAmount, EntityRef client) {

This comment has been minimized.

Copy link
@jellysnake

jellysnake Aug 18, 2019

Member

I would prefer a name change here to distinguish it from the prior method. Perhaps something like doRemoveFromInventory

if (matches.size() == 1) {
Prefab prefab = assetManager.getAsset(matches.iterator().next(), Prefab.class).orElse(null);

if (prefab != null && prefab.getComponent(ItemComponent.class) != null) {

This comment has been minimized.

Copy link
@jellysnake

jellysnake Aug 18, 2019

Member

hasComponent(foo.class) should be used over getComponent(foo.class) != null

}
}

private <T extends Comparable<T>> List<T> sortItems(Iterable<T> items) {

This comment has been minimized.

Copy link
@jellysnake

jellysnake Aug 18, 2019

Member

Why is this a seperate method?

@jdrueckert

This comment has been minimized.

Copy link

commented Aug 21, 2019

Just tested it out in-game.
1-3 work as expected.

4

  • works as expected -> error msg "Could not find an item of Core:pickaxe in your inventory" as no pickaxes in inventory
  • would prefer a msg like "Nothing to remove, you don't have Core:pickaxe in your inventory" though

5

  • doesn't work as expected -> would expect that as many items as I still have are removed
  • e.g. I have 3 pickaxes in my inventory and do remove pickaxe 7
  • -> remove 3 pickaxes
  • -> log "Removed 3 Core:pickaxe from your inventory. No more Core:pickaxe items to remove."
  • give pickaxe <amount> with more than inventory space also gives as many as possible and logs the number it gave. IMO remove should work the same way (reversed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.