refactor: implement ISlottedItemStorage in Container and NeoForge storage#400
Merged
Indemnity83 merged 3 commits intoMay 31, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ISlottedItemStorage and updates ContainerItemStorage to implement slotCount/slotView and slot-scoped insert/extract (exposed→actual slot mapping, face permission checks, simulation semantics). NeoForgeItemStorage.asNeoForge now returns a SlottedCommonItemHandler for ISlottedItemStorage inputs, backed by a SnapshotJournal that tracks per-(slot,item) pending deltas and applies net changes on commit. Adds unit tests for container and NeoForge adapters and adjusts NeoForge test classpath in Gradle. Sequence Diagram(s)sequenceDiagram
participant Client
participant SlottedCommonItemHandler
participant SnapshotJournal
participant ISlottedItemStorage
Client->>SlottedCommonItemHandler: insert(slot, item, amount, simulate/tx)
SlottedCommonItemHandler->>SnapshotJournal: record pending delta(slot,item,+amount)
SlottedCommonItemHandler->>SnapshotJournal: simulate availability vs pending
SnapshotJournal-->>SlottedCommonItemHandler: simulated result
alt commit
SlottedCommonItemHandler->>ISlottedItemStorage: insert(actualSlot, item, netAmount, simulate=false)
ISlottedItemStorage-->>SlottedCommonItemHandler: applied amount
SlottedCommonItemHandler->>SnapshotJournal: clear pending deltas
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
common/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.java (1)
171-179: ⚡ Quick winCache sided slot array once in
actualSlotfor consistent bounds+mapping.This avoids repeated
getSlotsForFace(side)lookups and keeps bounds checks tied to the exact same array used for index mapping.Proposed refactor
private int actualSlot(int exposedSlot) { - if (exposedSlot < 0 || exposedSlot >= slotCount()) { - throw new IndexOutOfBoundsException(exposedSlot); - } if (side != null && container instanceof WorldlyContainer wc) { - return wc.getSlotsForFace(side)[exposedSlot]; + int[] slots = wc.getSlotsForFace(side); + if (exposedSlot < 0 || exposedSlot >= slots.length) { + throw new IndexOutOfBoundsException(exposedSlot); + } + return slots[exposedSlot]; } + if (exposedSlot < 0 || exposedSlot >= container.getContainerSize()) { + throw new IndexOutOfBoundsException(exposedSlot); + } return exposedSlot; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.java` around lines 171 - 179, The actualSlot method currently calls wc.getSlotsForFace(side) twice which can yield inconsistent bounds vs mapping; cache the int[] slots = wc.getSlotsForFace(side) in actualSlot when container instanceof WorldlyContainer and side != null, validate exposedSlot against slots.length (instead of slotCount()), and then return slots[exposedSlot]; keep the existing fallback return exposedSlot and the initial negative/upper bound check for non-sided cases, updating checks to use the cached slots array so bounds and mapping use the same data.common/src/main/java/com/logistics/core/lib/storage/ISlottedItemStorage.java (1)
16-44: ⚡ Quick winDocument slot bounds and invalid-amount behavior in the public API.
Please specify contract behavior for out-of-range
slotvalues andmaxAmount <= 0(e.g., throwIndexOutOfBoundsExceptionvs return0) so implementations/adapters stay consistent.Proposed doc update
/** * Read a single slot. * * `@param` slot the exposed slot index * `@return` the non-empty slot view, or {`@code` null} when the slot is empty + * `@throws` IndexOutOfBoundsException when {`@code` slot} is outside {`@code` [0, slotCount())} */ `@Nullable` IItemView slotView(int slot); /** * Insert into a single exposed slot. @@ * `@param` maxAmount the maximum number of items to insert * `@param` simulate whether to leave backing state unchanged * `@return` the amount inserted, or that would be inserted when simulating + * `@throws` IndexOutOfBoundsException when {`@code` slot} is outside {`@code` [0, slotCount())} */ long insert(int slot, IItemKey item, long maxAmount, boolean simulate); /** * Extract from a single exposed slot. @@ * `@param` maxAmount the maximum number of items to extract * `@param` simulate whether to leave backing state unchanged * `@return` the amount extracted, or that would be extracted when simulating + * `@throws` IndexOutOfBoundsException when {`@code` slot} is outside {`@code` [0, slotCount())} */ long extract(int slot, IItemKey item, long maxAmount, boolean simulate);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/src/main/java/com/logistics/core/lib/storage/ISlottedItemStorage.java` around lines 16 - 44, Update the Javadoc for ISlottedItemStorage's methods (slotView, insert, extract) to explicitly document bounds and invalid-amount behavior: state that an out-of-range slot index will throw IndexOutOfBoundsException (rather than returning null or silently ignoring), and that a non-positive maxAmount (maxAmount <= 0) is treated as a no-op and the method returns 0 (no items inserted/extracted) even when simulate is true; keep existing contract that slotView returns null for an empty slot but still throws IndexOutOfBoundsException for invalid slot indexes.neoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemStorage.java (1)
136-138: 💤 Low valueMinor inefficiency: redundant map lookup after finding key.
firstPositivePendingalready iterates the map and finds the entry value, but thengetAmountAsLongperforms another lookup. Consider returning the amount directly fromfirstPositivePendingor caching it.♻️ Possible optimization
- `@Nullable` - private SlotItemKey firstPositivePending(int slot) { + private long firstPositivePendingAmount(int slot) { for (var entry : pendingDeltas.entrySet()) { if (entry.getKey().slot() == slot && entry.getValue() > 0) { - return entry.getKey(); + return entry.getValue(); } } - return null; + return 0; }Then update callers accordingly, or keep the current approach if the key is needed elsewhere.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@neoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemStorage.java` around lines 136 - 138, firstPositivePending currently finds a SlotItemKey with a positive pending delta but callers such as getAmountAsLong immediately do a second lookup in pendingDeltas; change firstPositivePending (or add an overload) to return the amount directly (e.g., return Long or OptionalLong alongside the SlotItemKey) so callers like getAmountAsLong can use the returned amount without a redundant pendingDeltas.getOrDefault call, and update any callers of firstPositivePending/getAmountAsLong accordingly to consume the amount-returning API.neoforge/src/test/java/com/logistics/neoforge/storage/NeoForgeItemStorageTest.java (1)
17-37: ⚡ Quick winConsider adding tests for insert/extract operations and transaction semantics.
The current tests verify slot count detection and empty slot behavior. Consider adding tests for:
- Insert/extract with actual item resources and amounts
- Transaction commit vs simulation (rollback)
- Out-of-bounds index handling (should throw
IndexOutOfBoundsException)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@neoforge/src/test/java/com/logistics/neoforge/storage/NeoForgeItemStorageTest.java` around lines 17 - 37, Add tests exercising insert/extract and transaction behavior using NeoForgeItemStorage.asNeoForge: create handlers from a populated slotted storage (EmptySlottedStorage -> replace with a small test storage filled via handler.insert) and from non-slotted storage (EmptyStorage) and assert insert returns expected inserted amount, getResource/getAmountAsLong reflect committed values after a committed Transaction and do not change after a simulated Transaction (rollback). Also add tests that calling handler.insert/handler.extract with an out-of-range index (e.g., index -1 or index >= handler.size()) throws IndexOutOfBoundsException. Reference ResourceHandler methods size(), insert(...), extract(...), getResource(int), getAmountAsLong(int) and NeoForgeItemStorage.asNeoForge/EmptySlottedStorage/EmptyStorage when adding these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@common/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.java`:
- Around line 171-179: The actualSlot method currently calls
wc.getSlotsForFace(side) twice which can yield inconsistent bounds vs mapping;
cache the int[] slots = wc.getSlotsForFace(side) in actualSlot when container
instanceof WorldlyContainer and side != null, validate exposedSlot against
slots.length (instead of slotCount()), and then return slots[exposedSlot]; keep
the existing fallback return exposedSlot and the initial negative/upper bound
check for non-sided cases, updating checks to use the cached slots array so
bounds and mapping use the same data.
In
`@common/src/main/java/com/logistics/core/lib/storage/ISlottedItemStorage.java`:
- Around line 16-44: Update the Javadoc for ISlottedItemStorage's methods
(slotView, insert, extract) to explicitly document bounds and invalid-amount
behavior: state that an out-of-range slot index will throw
IndexOutOfBoundsException (rather than returning null or silently ignoring), and
that a non-positive maxAmount (maxAmount <= 0) is treated as a no-op and the
method returns 0 (no items inserted/extracted) even when simulate is true; keep
existing contract that slotView returns null for an empty slot but still throws
IndexOutOfBoundsException for invalid slot indexes.
In
`@neoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemStorage.java`:
- Around line 136-138: firstPositivePending currently finds a SlotItemKey with a
positive pending delta but callers such as getAmountAsLong immediately do a
second lookup in pendingDeltas; change firstPositivePending (or add an overload)
to return the amount directly (e.g., return Long or OptionalLong alongside the
SlotItemKey) so callers like getAmountAsLong can use the returned amount without
a redundant pendingDeltas.getOrDefault call, and update any callers of
firstPositivePending/getAmountAsLong accordingly to consume the amount-returning
API.
In
`@neoforge/src/test/java/com/logistics/neoforge/storage/NeoForgeItemStorageTest.java`:
- Around line 17-37: Add tests exercising insert/extract and transaction
behavior using NeoForgeItemStorage.asNeoForge: create handlers from a populated
slotted storage (EmptySlottedStorage -> replace with a small test storage filled
via handler.insert) and from non-slotted storage (EmptyStorage) and assert
insert returns expected inserted amount, getResource/getAmountAsLong reflect
committed values after a committed Transaction and do not change after a
simulated Transaction (rollback). Also add tests that calling
handler.insert/handler.extract with an out-of-range index (e.g., index -1 or
index >= handler.size()) throws IndexOutOfBoundsException. Reference
ResourceHandler methods size(), insert(...), extract(...), getResource(int),
getAmountAsLong(int) and
NeoForgeItemStorage.asNeoForge/EmptySlottedStorage/EmptyStorage when adding
these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b181f40c-d98f-4c12-8448-d383f11defd4
📒 Files selected for processing (6)
common/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.javacommon/src/main/java/com/logistics/core/lib/storage/ISlottedItemStorage.javacommon/src/test/java/com/logistics/core/lib/storage/ContainerItemStorageTest.javaneoforge/build.gradleneoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemStorage.javaneoforge/src/test/java/com/logistics/neoforge/storage/NeoForgeItemStorageTest.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
neoforge/src/test/java/com/logistics/neoforge/storage/NeoForgeItemStorageTest.java (1)
31-46: ⚡ Quick winConsider consolidating the duplicate boundary tests.
Both
slottedHandler_rejectsOutOfRangeTransferIndexesandnonSlottedHandler_rejectsOutOfRangeTransferIndexestest identical boundary conditions with only the storage type differing. Refactoring to a@ParameterizedTestwith a@MethodSourcewould eliminate duplication and make it easier to add new storage variants in the future.♻️ Example refactor using parameterized test
+import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; +import java.util.stream.Stream; + -@Test -@DisplayName("slotted handler rejects out-of-range transfer indexes") -void slottedHandler_rejectsOutOfRangeTransferIndexes() { - ResourceHandler<ItemResource> handler = NeoForgeItemStorage.asNeoForge(new EmptySlottedStorage(1)); - - try (Transaction tx = Transaction.openRoot()) { - assertThatThrownBy(() -> handler.insert(-1, ItemResource.EMPTY, 1, tx)) - .isInstanceOf(IndexOutOfBoundsException.class); - assertThatThrownBy(() -> handler.insert(handler.size(), ItemResource.EMPTY, 1, tx)) - .isInstanceOf(IndexOutOfBoundsException.class); - assertThatThrownBy(() -> handler.extract(-1, ItemResource.EMPTY, 1, tx)) - .isInstanceOf(IndexOutOfBoundsException.class); - assertThatThrownBy(() -> handler.extract(handler.size(), ItemResource.EMPTY, 1, tx)) - .isInstanceOf(IndexOutOfBoundsException.class); - } -} - -@Test -@DisplayName("non-slotted handler rejects out-of-range transfer indexes") -void nonSlottedHandler_rejectsOutOfRangeTransferIndexes() { - ResourceHandler<ItemResource> handler = NeoForgeItemStorage.asNeoForge(new EmptyStorage()); +@ParameterizedTest(name = "{0}") +@MethodSource("storageVariants") +void handler_rejectsOutOfRangeTransferIndexes(String displayName, IItemStorage storage) { + ResourceHandler<ItemResource> handler = NeoForgeItemStorage.asNeoForge(storage); try (Transaction tx = Transaction.openRoot()) { assertThatThrownBy(() -> handler.insert(-1, ItemResource.EMPTY, 1, tx)) .isInstanceOf(IndexOutOfBoundsException.class); assertThatThrownBy(() -> handler.insert(handler.size(), ItemResource.EMPTY, 1, tx)) .isInstanceOf(IndexOutOfBoundsException.class); assertThatThrownBy(() -> handler.extract(-1, ItemResource.EMPTY, 1, tx)) .isInstanceOf(IndexOutOfBoundsException.class); assertThatThrownBy(() -> handler.extract(handler.size(), ItemResource.EMPTY, 1, tx)) .isInstanceOf(IndexOutOfBoundsException.class); } } + +private static Stream<org.junit.jupiter.params.provider.Arguments> storageVariants() { + return Stream.of( + org.junit.jupiter.params.provider.Arguments.of("slotted handler", new EmptySlottedStorage(1)), + org.junit.jupiter.params.provider.Arguments.of("non-slotted handler", new EmptyStorage()) + ); +}Also applies to: 58-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@neoforge/src/test/java/com/logistics/neoforge/storage/NeoForgeItemStorageTest.java` around lines 31 - 46, Consolidate the two duplicate tests into a single `@ParameterizedTest` that takes a ResourceHandler<ItemResource> (or a factory) from a `@MethodSource`; replace the existing slottedHandler_rejectsOutOfRangeTransferIndexes and nonSlottedHandler_rejectsOutOfRangeTransferIndexes with one method (e.g., outOfRangeTransferIndexes) that, given a handler produced by MethodSource (provide handlers like NeoForgeItemStorage.asNeoForge(new EmptySlottedStorage(1)) and NeoForgeItemStorage.asNeoForge(new EmptyNonSlottedStorage(1))), opens Transaction.openRoot() and asserts that handler.insert(-1,...), handler.insert(handler.size(),...), handler.extract(-1,...), and handler.extract(handler.size(),...) each throw IndexOutOfBoundsException (use ItemResource.EMPTY and the same quantity and tx); add the MethodSource provider method to return the two handler instances for reuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@neoforge/src/test/java/com/logistics/neoforge/storage/NeoForgeItemStorageTest.java`:
- Around line 31-46: Consolidate the two duplicate tests into a single
`@ParameterizedTest` that takes a ResourceHandler<ItemResource> (or a factory)
from a `@MethodSource`; replace the existing
slottedHandler_rejectsOutOfRangeTransferIndexes and
nonSlottedHandler_rejectsOutOfRangeTransferIndexes with one method (e.g.,
outOfRangeTransferIndexes) that, given a handler produced by MethodSource
(provide handlers like NeoForgeItemStorage.asNeoForge(new
EmptySlottedStorage(1)) and NeoForgeItemStorage.asNeoForge(new
EmptyNonSlottedStorage(1))), opens Transaction.openRoot() and asserts that
handler.insert(-1,...), handler.insert(handler.size(),...),
handler.extract(-1,...), and handler.extract(handler.size(),...) each throw
IndexOutOfBoundsException (use ItemResource.EMPTY and the same quantity and tx);
add the MethodSource provider method to return the two handler instances for
reuse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e40bc84-382f-4d41-8399-f2751c7b3f55
📒 Files selected for processing (4)
common/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.javacommon/src/main/java/com/logistics/core/lib/storage/ISlottedItemStorage.javaneoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemStorage.javaneoforge/src/test/java/com/logistics/neoforge/storage/NeoForgeItemStorageTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- common/src/main/java/com/logistics/core/lib/storage/ISlottedItemStorage.java
- common/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.java
- neoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemStorage.java
Indemnity83
added a commit
that referenced
this pull request
May 31, 2026
Indemnity83
added a commit
that referenced
this pull request
May 31, 2026
This was referenced May 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This update introduces significant enhancements to the item storage capabilities within the logistics system. The implementation of the
ISlottedItemStorageinterface allows for more efficient and organized item management in containers, enhancing the flexibility of item insertion and extraction while maintaining compatibility with existing systems.Changes
New Interface Implementation
ISlottedItemStorageinterface to define a contract for item storages with addressable slots.ContainerItemStorageclass to implementISlottedItemStorage, allowing for operations on specific slots.Item Insertion and Extraction
Enhanced Testing
ContainerItemStorageto validate the new slot-based item operations.Notes