Stack items with expiration dates - #303
Conversation
WalkthroughThe changes adjust the stacking logic in the inventory and item collection classes. In the InventoryManager, the Add method now always passes true for stacking by removing the conditional check on the item’s expiry time. In the ItemCollection, the Add method’s return syntax is simplified and the CanStack method is enhanced to include a check that compares item expiry times at a minute granularity. Both modifications maintain their original method signatures while streamlining internal logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InventoryManager
participant ItemCollection
User->>InventoryManager: Call Add(item, notifyNew, commit)
InventoryManager->>ItemCollection: Call Add(item, stack=true)
Note right of ItemCollection: Evaluate stacking with updated expiry check
ItemCollection-->>InventoryManager: Return result
InventoryManager-->>User: Return result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Maple2.Server.Game/Manager/Items/ItemCollection.cs (1)
321-322: Consider edge cases in expiry time comparison.The changes correctly implement stacking based on expiry times at minute granularity. However, consider these improvements:
- Integer division might truncate seconds differently for times close to minute boundaries
- No null checks for
ExpiryTimeproperty- No validation that
ExpiryTimeis non-negativeConsider this safer implementation:
- && Equals(item.Transfer, stack.Transfer) - && item.ExpiryTime / 60 == stack.ExpiryTime / 60; // Expiry time is in seconds, convert to minutes + && Equals(item.Transfer, stack.Transfer) + && (item.ExpiryTime == null && stack.ExpiryTime == null || // Both items have no expiry + item.ExpiryTime != null && stack.ExpiryTime != null && // Both items have expiry + item.ExpiryTime >= 0 && stack.ExpiryTime >= 0 && // Validate non-negative + Math.Abs(item.ExpiryTime.Value - stack.ExpiryTime.Value) < 60); // Within same minuteMaple2.Server.Game/Manager/Items/InventoryManager.cs (1)
203-207: Consider adding test coverage for stacking items with different expiry times.The current test file only covers the loyalty discount calculation. Consider adding test cases to verify:
- Stacking items with different expiry times
- Stacking items with no expiry time
- Edge cases around item stacking limits
Would you like me to generate test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.Server.Game/Manager/Items/InventoryManager.cs(1 hunks)Maple2.Server.Game/Manager/Items/ItemCollection.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
Maple2.Server.Game/Manager/Items/ItemCollection.cs (1)
146-146: Verify C# version compatibility for collection expressions.The code uses C# 12's collection expressions feature (
[]syntax). Ensure that the project targets a compatible .NET version.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check .NET SDK version and C# language version in the project file # Find and check all .csproj files fd -e csproj -x cat {} \; | grep -E "TargetFramework|LangVersion"Length of output: 1042
C# Version Compatibility Verified for Collection Expressions
The project targets .NET 8.0 with LangVersion set to 12, which fully supports the C# 12 collection expressions syntax used at line 146. No compatibility issues were found.
Maple2.Server.Game/Manager/Items/InventoryManager.cs (1)
203-203: Verify the implications of always allowing item stacking.The change to always pass
truefor stacking could affect how items with different expiry times are handled. This might lead to unexpected behavior if items with different expiry times are stacked together.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for any special handling of item expiry times in stacking logic # Look for references to ExpiryTime in relation to stacking # Search for ExpiryTime usage in stacking context ast-grep --pattern 'ExpiryTime' # Search for stack-related methods that might be affected rg -A 5 'CanStack|GetStackResult'Length of output: 8324
Verify potential expiry time discrepancies when forcing stacking behavior.
The change to always pass
stack: truemeans that stacking will always be attempted. However, the underlying logic in ItemCollection.cs (specifically in the CanStack method) still verifies that items can only stack if their expiry times (when converted to minutes) match, along with other properties like Id, Rarity, and Transfer. In other words, even with stacking always enabled at the InventoryManager level, items with mismatched expiry times won’t stack together.
- Confirm that the strict expiry time check (
item.ExpiryTime / 60 == stack.ExpiryTime / 60) continues to prevent unintended stacking of items with different expiry values.- It may be beneficial to add or review tests covering edge cases where items have slightly different expiry times to ensure that they are correctly handled (i.e., allocated to separate slots rather than being forced to stack).
2 participants
Summary by CodeRabbit
Refactor
Style