GH-865 Fix Illegal Stack and no space in /give command.

[vLuckyyy]

I added an option in the config, which determines whether in the case when the inventory is full, the item should be thrown to the floor, or should return information that there is no space. I fixed the problem with IllegalStack being given by a command such as /give DIRT 512, then 99 dirt would appear.

Summary by CodeRabbit

• New Features

  • Introduced a configuration option to control item dropping behavior when inventory is full.
  • Added a service for managing item distribution to players, enhancing modularity and functionality.
  • Implemented new notices to inform players when their inventory is full and items cannot be given.
  • Added utility functions for checking available space in inventory.

• Bug Fixes

  • Improved player feedback for insufficient inventory space during item acquisition.

[coderabbitai]

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (6)

eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (1)

372-374: LGTM! Consider enhancing the description.

The addition of the dropOnFullInventory configuration option aligns well with the PR objectives. It provides users with the ability to control the behavior of the /give command when a player's inventory is full.

Consider enhancing the description to provide more context:

-        @Description({ " ", "# Determines whether items should be dropped on the ground when the player's inventory is full" })
+        @Description({ " ", "# Determines whether items should be dropped on the ground when the player's inventory is full during /give command execution" })
         public boolean dropOnFullInventory = true;

This minor change explicitly ties the configuration to the /give command, making it clearer for users.

eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)

740-740: LGTM! Consider adding a placeholder for consistency.

The new giveNoSpace message is a good addition and aligns well with the PR objectives. It provides clear feedback when a player's inventory is full.

For consistency with other messages in this class, consider adding a placeholder for potential customization. For example:

-        public Notice giveNoSpace = Notice.chat("<red>✘ <dark_red>Not enough space in inventory!");
+        public Notice giveNoSpace = Notice.chat("<red>✘ <dark_red>Not enough space in inventory! {ITEM}");

This would allow for including the item name or any other relevant information in the message if needed in the future.

eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (2)

755-756: LGTM! Consider adding placeholder for item name.

The addition of the giveNoSpace message is appropriate and aligns with the PR objectives. It provides clear feedback when a player's inventory is full.

Consider adding a placeholder for the item name, e.g., {ITEM}. This would make the message more informative:

-        public Notice giveNoSpace = Notice.chat("<red>✘ <dark_red>Błąd: <red>Brak miejsca w ekwipunku!");
+        public Notice giveNoSpace = Notice.chat("<red>✘ <dark_red>Błąd: <red>Brak miejsca w ekwipunku dla {ITEM}!");

---

Line range hint 1-756: Consider future refactoring for improved maintainability

While the current change is appropriate and minimal, the overall file structure is quite complex. For future consideration:

1. Consider splitting this large file into smaller, more focused files for each major section (e.g., chat, teleportation, inventory).
2. Implement a more modular approach to loading translations, which could improve performance and make it easier to add new languages.
3. Consider using a dedicated localization framework or library to manage translations more efficiently.

These suggestions are out of scope for the current PR but could be valuable for future maintenance and scalability of the plugin.

eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveService.java (1)

116-136: Consider optimizing the hasSpace method for efficiency

Currently, the hasSpace method first checks for an empty slot and then iterates through the inventory to find stackable items. Depending on the typical state of player inventories, checking for stackable items first might be more efficient, as it avoids using additional slots when existing ones can be used.

If optimizing for minimal slot usage is desired, you could adjust the order of the checks:

        for (ItemStack contents : inventory.getContents()) {
            if (contents == null) {
                continue;
            }

            if (contents.isSimilar(itemStack) && contents.getMaxStackSize() > contents.getAmount()) {
                return true;
            }
        }

        return inventory.firstEmpty() != -1;

However, if the current logic suits the game's inventory management strategy, no changes are necessary.

eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveCommand.java (1)

Line range hint 36-121: Consider refactoring to reduce code duplication among execute methods

The execute methods share similar logic with variations in parameters. To improve maintainability and reduce duplication, consider refactoring common code into a helper method or implementing a more generic approach.

Here's an example of how you might refactor the common logic into a helper method:

private void giveItemAndNotify(Viewer viewer, Player target, Material material, Integer amount, Enchantment enchantment, Integer level) {
    String formattedMaterial = MaterialUtil.format(material);

    if (amount == null) {
        amount = 1;
    }

    if (enchantment != null && level != null) {
        this.giveService.giveItem(target, material, amount, enchantment, level);
    } else {
        this.giveService.giveItem(target, material, amount);
    }

    NoticeBuilder noticeBuilder = this.noticeService.create()
        .placeholder("{ITEM}", formattedMaterial)
        .player(target.getUniqueId());

    if (enchantment != null && level != null) {
        noticeBuilder
            .placeholder("{ENCHANTMENT}", enchantment.getKey().getKey())
            .placeholder("{ENCHANTMENT_LEVEL}", String.valueOf(level))
            .notice(translation -> translation.item().giveReceivedEnchantment());
    } else {
        noticeBuilder.notice(translation -> translation.item().giveReceived());
    }

    noticeBuilder.send();

    if (!viewer.isSame(target)) {
        NoticeBuilder viewerNotice = this.noticeService.create()
            .placeholder("{ITEM}", formattedMaterial)
            .placeholder("{PLAYER}", target.getName())
            .viewer(viewer);

        if (enchantment != null && level != null) {
            viewerNotice
                .placeholder("{ENCHANTMENT}", enchantment.getKey().getKey())
                .placeholder("{ENCHANTMENT_LEVEL}", String.valueOf(level))
                .notice(translation -> translation.item().giveGivenEnchantment());
        } else {
            viewerNotice.notice(translation -> translation.item().giveGiven());
        }

        viewerNotice.send();
    }
}

