# Algorithm Critique

Discussion in 'Spigot Plugin Development' started by Hunky524, May 11, 2016.

1. ### Hunky524

I don't have a question, but as this section of the forum is dedicated to plugin development, I'd thought I'd just get your ideas from a quick algorithm I made. So the problem I needed to solve was to be able to give a player an item. Sounds simple right? The idea was, when a player is given/bought an item, the method will only give them the item if they have space for it; the method will also try to fill pre-existing slots already containing the same item before filling empty slots if needed. I know Spigot has a method to give players items, however, that method returns a HashMap of any items it was not able to give the play, which is not exactly what I want. Anyway, if you want to take a look and give feedback on areas that I might have redundant code, or if you know a more efficient method to do something, I am open to all ideas

Code (Text):
private boolean givePlayerItem(Player p, ItemStack item) {

//Slot Number --- Item Amount
//Stores Slots and Amounts for Pre-Existing Items
HashMap<Integer, Integer> itemStore = new HashMap<Integer, Integer>();
//Stores Last Known Slot Number of an Empty Slot
int emptyStore = -1;
//Stores ItemStack Amount
int stack = item.getAmount();

//Check for existing ItemStacks and Empty Spaces
for (int i = 0; i < p.getInventory().getSize(); i++) {
if (p.getInventory().getItem(i).isSimilar(item)) {
if (p.getInventory().getItem(i).getAmount() == item.getMaxStackSize()) {

} else if (stack - p.getInventory().getItem(i).getAmount() < 1) {
itemStore.put(i, stack);
break;
} else {
itemStore.put(i, stack - p.getInventory().getItem(i).getAmount());
stack = stack - (item.getMaxStackSize() - p.getInventory().getItem(i).getAmount());
if (stack < 1)
break;
}
} else if (p.getInventory().getItem(i) == null || p.getInventory().getItem(i).getType() == Material.AIR) {
emptyStore = i;
} else {

}
}//Initial for Loop

//No Slots Available
if (itemStore.isEmpty() && emptyStore == -1) {
return false;
}

//Not Enough Space
else if (!itemStore.isEmpty() && emptyStore == -1 && stack > 0) {
return false;
}

//Filled Empty Slot
else if (itemStore.isEmpty() && !(emptyStore == -1)) {
p.getInventory().setItem(emptyStore, item);
return true;
}

//Has Enough Slots of the Same Item
else if (!itemStore.isEmpty() && stack < 1) {
for (int i : itemStore.keySet()) {
ItemStack givenItem = p.getInventory().getItem(i);
givenItem.setAmount(p.getInventory().getItem(i).getAmount() + itemStore.get(i));
p.getInventory().setItem(i, givenItem);
}

return true;
}

//Has Some Slots, Fill Rest in Empty Slots
else {
int leftOver = item.getAmount();

for (int i : itemStore.keySet()) {
ItemStack givenItemLoop = p.getInventory().getItem(i);
givenItemLoop.setAmount(p.getInventory().getItem(i).getAmount() + itemStore.get(i));
p.getInventory().setItem(i, givenItemLoop);
leftOver = leftOver - itemStore.get(i);
}

ItemStack emptyItemGive = item;
emptyItemGive.setAmount(leftOver);
p.getInventory().setItem(emptyStore, emptyItemGive);

return true;
}
}

2. ### KazamiThe9029

You can check if addItem HashMap is empty then the inventory has enough space for the ItemStack, and if it's not empty then it doesn't, however, this would add the item with the addItem method, you can remove the item later, but this is not really an accurate solution.
What I would do is make an integer object and then a for loop to the player.getInventory().getStorageContents() and check if the item is null, if it is, then add the maximum stack size to the integer object, if the item isn't null, check if the item is similar to the bought item, if so, you'll add the maximum item stack size minus the item amount to the integer object, then end the loop, and check if the bought item amount is smaller than or equals to the integer object, if so, add the item. I didn't test your method but it looks great, but you know, you could've done it much simpler.
Here's a code I just wrote
Code (Text):
ItemStack BOUGHT_ITEM = new ItemStack(Material.DIAMOND, 10);
int EMPTY = 0;

for (ItemStack ITEMSTACK : PLAYER.getInventory().getStorageContents()) {
if (ITEMSTACK == null) {
EMPTY += BOUGHT_ITEM.getType().getMaxStackSize();
} else if (ITEMSTACK.isSimilar(BOUGHT_ITEM)) {
EMPTY += ITEMSTACK.getType().getMaxStackSize() - ITEMSTACK.getAmount();
}
}
if (BOUGHT_ITEM.getAmount() <= EMPTY) {
PLAYER.sendMessage(ChatColor.GOLD
+ "You have enough space to get " + BOUGHT_ITEM.getAmount() + " " + BOUGHT_ITEM.getType().toString() + ", EMPTY is "
+ EMPTY + ".");
} else {
PLAYER.sendMessage(ChatColor.RED
+ "You do not have enough space to get " + BOUGHT_ITEM.getAmount() + " " + BOUGHT_ITEM.getType().toString() + ", EMPTY is "
+ EMPTY + ".");
}

#2
Last edited: May 11, 2016
• Like x 1
3. ### Hunky524

Oh damn, yea. I see your method is definitely much shorter and cleaner than mine. Your method utilizes the pre-built method in spigot to add items with a simple check before hand. In my method I am keeping track of all the potential spaces that I can store the item and then run a bunch of if-checks to see if I actually can store the item. The only real thing I would (maybe) add to your method if I were to "borrow" it, would be to modify it a little bit so that when/if it found enough spaces of the same item, it broke out of the for loop even though it probably makes an extremely insignificant difference.

• Creative x 1
4. ### KazamiThe9029

Sounds like a good idea, I'll make sure to do it if I needed it in the future