Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces nullability constraints and default initialization across database and game models. Key changes include marking properties as Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Maple2.Tools/Extensions/PacketExtensions.cs (1)
64-71:⚠️ Potential issue | 🔴 Critical
ReadArray<T>always returns an all-default array — loop condition is inverted.Line 66 uses
i > sizeinstead ofi < size. Sinceistarts at0andsizeis expected to be a positive count, the condition isfalseon the very first iteration, the loop body never runs, and all elements remain at their default value.🐛 Proposed fix
- for (int i = 0; i > size; i++) { + for (int i = 0; i < size; i++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Tools/Extensions/PacketExtensions.cs` around lines 64 - 71, The for-loop in ReadArray<T>(this IByteReader packet, int size) has its condition inverted (uses i > size) so the body never executes; change the loop condition to i < size (keeping var result = new T[size]; and result[i] = packet.Read<T>();) so the method fills the array from 0 to size-1; also consider validating that size is non-negative before allocating if you want additional safety.Maple2.File.Ingest/Mapper/TableMapper.cs (1)
41-46:⚠️ Potential issue | 🟡 MinorRemove the unused
languagefield — it is assigned but never read.The assignment at line 46 eliminates the "field never assigned" warning (CS0649), but introduces "assigned but value never used" (IDE0052). The field is declared, assigned in the constructor, and never referenced again. All locale-sensitive work flows through
parser(line 44), which already received the language parameter directly.The cleanest fix is to remove the field declaration entirely:
Proposed fix
private readonly TableParser parser; private readonly ItemOptionParser optionParser; -private readonly string language; public TableMapper(M2dReader xmlReader, string language) { parser = new TableParser(xmlReader, language); optionParser = new ItemOptionParser(xmlReader); - this.language = language; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Ingest/Mapper/TableMapper.cs` around lines 41 - 46, The private field 'language' in TableMapper is assigned in the TableMapper constructor but never used; remove the unused field declaration and its assignment in the constructor so only parser (created via new TableParser(xmlReader, language)) and optionParser remain; ensure no other code references the 'language' field before deleting it and run a build to confirm the IDE0052 warning is resolved.Maple2.Server.Tests/Validators/CharacterNameValidatorTests.cs (2)
10-14:⚠️ Potential issue | 🟡 MinorTest method name is inverted; comment is stale.
ValidName_ShouldReturnFalseassertsIs.True— the name says the opposite of what the test verifies. The inline comment on line 11 also describes the return as "null (no error)", butValidNamereturnsbool. Both should be corrected to avoid confusion in test-failure reports.🐛 Proposed fix
- public void ValidName_ShouldReturnFalse() { - // Valid names should return null (no error) + public void ValidName_ShouldReturnTrue() { + // Valid names should return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Tests/Validators/CharacterNameValidatorTests.cs` around lines 10 - 14, The test method name ValidName_ShouldReturnFalse is incorrect and the inline comment is stale; rename the test to something like ValidName_ShouldReturnTrue (or ValidName_ReturnsTrue) and update the comment to state that NameValidator.ValidName returns a bool and true indicates a valid name; keep the existing assertions using NameValidator.ValidName("ValidName") and NameValidator.ValidName("Test123") but change the comment text to reflect "true = valid" so test name, comment, and assertions are consistent.
171-176:⚠️ Potential issue | 🟡 MinorRemove duplicate assertions — all six lines are byte-for-byte identical.
Lines 171–173 and 174–176 contain exact duplicates. The hex analysis confirms both sets are NFC-encoded identically (
José: 4a6f73c3a9,Renée: 52656ec3a965,Beyoncé: 4265796f6e63c3a9), so there is no intentional Unicode normalization coverage here. These are accidental duplicates and should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Tests/Validators/CharacterNameValidatorTests.cs` around lines 171 - 176, The test contains duplicate assertions repeated twice — remove the second set of identical lines so only one assertion per Unicode name remains; specifically, in CharacterNameValidatorTests remove the duplicate calls to Assert.That(NameValidator.ValidName("José"), Is.True), Assert.That(NameValidator.ValidName("Renée"), Is.True), and Assert.That(NameValidator.ValidName("Beyoncé"), Is.True) and keep the single original set that calls NameValidator.ValidName.Maple2.Database/Storage/Game/GameStorage.Shop.cs (1)
60-65:⚠️ Potential issue | 🟠 Major
GetCharacterShopItemDatapropagatesnullelements after the newToShopItemDataguard.Prior to this PR,
ToShopItemDataonly returnednullfor anullmodel, whichSelectover a live EF query never produces. The new guard at lines 94–96 meansToShopItemDatacan now returnnullfor a non-null model whenmodel.Item == null.Select(ToShopItemData)will then include thosenullentries in the resulting list; the!only suppresses the compiler warning about the list itself — it does not filter the elements.
ShopManager's constructor iterates this collection and immediately accessesitemData.ShopId, causing aNullReferenceExceptionfor any persisted record whoseItemJSON column isnull. The null-forgiving!atShopManager.csline 168 (Item = itemData.Item!) is a downstream symptom of the same root cause.🛡️ Proposed fix — filter out unconvertible entries
public ICollection<CharacterShopItemData> GetCharacterShopItemData(long ownerId) { return Context.CharacterShopItemData.Where(data => data.OwnerId == ownerId) .AsEnumerable() .Select(ToShopItemData) - .ToList()!; + .OfType<CharacterShopItemData>() + .ToList(); }
OfType<CharacterShopItemData>()discards allnullresults and removes the need for the suppression operator. With this fix, everyitemDatain the collection is guaranteed non-null, anditemData.Itemis also non-null (guaranteed byToShopItemData's own logic), so the null-forgiving!inShopManager.csline 168 can safely be removed as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Database/Storage/Game/GameStorage.Shop.cs` around lines 60 - 65, GetCharacterShopItemData currently calls .Select(ToShopItemData) which can yield null entries when ToShopItemData returns null for models with model.Item == null; change the pipeline to filter those out by using .Select(ToShopItemData).OfType<CharacterShopItemData>().ToList() (removing the null-forgiving !), so the returned collection contains only non-null CharacterShopItemData; then update ShopManager's constructor usage (where it reads itemData.ShopId and sets Item = itemData.Item!) to remove the unnecessary null-forgiving operator since items are now guaranteed non-null.
🧹 Nitpick comments (5)
Maple2.File.Ingest/Mapper/TriggerMapper.cs (1)
12-13: Scope the warning suppression to the affected block instead of the whole file.Both directives are placed at file level, so they silence CS0162 (and the ReSharper equivalent) for every line in the file. The only actual unreachable block is the
if (Constant.DebugTriggers)section at line 52. Suppressing file-wide means any future unreachable-code bugs introduced elsewhere in this file will be silently ignored.♻️ Proposed fix: scope suppression to the debug block
Remove the file-level directives (lines 12–13):
-// ReSharper disable HeuristicUnreachableCode -#pragma warning disable CS0162 // Unreachable code detectedAnd wrap only the affected block:
+ // ReSharper disable once HeuristicUnreachableCode +#pragma warning disable CS0162 if (Constant.DebugTriggers) { // for debugging purposes ... } +#pragma warning restore CS0162🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Ingest/Mapper/TriggerMapper.cs` around lines 12 - 13, The file currently disables ReSharper heuristic and CS0162 at file scope; remove those top-level directives and instead scope the suppression to only the unreachable block by placing the ReSharper disable and `#pragma` warning disable CS0162 immediately before the if (Constant.DebugTriggers) block inside TriggerMapper (the debug-only block around line with the if (Constant.DebugTriggers)) and then restore ReSharper and `#pragma` warning restore CS0162 immediately after that block so the rest of the file keeps normal unreachable-code warnings.Maple2.Server.Game/Util/TriggerStorage.cs (1)
10-11: Consider scoping the warning suppressions to the affected block rather than the entire file.Both the ReSharper directive and
#pragma warning disable CS0162are applied file-wide, which could silently hide legitimate unreachable-code issues added elsewhere in this file in the future. Tightening them around the specificif (Constant.DebugTriggers)block is safer and communicates intent more clearly.♻️ Proposed narrowing of suppression scope
Remove the file-level directives at lines 10–11:
-// ReSharper disable HeuristicUnreachableCode -#pragma warning disable CS0162 // Unreachable code detectedAnd wrap only the affected conditional in
TryGet:+ // ReSharper disable HeuristicUnreachableCode +#pragma warning disable CS0162 // Unreachable code detected if (!Constant.DebugTriggers) { ... } else { ... } +#pragma warning restore CS0162 + // ReSharper restore HeuristicUnreachableCode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Util/TriggerStorage.cs` around lines 10 - 11, Remove the file-wide ReSharper disable and `#pragma` warning disable CS0162 and instead apply those suppressions locally around the specific conditional in TryGet that checks if (Constant.DebugTriggers); i.e., delete the top-of-file directives, then wrap only the block of code inside the TryGet (the if (Constant.DebugTriggers) { ... }) with the ReSharper disable and `#pragma` warning disable/restore so the suppression covers just that conditional and is restored immediately after.Maple2.Server.Game/Commands/HomeCommands/RandomNumberCommand.cs (1)
9-10: Scope the warning suppressions to the specific block rather than the entire file.Both directives are file-level with no matching
restore:// ReSharper disable HeuristicUnreachableCode `#pragma` warning disable CS0162This silences the warnings across the whole file, which can mask legitimate CS0162 diagnostics introduced later. Consider either:
- Tight pragma scope — wrap only the unreachable block:
♻️ Proposed refactor — scoped suppression
-// ReSharper disable HeuristicUnreachableCode -#pragma warning disable CS0162 // Unreachable code detected ... + // ReSharper disable once HeuristicUnreachableCode +#pragma warning disable CS0162 // Unreachable code detected if (!Constant.EnableRollEverywhere) { return; } +#pragma warning restore CS0162 var interfaceText = new InterfaceText(StringCode.s_ugcmap_fun_roll, character.Name, rng.ToString()); session.Field.Broadcast(NoticePacket.Notice(NoticePacket.Flags.Message, interfaceText));
#ifconditional compilation — ifEnableRollEverywhereis a build-time toggle,#ifeliminates the need for suppression entirely and is the idiomatic C# pattern for compile-time feature flags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Commands/HomeCommands/RandomNumberCommand.cs` around lines 9 - 10, Current file-wide suppression of ReSharper's HeuristicUnreachableCode and pragma CS0162 should be narrowed: remove the top-of-file directives and instead place scoped suppression immediately around the unreachable code in the RandomNumberCommand class (e.g., around the specific method or conditional block inside RandomNumberCommand.Execute/Handle) by adding // ReSharper disable HeuristicUnreachableCode and `#pragma` warning disable CS0162 just before that block and corresponding // ReSharper restore HeuristicUnreachableCode and `#pragma` warning restore CS0162 immediately after; alternatively, if the unreachable path is controlled by a compile-time flag (EnableRollEverywhere), replace the suppressed block with `#if` ENABLEROLLEVERYWHERE ... `#endif` around the code to avoid suppressions entirely.Maple2.Server.Tests/Validators/CharacterNameValidatorTests.cs (1)
3-4: Scope the warning suppression to the affected guards rather than the entire file.Placing
#pragma warning disable CS0162at file level prevents the compiler from detecting any genuinely unreachable code added anywhere in this file in the future. Since the only affected paths are the fourif (!Constant.AllowUnicodeInNames) { return; }guards, prefer restoring the warning immediately after each guard (or wrapping just those lines).♻️ Example: per-guard scoping instead of file-level disable
-// ReSharper disable HeuristicUnreachableCode -#pragma warning disable CS0162 // Unreachable code detectedThen at each Unicode-conditional guard, e.g.:
+ // ReSharper disable once HeuristicUnreachableCode +#pragma warning disable CS0162 if (!Constant.AllowUnicodeInNames) { return; } +#pragma warning restore CS0162🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Tests/Validators/CharacterNameValidatorTests.cs` around lines 3 - 4, Instead of disabling CS0162 for the whole file, limit the suppression to just the unreachable guards: around each occurrence of the Unicode guard (the if (!Constant.AllowUnicodeInNames) { return; } checks used in the CharacterNameValidatorTests) wrap or annotate only that statement block with `#pragma` warning disable CS0162 immediately before it and `#pragma` warning restore CS0162 immediately after it (or place the single guard line inside a scoped pragma) so the warning remains enabled for the rest of the file.Maple2.Model/Validators/CharacterNameValidator.cs (1)
27-29: Redundant guard — logically dead code.Both regex patterns (
^[A-Za-z0-9]+$and^[\p{L}0-9]+$) exclusively match alphanumeric characters, so anynamethat reaches line 27 is already guaranteed to contain only letters and/or digits.name.All(c => !char.IsLetterOrDigit(c))will always evaluate tofalsehere and the block can never be entered.♻️ Proposed cleanup
- // Check for names that are only special characters - if (name.All(c => !char.IsLetterOrDigit(c))) { - return false; - } - return true; // Valid name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Model/Validators/CharacterNameValidator.cs` around lines 27 - 29, The guard checking name.All(c => !char.IsLetterOrDigit(c)) in CharacterNameValidator.cs is redundant because the preceding regex checks (patterns ^[A-Za-z0-9]+$ and ^[\p{L}0-9]+$) already ensure the string contains only letters/digits; remove this dead if-block to simplify the Validate logic (locate the conditional using name.All(...) in the validation method and delete the entire if branch).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.Database/Model/Quest.cs`:
- Line 18: The property Conditions in class Quest uses the C# 12 collection
expression "[]" which SortedDictionary<TKey,TValue> doesn't support; replace the
collection expression with an explicit constructor call (use new
SortedDictionary<int, QuestCondition>() or new()) when initializing Conditions
to ensure proper runtime/compile behavior for the SortedDictionary<int,
QuestCondition> property in Quest.cs.
In `@Maple2.Server.Game/Manager/ShopManager.cs`:
- Around line 167-170: Remove the null-forgiving operator on itemData.Item in
the instancedShop.Items assignment and ensure upstream data is filtered so
itemData.Item cannot be null: update GetCharacterShopItemData to exclude null
results produced by ToShopItemData (so every entry in dataDictionary has a
non-null Item) and then change the line that constructs new
ShopItem(shopItem.Metadata) in ShopManager (instancedShop.Items[shopItemId] =
...) to use itemData.Item without the trailing '!'. This pairs the caller change
(remove '!') with the data-source fix (filter nulls in
GetCharacterShopItemData/ToShopItemData) so runtime nulls cannot occur.
In `@Maple2.Server.Game/Model/Field/Actor/Actor.cs`:
- Line 26: The NpcMetadata property is declared nullable but the constructor
parameter NpcMetadataStorage npcMetadata (and assignments on the constructor)
guarantee it is always provided; change the property declaration from a nullable
to a required property by replacing "public NpcMetadataStorage? NpcMetadata {
get; init; }" with a required non-nullable declaration (e.g., "public required
NpcMetadataStorage NpcMetadata { get; init; }") so callers and overrides like
FieldActor and call sites using .TryGet()/.GetAnimation() no longer see a false
nullable signal; ensure the constructor assignment remains unchanged and update
any inheritors if necessary.
In `@Maple2.Tools/Extensions/PacketExtensions.cs`:
- Line 81: In ReadArray<T> change the loop condition so it iterates from 0 up to
size (use i < size instead of i > size) so elements are actually populated; keep
using RuntimeHelpers.GetUninitializedObject(typeof(T)) cast to (T) to create the
uninitialized instance and ensure ReadFrom on that instance populates fields as
intended (ReadArray<T>, ReadFrom, and RuntimeHelpers.GetUninitializedObject are
the relevant symbols).
---
Outside diff comments:
In `@Maple2.Database/Storage/Game/GameStorage.Shop.cs`:
- Around line 60-65: GetCharacterShopItemData currently calls
.Select(ToShopItemData) which can yield null entries when ToShopItemData returns
null for models with model.Item == null; change the pipeline to filter those out
by using .Select(ToShopItemData).OfType<CharacterShopItemData>().ToList()
(removing the null-forgiving !), so the returned collection contains only
non-null CharacterShopItemData; then update ShopManager's constructor usage
(where it reads itemData.ShopId and sets Item = itemData.Item!) to remove the
unnecessary null-forgiving operator since items are now guaranteed non-null.
In `@Maple2.File.Ingest/Mapper/TableMapper.cs`:
- Around line 41-46: The private field 'language' in TableMapper is assigned in
the TableMapper constructor but never used; remove the unused field declaration
and its assignment in the constructor so only parser (created via new
TableParser(xmlReader, language)) and optionParser remain; ensure no other code
references the 'language' field before deleting it and run a build to confirm
the IDE0052 warning is resolved.
In `@Maple2.Server.Tests/Validators/CharacterNameValidatorTests.cs`:
- Around line 10-14: The test method name ValidName_ShouldReturnFalse is
incorrect and the inline comment is stale; rename the test to something like
ValidName_ShouldReturnTrue (or ValidName_ReturnsTrue) and update the comment to
state that NameValidator.ValidName returns a bool and true indicates a valid
name; keep the existing assertions using NameValidator.ValidName("ValidName")
and NameValidator.ValidName("Test123") but change the comment text to reflect
"true = valid" so test name, comment, and assertions are consistent.
- Around line 171-176: The test contains duplicate assertions repeated twice —
remove the second set of identical lines so only one assertion per Unicode name
remains; specifically, in CharacterNameValidatorTests remove the duplicate calls
to Assert.That(NameValidator.ValidName("José"), Is.True),
Assert.That(NameValidator.ValidName("Renée"), Is.True), and
Assert.That(NameValidator.ValidName("Beyoncé"), Is.True) and keep the single
original set that calls NameValidator.ValidName.
In `@Maple2.Tools/Extensions/PacketExtensions.cs`:
- Around line 64-71: The for-loop in ReadArray<T>(this IByteReader packet, int
size) has its condition inverted (uses i > size) so the body never executes;
change the loop condition to i < size (keeping var result = new T[size]; and
result[i] = packet.Read<T>();) so the method fills the array from 0 to size-1;
also consider validating that size is non-negative before allocating if you want
additional safety.
---
Nitpick comments:
In `@Maple2.File.Ingest/Mapper/TriggerMapper.cs`:
- Around line 12-13: The file currently disables ReSharper heuristic and CS0162
at file scope; remove those top-level directives and instead scope the
suppression to only the unreachable block by placing the ReSharper disable and
`#pragma` warning disable CS0162 immediately before the if
(Constant.DebugTriggers) block inside TriggerMapper (the debug-only block around
line with the if (Constant.DebugTriggers)) and then restore ReSharper and
`#pragma` warning restore CS0162 immediately after that block so the rest of the
file keeps normal unreachable-code warnings.
In `@Maple2.Model/Validators/CharacterNameValidator.cs`:
- Around line 27-29: The guard checking name.All(c => !char.IsLetterOrDigit(c))
in CharacterNameValidator.cs is redundant because the preceding regex checks
(patterns ^[A-Za-z0-9]+$ and ^[\p{L}0-9]+$) already ensure the string contains
only letters/digits; remove this dead if-block to simplify the Validate logic
(locate the conditional using name.All(...) in the validation method and delete
the entire if branch).
In `@Maple2.Server.Game/Commands/HomeCommands/RandomNumberCommand.cs`:
- Around line 9-10: Current file-wide suppression of ReSharper's
HeuristicUnreachableCode and pragma CS0162 should be narrowed: remove the
top-of-file directives and instead place scoped suppression immediately around
the unreachable code in the RandomNumberCommand class (e.g., around the specific
method or conditional block inside RandomNumberCommand.Execute/Handle) by adding
// ReSharper disable HeuristicUnreachableCode and `#pragma` warning disable CS0162
just before that block and corresponding // ReSharper restore
HeuristicUnreachableCode and `#pragma` warning restore CS0162 immediately after;
alternatively, if the unreachable path is controlled by a compile-time flag
(EnableRollEverywhere), replace the suppressed block with `#if`
ENABLEROLLEVERYWHERE ... `#endif` around the code to avoid suppressions entirely.
In `@Maple2.Server.Game/Util/TriggerStorage.cs`:
- Around line 10-11: Remove the file-wide ReSharper disable and `#pragma` warning
disable CS0162 and instead apply those suppressions locally around the specific
conditional in TryGet that checks if (Constant.DebugTriggers); i.e., delete the
top-of-file directives, then wrap only the block of code inside the TryGet (the
if (Constant.DebugTriggers) { ... }) with the ReSharper disable and `#pragma`
warning disable/restore so the suppression covers just that conditional and is
restored immediately after.
In `@Maple2.Server.Tests/Validators/CharacterNameValidatorTests.cs`:
- Around line 3-4: Instead of disabling CS0162 for the whole file, limit the
suppression to just the unreachable guards: around each occurrence of the
Unicode guard (the if (!Constant.AllowUnicodeInNames) { return; } checks used in
the CharacterNameValidatorTests) wrap or annotate only that statement block with
`#pragma` warning disable CS0162 immediately before it and `#pragma` warning restore
CS0162 immediately after it (or place the single guard line inside a scoped
pragma) so the warning remains enabled for the rest of the file.
Zintixx
approved these changes
Feb 18, 2026
Remove the implicit conversion from the DB entity and make Maple2.Model.Game.Shop.CharacterShopItemData.Item a required property. Update GameStorage to explicitly construct model CharacterShopItemData instances (populate ShopId, ShopItemId, StockPurchased, Item) and filter out nulls from the query pipeline. Adjust ShopManager to use the non-null Item without the null-forgiving operator. These changes enforce non-null Item usage and remove the hidden implicit conversion for clearer, safer conversions.
Remove the nullable annotation from NpcMetadata on IActor and Actor<T>, making it a non-nullable NpcMetadataStorage. This enforces that actors always have associated metadata and simplifies null-handling in actor-related logic. Files updated: Actor.cs, IActor.cs.
Correct the for-loop condition in PacketExtensions.ReadArray<T> from 'i > size' to 'i < size' so the array elements are actually read. Also remove a stray hidden BOM character at the top of PacketExtensions.cs to clean up the file header.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Maple2.Database/Storage/Game/GameStorage.Shop.cs (1)
64-66: Consider using.OfType<CharacterShopItemData>()for brevity.
.OfType<T>()combines the null-filter and cast into a single call, which is the idiomatic C# approach here.♻️ Suggested simplification
.Select(ToShopItemData) - .Where(data => data != null) - .Cast<CharacterShopItemData>() + .OfType<CharacterShopItemData>() .ToList();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Database/Storage/Game/GameStorage.Shop.cs` around lines 64 - 66, Replace the manual null-filter + cast chain in GameStorage.Shop.cs with the idiomatic OfType<T>() call: locate the LINQ chain that currently uses .Where(data => data != null).Cast<CharacterShopItemData>().ToList() (within the GameStorage / shop loading logic) and change it to use .OfType<CharacterShopItemData>().ToList() so nulls are filtered and items are cast in one step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.Server.Game/Model/Field/Actor/IActor.cs`:
- Line 13: Update the IActor interface's NpcMetadata property to use the C#
required-property pattern: change the declaration of NpcMetadata in IActor (the
property named NpcMetadata of type NpcMetadataStorage) to be required so
DI-injected implementations (FieldPlayer, FieldNpc, FieldActor) satisfy the
non-null initialization contract—i.e., replace the current non-nullable
init-only property with a required init-only property declaration for
NpcMetadata.
---
Duplicate comments:
In `@Maple2.Tools/Extensions/PacketExtensions.cs`:
- Around line 66-71: The loop in ReadArray<T> used the wrong condition (i >
size) causing the body to never run; change the loop condition to i < size so
result[i] is assigned from packet.Read<T>() for each index, ensuring the array
is populated rather than left with default values.
- Around line 79-84: Replace the obsolete
FormatterServices.GetSafeUninitializedObject usage with
RuntimeHelpers.GetUninitializedObject in the ReadClass<T> extension: create the
uninitialized instance via RuntimeHelpers.GetUninitializedObject(typeof(T))
inside ReadClass<T>, then call the instance method ReadFrom(packet)
(IByteDeserializable.ReadFrom) to populate fields at runtime; keep the method
signature ReadClass<T>(this IByteReader packet) where T : IByteDeserializable
and return the populated instance.
---
Nitpick comments:
In `@Maple2.Database/Storage/Game/GameStorage.Shop.cs`:
- Around line 64-66: Replace the manual null-filter + cast chain in
GameStorage.Shop.cs with the idiomatic OfType<T>() call: locate the LINQ chain
that currently uses .Where(data => data !=
null).Cast<CharacterShopItemData>().ToList() (within the GameStorage / shop
loading logic) and change it to use .OfType<CharacterShopItemData>().ToList() so
nulls are filtered and items are cast in one step.
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
2 participants
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 by CodeRabbit
Refactor
Chores