You can then simplify your execute methods by calling this helper method with appropriate parameters.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3f84480 and ae4c931.

📒 Files selected for processing (6)

• eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (1 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveCommand.java (5 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveService.java (1 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java (1 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (1 hunks)

🧰 Additional context used

🔇 Additional comments (8)

eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java (1)

384-384: LGTM: New method aligns with PR objectives

The addition of the giveNoSpace() method is a good implementation that addresses the PR objective of providing clearer feedback when there's no available space in the inventory. The method name is clear, its return type is consistent with other methods in this interface, and its placement is logical within the ItemSection.

eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (1)

372-374: Summary: New configuration option successfully addresses PR objective

The addition of the dropOnFullInventory configuration option in the Items class successfully addresses one of the main objectives of the PR. It provides a way for users to control whether items should be dropped on the ground or not when a player's inventory is full during the execution of the /give command.

This change is well-implemented, properly documented, and doesn't introduce any issues to the existing codebase. It enhances the flexibility of the plugin, allowing server administrators to customize the behavior according to their preferences.

To ensure that this configuration option is properly utilized in the /give command implementation, please run the following verification script:

This script will help confirm that the new configuration option is being used correctly in the implementation of the /give command.

✅ Verification successful

Verification Successful: dropOnFullInventory is Properly Utilized

The dropOnFullInventory configuration option is correctly referenced in GiveService.java, ensuring that items are appropriately handled when a player's inventory is full during the /give command execution. No issues were detected regarding its implementation and usage within the relevant classes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of dropOnFullInventory configuration in GiveCommand or related classes

# Search for references to dropOnFullInventory in relevant files
rg --type java -g 'GiveCommand.java' -g 'GiveService.java' -g '*Give*.java' 'dropOnFullInventory'

Length of output: 418

eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveCommand.java (6)

23-28: Good use of GiveService to improve modularity

Refactoring to use GiveService centralizes the item-giving logic, enhancing modularity and maintainability of the code.

---

36-36: Delegation of item-giving to GiveService

Using giveService.giveItem(player, material) promotes code reuse and keeps the command logic clean.

---

50-50: Consistent use of GiveService for item distribution

Delegating item-giving to GiveService with giveService.giveItem(target, material) maintains consistency and improves code organization.

---

71-71: Item giving with amount parameter using GiveService

Utilizing giveService.giveItem(player, material, amount) ensures consistent handling of items with quantities.

---

85-85: Delegation with amount parameter to GiveService

Calling giveService.giveItem(target, material, amount) for item giving with quantity enhances code consistency.

---

106-106: Enhanced item giving with enchantments via GiveService

Using giveService.giveItem(target, material, amount, enchantment, level) centralizes and streamlines the process of giving enchanted items.

[coderabbitai]

Actionable comments posted: 5

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ae4c931 and 032dad7.

📒 Files selected for processing (1)

• eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveService.java (1 hunks)

🧰 Additional context used

🔇 Additional comments (5)

eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveService.java (5)

23-29: LGTM: Constructor properly initializes dependencies

The constructor is correctly annotated with @Inject for dependency injection. It initializes the pluginConfiguration and noticeService dependencies, and sets the defaultGiveAmount from the configuration. This approach ensures that the service is properly set up with its required dependencies.

---

31-37: LGTM: Method correctly handles default item giving

This overloaded giveItem method appropriately checks for invalid material before proceeding to give the item. It uses the defaultGiveAmount when calling the giveItems method, which is the expected behavior for this version of the method.

---

116-136: LGTM: Efficient inventory space checking

The hasSpace method is well-implemented and efficient. It correctly checks for:

1. Empty slots in the inventory.
2. Existing similar item stacks that have space for more items.

This approach ensures accurate detection of available space for new items, considering both completely empty slots and partially filled stacks.

---

138-140: LGTM: Concise and effective EnchantmentLevelPair record

The EnchantmentLevelPair record is a good use of Java's record feature. It provides a compact and clear way to associate an enchantment with its level. This approach enhances code readability and maintainability when dealing with enchantments throughout the class.

---

1-140: Overall assessment: Well-structured service with room for minor improvements

The GiveService class is generally well-implemented, providing a robust solution for giving items to players. Here's a summary of the main points:

1. The overall structure and dependency injection are well done.
2. Most methods are logically sound and efficient.
3. Areas for improvement include:
   • Adding input validation for amount and level parameters in public methods.
   • Fixing the inverted condition in isInvalidMaterial.
   • Improving user feedback in giveItemStack for different scenarios.
   • Using record accessor methods correctly for enchantments.

Addressing these points will enhance the reliability and user experience of the item giving functionality. Great job on the implementation, and these small tweaks will make it even better!

[Rollczi]

This way to resolve items is not safe because you can give itemstack and next resolved itemstack could have not space to put inside player inventory.

[vLuckyyy]

I think I fixed it, can you take a look?

2024-10-15.01-33-04.mp4

[Rollczi]

This is not the case. You should switch config dropItems to false.

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)

eternalcore-core/src/main/java/com/eternalcode/core/util/InventoryUtil.java (2)

7-9: Consider making the constructor private.

While throwing an exception in the constructor effectively prevents instantiation, it's more idiomatic to use a private constructor for utility classes. This approach prevents instantiation at compile-time rather than runtime.

Here's a suggested change:

-public InventoryUtil() {
+private InventoryUtil() {
    throw new UnsupportedOperationException("This is a utility class and cannot be instantiated");
}

---

11-31: LGTM: Method logic is correct. Consider using streams for a more concise implementation.

The hasSpace method correctly checks for available space in the inventory. It efficiently checks for an empty slot first before iterating through the contents. The null check, similarity check, and stack size check are all appropriate.

As a suggestion for improvement, consider using Java streams for a more concise implementation:

public static boolean hasSpace(Inventory inventory, ItemStack itemStack) {
    if (inventory.firstEmpty() != -1) {
        return true;
    }

    return Arrays.stream(inventory.getContents())
            .filter(Objects::nonNull)
            .filter(item -> item.isSimilar(itemStack))
            .anyMatch(item -> item.getAmount() < item.getMaxStackSize());
}

This implementation maintains the same logic but is more concise and potentially more readable.

eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveCommand.java (1)

Line range hint 1-124: Overall improvement in code structure and functionality

The changes in this file consistently implement the new GiveService across all execute methods, effectively addressing the PR objectives of improving item distribution and handling various scenarios. The refactoring maintains backward compatibility while enhancing the separation of concerns.

Suggestion for future improvement:
Consider refactoring the notification logic in the execute methods to reduce code duplication. A helper method could be introduced to handle the creation and sending of notifications, which would further improve the maintainability of the code.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3f84480 and f724ebd.

📒 Files selected for processing (7)

• eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (1 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveCommand.java (5 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveService.java (1 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java (1 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (1 hunks)
• eternalcore-core/src/main/java/com/eternalcode/core/util/InventoryUtil.java (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (5)

• eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
• eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveService.java
• eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java
• eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java
• eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java

🧰 Additional context used

🔇 Additional comments (7)

eternalcore-core/src/main/java/com/eternalcode/core/util/InventoryUtil.java (1)

1-6: LGTM: Class structure and package declaration are appropriate.

The class is correctly structured as a utility class within the com.eternalcode.core.util package. The final modifier prevents inheritance, which is a good practice for utility classes.

eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveCommand.java (6)

23-23: Improved design with GiveService integration

The introduction of GiveService and its integration in the constructor enhances the separation of concerns. This change aligns well with the PR objectives by moving the item-giving logic out of the command class, resulting in a more modular and maintainable design.

Also applies to: 26-28

---

36-36: Consistent use of GiveService

The replacement of the direct item-giving logic with a call to giveService.giveItem() is consistent with the new design. This change maintains the method's functionality while leveraging the new service-oriented approach.

---

50-50: Consistent use of GiveService for target player

The implementation of giveService.giveItem() for giving items to a target player is consistent with the new design. This change maintains the method's functionality while adhering to the new service-oriented approach.

---

71-71: Effective use of GiveService for custom amounts

The implementation of giveService.giveItem() with a custom amount parameter effectively delegates the responsibility of handling different item quantities to the GiveService. This change maintains the method's functionality while leveraging the new service-oriented approach and addresses the PR objective of fixing issues related to item stack sizes.

---

85-85: Consistent use of GiveService for target player with custom amounts

The implementation of giveService.giveItem() for giving items with a custom amount to a target player is consistent with the new design. This change maintains the method's functionality while adhering to the new service-oriented approach and addresses the PR objective of handling different item quantities correctly.

---

106-106: Comprehensive use of GiveService for enchanted items

The implementation of giveService.giveItem() with parameters for target player, material, amount, enchantment, and level demonstrates a comprehensive approach to item distribution. This change effectively delegates the complex logic of creating and distributing enchanted items to the GiveService, maintaining consistency with the new design while addressing the PR objectives related to item handling and enchantments.

[igoyek]

If you tested it, looks good, nice job ;)

[Rollczi]

I wait for fix